Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(bulk-import): add permissions support to the backend endpoints [RHIDP-1208] #1890

Conversation

rm3l
Copy link
Member

@rm3l rm3l commented Jul 15, 2024

What does this PR do / why we need it:

This PR adds RBAC support to the bulk-import backend plugin API endpoints.
Only admins and users with the bulk.import permission have access to the bulk import backend API endpoints.

Which issue(s) this PR fixes:

Fixes https://issues.redhat.com/browse/RHIDP-1208

PR acceptance criteria:

  • Unit tests

  • Integration tests

  • Documentation

How to test changes / Special notes to the reviewer:

Note that the GET /ping endpoint is unauthenticated and does not enforce any permission.

All the other endpoints should check for the bulk.import permission.

g, user:default/<YOUR_USERNAME>, role:default/bulk-import

p, role:default/bulk-import, bulk.import, use, allow
p, role:default/bulk-import, catalog-entity.read, read, allow
p, role:default/bulk-import, catalog-entity.create, create, allow
  • Reference the CSV file above in the RBAC Backend plugin configuration:
permission:
  enabled: true
  rbac:
    policies-csv-file: /path/to/rbac-policy.csv
    policyFileReload: true

Now try to query the bulk import backend API endpoints, e.g.:

  • Without authentication:
$ http GET http://localhost:7007/api/bulk-import-backend/organizations

{
    "error": {
        "message": "Missing credentials",
        "name": "AuthenticationError",
        "stack": "AuthenticationError: Missing credentials\n    at DefaultHttpAuthService.credentials (/home/asoro/work/projects/backstage/janus-idp/backstage-plugins/node_modules/@backstage/backend-defaults/src/entrypoints/httpAuth/httpAuthServiceFactory.ts:150:13)\n    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)"
    },
    "request": {
        "method": "GET",
        "url": "/api/bulk-import-backend/organizations"
    },
    "response": {
        "statusCode": 401
    }
}
  • Without the right bulk-import permission:
$ http -A bearer -a "<unathorized_user_jwt_token>" GET http://localhost:7007/api/bulk-import-backend/organizations

{
    "error": {
        "message": "Unauthorized",
        "name": "NotAllowedError",
        "stack": "NotAllowedError: Unauthorized\n    at permissionCheck (/home/asoro/work/projects/backstage/janus-idp/backstage-plugins/plugins/bulk-import-backend/src/helpers/auth.ts:49:11)\n    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)\n    at <anonymous> (/home/asoro/work/projects/backstage/janus-idp/backstage-plugins/plugins/bulk-import-backend/src/service/router.ts:128:7)\n    at OpenAPIBackend.handleRequest (/home/asoro/work/projects/backstage/janus-idp/backstage-plugins/node_modules/openapi-backend/src/backend.ts:299:27)"
    },
    "request": {
        "method": "GET",
        "url": "/repositories"
    },
    "response": {
        "statusCode": 403
    }
}
  • For admins or users with the expected bulk-import permission:
$ http -A bearer -a "<authorized_user_jwt_token>" GET http://localhost:7007/api/bulk-import-backend/organizations

{                                                                                                                                                  
    "errors": [],                                                                                                                                  
    "organizations": [ 
       ...
    ],
    "pagePerIntegration": 1,
    "sizePerIntegration": 20,
    "totalCount": 18
}

Copy link

openshift-ci bot commented Jul 15, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from rm3l. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rm3l rm3l force-pushed the RHIDP-1208--add-rbac-support-to-the-import-from-git-plugin branch from 75f84f2 to 98268f7 Compare July 22, 2024 09:20
@rm3l rm3l force-pushed the RHIDP-1208--add-rbac-support-to-the-import-from-git-plugin branch 3 times, most recently from c8c3195 to 8a441e9 Compare July 24, 2024 16:20
@rm3l rm3l changed the title feat(bulk-import): add RBAC support to the Bulk Import from GitHub Backend Plugin API endpoints [RHIDP 1208] feat(bulk-import): add permissions support to the Bulk Import Backend API endpoints [RHIDP 1208] Jul 24, 2024
@rm3l rm3l force-pushed the RHIDP-1208--add-rbac-support-to-the-import-from-git-plugin branch from 8a441e9 to ca377c0 Compare July 25, 2024 16:14
@rm3l rm3l changed the title feat(bulk-import): add permissions support to the Bulk Import Backend API endpoints [RHIDP 1208] feat(bulk-import): add permissions support to the Bulk Import Backend API endpoints [RHIDP-1208] Jul 25, 2024
@rm3l rm3l force-pushed the RHIDP-1208--add-rbac-support-to-the-import-from-git-plugin branch from ca377c0 to a2db3fb Compare July 26, 2024 09:29
@rm3l rm3l marked this pull request as ready for review July 26, 2024 10:13
@rm3l
Copy link
Member Author

rm3l commented Jul 26, 2024

/cc @debsmita1 @ciiay

@openshift-ci openshift-ci bot requested review from ciiay and debsmita1 July 26, 2024 10:14
@rm3l rm3l force-pushed the RHIDP-1208--add-rbac-support-to-the-import-from-git-plugin branch from a2db3fb to 41f6823 Compare July 27, 2024 07:14
@rm3l rm3l changed the title feat(bulk-import): add permissions support to the Bulk Import Backend API endpoints [RHIDP-1208] feat(bulk-import): add permissions support to the backend endpoints [RHIDP-1208] Jul 27, 2024
Copy link
Contributor

@ciiay ciiay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I only have one concern that without passing any token, I got 403: Unauthorized error instead of 401: Missing credentials error as shown in your description.

I tested with adding backend.add(import('@janus-idp/backstage-plugin-bulk-import-backend/alpha')); in packages/backend/src/index.ts and run yarn start:backstage to start the bulk-import backend. Followed the test steps to cover the below 3 test cases.

Without authentication
➜  ~ http GET http://localhost:7007/api/bulk-import-backend/organizations
{
    "error": {
        "message": "Unauthorized",
        "name": "NotAllowedError",
        "stack": "NotAllowedError: Unauthorized\n    at permissionCheck (/Users/yicai/redhat/backstage-plugins/plugins/bulk-import-backend/src/helpers/auth.ts:49:11)\n    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)\n    at <anonymous> (/Users/yicai/redhat/backstage-plugins/plugins/bulk-import-backend/src/service/router.ts:102:7)\n    at OpenAPIBackend.handleRequest (/Users/yicai/redhat/backstage-plugins/node_modules/openapi-backend/src/backend.ts:299:27)"
    },
    "request": {
        "method": "GET",
        "url": "/organizations"
    },
    "response": {
        "statusCode": 403
    }
}
Without the right `bulk-import` permission
➜  ~ http -A bearer -a "<unauthorized_user_JWT_token>" GET http://localhost:7007/api/bulk-import-backend/organizations
{
    "error": {
        "message": "Unauthorized",
        "name": "NotAllowedError",
        "stack": "NotAllowedError: Unauthorized\n    at permissionCheck (/Users/yicai/redhat/backstage-plugins/plugins/bulk-import-backend/src/helpers/auth.ts:49:11)\n    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)\n    at <anonymous> (/Users/yicai/redhat/backstage-plugins/plugins/bulk-import-backend/src/service/router.ts:102:7)\n    at OpenAPIBackend.handleRequest (/Users/yicai/redhat/backstage-plugins/node_modules/openapi-backend/src/backend.ts:299:27)"
    },
    "request": {
        "method": "GET",
        "url": "/organizations"
    },
    "response": {
        "statusCode": 403
    }
}
For admins or users with the expected bulk-import permission:
➜  ~ http -A bearer -a "<authorized_user_JWT_token>" GET http://localhost:7007/api/bulk-import-backend/organizations

{
    "errors": [],
    "organizations": [
        {
            "description": "The developer and operations friendly Kubernetes distro",
            "errors": [],
            "id": "792337",
            "name": "openshift",
            "url": "https://api.github.com/orgs/openshift"
        },
        {
            "description": "Github home of the Red Hat Developer program.",
            "errors": [],
            "id": "11033755",
            "name": "redhat-developer",
            "url": "https://api.github.com/orgs/redhat-developer"
        },
        {
            "description": "Get stuff done with Kubernetes!",
            "errors": [],
            "id": "30269780",
            "name": "argoproj",
            "url": "https://api.github.com/orgs/argoproj"
        },
        {
            "errors": [],
            "id": "77452707",
            "name": "rh-gitops-midstream",
            "url": "https://api.github.com/orgs/rh-gitops-midstream"
        },
        {
            "description": "A Red Hat sponsored community for building Internal Development Platforms and Plugins with backstage.io ",
            "errors": [],
            "id": "117844786",
            "name": "janus-idp",
            "url": "https://api.github.com/orgs/janus-idp"
        }
    ],
    "totalCount": 5
}

@rm3l rm3l force-pushed the RHIDP-1208--add-rbac-support-to-the-import-from-git-plugin branch from 41f6823 to 4704322 Compare August 19, 2024 13:38
@rm3l rm3l changed the title feat(bulk-import): add permissions support to the backend endpoints [RHIDP-1208] feat(bulk-import)!: add permissions support to the backend endpoints [RHIDP-1208] Aug 19, 2024
@rm3l
Copy link
Member Author

rm3l commented Aug 19, 2024

Looks good to me. I only have one concern that without passing any token, I got 403: Unauthorized error instead of 401: Missing credentials error as shown in your description.

I tested with adding backend.add(import('@janus-idp/backstage-plugin-bulk-import-backend/alpha')); in packages/backend/src/index.ts and run yarn start:backstage to start the bulk-import backend. Followed the test steps to cover the below 3 test cases.

Without authentication

➜  ~ http GET http://localhost:7007/api/bulk-import-backend/organizations
{
    "error": {
        "message": "Unauthorized",
        "name": "NotAllowedError",
        "stack": "NotAllowedError: Unauthorized\n    at permissionCheck (/Users/yicai/redhat/backstage-plugins/plugins/bulk-import-backend/src/helpers/auth.ts:49:11)\n    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)\n    at <anonymous> (/Users/yicai/redhat/backstage-plugins/plugins/bulk-import-backend/src/service/router.ts:102:7)\n    at OpenAPIBackend.handleRequest (/Users/yicai/redhat/backstage-plugins/node_modules/openapi-backend/src/backend.ts:299:27)"
    },
    "request": {
        "method": "GET",
        "url": "/organizations"
    },
    "response": {
        "statusCode": 403
    }
}

@ciiay Thanks for testing this. I tested again and, as expected, I am getting a 401 when no token is passed. See below.

$ http GET http://localhost:7007/api/bulk-import/organizations
HTTP/1.1 401 Unauthorized
Connection: keep-alive
Content-Length: 555
Content-Security-Policy: default-src 'self';base-uri 'self';font-src 'self' https: data:;frame-ancestors 'self';img-src 'self' data:;object-src 'none';script-src 'self' 'unsafe-eval';script-src-attr 'none';style-src 'self' https: 'unsafe-inline';upgrade-insecure-requests;connect-src 'self' http: https:
Content-Type: application/json; charset=utf-8
Date: Mon, 19 Aug 2024 13:57:21 GMT
ETag: W/"22b-Uv/Mb8uC/9+NyqsJArye/dzts4E"
Keep-Alive: timeout=5
Referrer-Policy: no-referrer
Strict-Transport-Security: max-age=15552000; includeSubDomains
Vary: Accept-Encoding
X-Content-Type-Options: nosniff
X-DNS-Prefetch-Control: off
X-Download-Options: noopen
X-Frame-Options: SAMEORIGIN
X-Permitted-Cross-Domain-Policies: none
X-XSS-Protection: 0

{
    "error": {
        "message": "Missing credentials",
        "name": "AuthenticationError",
        "stack": "AuthenticationError: Missing credentials\n    at DefaultHttpAuthService.credentials (/home/asoro/work/projects/backstage/janus-idp/backstage-plugins/node_modules/@backstage/backend-defaults/src/entrypoints/httpAuth/httpAuthServiceFactory.ts:150:13)\n    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)"
    },
    "request": {
        "method": "GET",
        "url": "/api/bulk-import/organizations?pagePerIntegration=1&sizePerIntegration=2000"
    },
    "response": {
        "statusCode": 401
    }
}

Can you please run http with the --verbose flag and share the output? It would be great to share the backend logs as well. Thanks.

@rm3l rm3l requested review from debsmita1 and ciiay August 19, 2024 14:10
PatAKnight
PatAKnight previously approved these changes Aug 19, 2024
Copy link
Member

@PatAKnight PatAKnight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested as well and it worked for me, lgtm

Without authentication

curl "http://localhost:7007/api/bulk-import/organizations" | jq

{
  "error": {
    "name": "AuthenticationError",
    "message": "Missing credentials",
    "stack": "AuthenticationError: Missing credentials\n    at DefaultHttpAuthService.credentials (/home/patrick/Documents/janus/backstage-plugins/node_modules/@backstage/backend-defaults/src/entrypoints/httpAuth/httpAuthServiceFactory.ts:150:13)\n    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)"
  },
  "request": {
    "method": "GET",
    "url": "/api/bulk-import/organizations"
  },
  "response": {
    "statusCode": 401
  }
}

With deny

curl "http://localhost:7007/api/bulk-import/organizations" -H "Authorization: Bearer $token" | jq

{
  "error": {
    "name": "NotAllowedError",
    "message": "Unauthorized",
    "stack": "NotAllowedError: Unauthorized\n    at permissionCheck (/home/patrick/Documents/janus/backstage-plugins/plugins/bulk-import-backend/src/helpers/auth.ts:49:11)\n    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)\n    at <anonymous> (/home/patrick/Documents/janus/backstage-plugins/plugins/bulk-import-backend/src/service/router.ts:102:7)\n    at OpenAPIBackend.handleRequest (/home/patrick/Documents/janus/backstage-plugins/node_modules/openapi-backend/src/backend.ts:299:27)"
  },
  "request": {
    "method": "GET",
    "url": "/organizations"
  },
  "response": {
    "statusCode": 403
  }
}

With allow

curl "http://localhost:7007/api/bulk-import/organizations" -H "Authorization: Bearer $token" | jq

{
  "errors": [],
  "organizations": [
    {
      "id": "152328355",
      "name": "test-org-pat",
      "url": "https://api.github.com/users/test-org-pat",
      "totalRepoCount": 3,
      "errors": []
    }
  ],
  "totalCount": 1,
  "pagePerIntegration": 1,
  "sizePerIntegration": 20
}

These endpoints will only be accessible by admins or
users with the 'bulk.import' permission.
…ckend plugins

Note that this changes the default API base path from `/api/bulk-import-backend` to `/api/bulk-import`
@rm3l rm3l force-pushed the RHIDP-1208--add-rbac-support-to-the-import-from-git-plugin branch from 4704322 to 7b85194 Compare August 20, 2024 08:00
@openshift-ci openshift-ci bot removed the lgtm label Aug 20, 2024
Copy link

sonarcloud bot commented Aug 20, 2024

@rm3l
Copy link
Member Author

rm3l commented Aug 20, 2024

Rebased and force-pushed to fix Git conflicts with the following files:

  • plugins/bulk-import-backend/src/service/router.test.ts

@rm3l rm3l changed the title feat(bulk-import)!: add permissions support to the backend endpoints [RHIDP-1208] feat(bulk-import): add permissions support to the backend endpoints [RHIDP-1208] Aug 20, 2024
Copy link
Collaborator

@AndrienkoAleksandr AndrienkoAleksandr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. @debsmita1 Can you confirm that bulk import frontend plugin will be still working after changing endpoint url?

@debsmita1
Copy link
Member

debsmita1 commented Aug 20, 2024

Looks good to me. @debsmita1 Can you confirm that bulk import frontend plugin will be still working after changing endpoint url?

@AndrienkoAleksandr The frontend with the API integration hasn't merged yet. Once this one gets merged, I will update my PR

@openshift-ci openshift-ci bot added the lgtm label Aug 20, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 638df72 into janus-idp:main Aug 20, 2024
12 checks passed
@rm3l rm3l deleted the RHIDP-1208--add-rbac-support-to-the-import-from-git-plugin branch August 20, 2024 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants