Skip to content

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

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

jescalada
Copy link
Contributor

@jescalada jescalada commented Apr 13, 2025

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 in proxy.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 their req.user.admin == true:

"apiAuthentication": [
    {
      "type": "jwt",
      "enabled": true,
      "jwtConfig": {
        "clientID": "<my-client-id>",
        "authorityURL": "https://accounts.google.com",
        "roleMapping": {
          "admin": {
            "name": "Juan E"
          }
        }
      }
    }
  ]

Example:

This PR allows assigning arbitrary roles to users for granular JWT auth. For example, the POST /repo/ endpoint requires admin permissions:

router.post('/', async (req, res) => {
  if (req.user && req.user.admin) {
    // Do something
  } else {
    res.status(401).send({
      message: 'You are not authorised to perform this action...',
    });
  }
});

We can map our own provider-specific claim (name), so that only those users whose name matches the configured value (in this case Juan E), will be able to obtain that in-app role (admin). Setting a valid access_token as Bearer token will allow us to get the required req.user.admin:

image

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:

  • Add JWT claim to app role mapping functionality
  • Make JWT role mappings configurable
    • admin role set by default
  • Add unit tests for jwtAuthHandler and jwtUtils files
  • Fix minor error handling issues
    • Frontend now shows descriptive messages on users/repos/pushes fetch failure

Copy link

netlify bot commented Apr 13, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 ready!

Name Link
🔨 Latest commit 9f554b7
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/6852cc04c3f1980007eda859
😎 Deploy Preview https://deploy-preview-977--endearing-brigadeiros-63f9d0.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@jescalada jescalada linked an issue Apr 13, 2025 that may be closed by this pull request
Copy link

codecov bot commented Apr 16, 2025

Codecov Report

Attention: Patch coverage is 78.57143% with 12 lines in your changes missing coverage. Please review.

Project coverage is 71.12%. Comparing base (d13686d) to head (9f554b7).

Files with missing lines Patch % Lines
src/service/passport/oidc.js 0.00% 6 Missing ⚠️
src/service/routes/repo.js 25.00% 3 Missing ⚠️
src/service/passport/jwtUtils.js 94.44% 2 Missing ⚠️
src/service/passport/jwtAuthHandler.js 83.33% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@dcoric dcoric left a 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
Copy link
Contributor Author

jescalada commented Jun 13, 2025

Confirming that JWT flow works correctly after merge conflict fixes (needed to reconcile OIDC changes):

image

Confirming that role assignment works correctly too (on admin-only endpoint):

image

@jescalada jescalada requested a review from dcoric June 15, 2025 12:06
@JamieSlome
Copy link
Member

@jescalada - would this be suitable for a minor bump rather than patch bump?

@JamieSlome
Copy link
Member

JamieSlome commented Jun 16, 2025

@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 👍

@jescalada
Copy link
Contributor Author

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 src/ui files to display an error message in case the JWT is active (which makes the frontend API calls fail). Ideally, this would be solved by adding a form/prompt to set the token in the frontend, but I think this might be overkill to add right now unless someone specifically asks for it. I also improved the error handling so that users can see the error from the backend instead of just "Something went wrong...". Most of the other stuff is just refactoring/cleaning up the early JWT implementation.

Other than that, I added a doc page and E2E test based on @dcoric's review.

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.

Add mapping for JWT claims into app roles
3 participants