-
Notifications
You must be signed in to change notification settings - Fork 134
feat(auth): add role mapping for JWT auth claims #977
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #977 +/- ##
==========================================
+ Coverage 69.51% 71.12% +1.61%
==========================================
Files 54 55 +1
Lines 2227 2244 +17
Branches 248 249 +1
==========================================
+ Hits 1548 1596 +48
+ Misses 648 618 -30
+ Partials 31 30 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good, I just have a small suggestions for Improvement:
Cypress Tests:
- Login Tests: Consider adding a negative test case in
login.cy.js
to verify error handling for invalid login attempts.
Config Schema:
- Documentation: Enhance the
apiAuthentication
property with examples or additional documentation to clarify its usage and purpose.
@jescalada - would this be suitable for a minor bump rather than patch bump? |
@jescalada - I also see some files changed that seem unrelated to this improvement/feature? I've reviewed the PR otherwise, and happy to merge once this and the previous comment are addressed 👍 |
Hi @JamieSlome, pardon the delay! This PR would need a minor bump, since it pretty much counts as an extra feature for the JWT auth. I modified some of the Other than that, I added a doc page and E2E test based on @dcoric's review. |
This PR builds on #967 and adds the possibility of defining mappings for JWT claims into app roles, such as the admin role. This adds flexibility to the API, allowing automated tools to use admin-only endpoints.
It also adds a test suite for the JWT-related additions in both PRs.
Here's a sample
apiAuthentication
entry inproxy.config.json
to understand the structure of the mapping definition. In this case, all users with the claim"name"
set to a value of"Juan E"
will have theirreq.user.admin == true
:Example:
This PR allows assigning arbitrary roles to users for granular JWT auth. For example, the
POST /repo/
endpoint requires admin permissions:We can map our own provider-specific
claim
(name
), so that only those users whosename
matches the configured value (in this caseJuan E
), will be able to obtain that in-app role (admin
). Setting a validaccess_token
as Bearer token will allow us to get the requiredreq.user.admin
:Note that a valid access token can be obtained following the flow described in #967. This will also require a valid OIDC setup enabled (refer to #906 for a sample OIDC config).
Changelog:
claim
to app role mapping functionalityadmin
role set by defaultjwtAuthHandler
andjwtUtils
files