-
Notifications
You must be signed in to change notification settings - Fork 130
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
Free license according to FSF (https://www.gnu.org/licenses/license-list.html#SILOFL)
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #977 +/- ##
==========================================
+ Coverage 49.65% 51.86% +2.21%
==========================================
Files 48 51 +3
Lines 1718 1799 +81
Branches 175 176 +1
==========================================
+ Hits 853 933 +80
- Misses 841 842 +1
Partials 24 24 ☔ 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.
@@ -19,6 +19,18 @@ describe('Login page', () => { | |||
cy.get('[data-test="login"]').should('exist'); | |||
}); | |||
|
|||
it('should redirect to repo list on valid login', () => { |
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.
The new test "should redirect to repo list on valid login" is good, but consider adding a negative test case (e.g., invalid login) to ensure the login flow handles errors gracefully.
@@ -98,6 +109,22 @@ | |||
"contactEmail": "", | |||
"csrfProtection": true, | |||
"plugins": [], | |||
"apiAuthentication": [ |
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.
The new apiAuthentication property is well-defined, but consider adding examples or additional documentation to clarify how this property should be used in practice. This will help other developers understand its purpose and usage.
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
:Changelog:
admin
role set by defaultjwtAuthHandler
andjwtUtils
files