Skip to content

feat(auth): Add JWT auth for API routes #967

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

Merged
merged 24 commits into from
May 29, 2025

Conversation

jescalada
Copy link
Contributor

@jescalada jescalada commented Mar 28, 2025

Fixes #905.

This PR aims to add an optional security layer by using a JWT check for /repo, /user, and /push API endpoints.

  • If jwt auth method is present, enabled in the proxy.config.json AND JWT_SECRET environment variable is set:

    • When logging in through the UI, you can access data as usual
    • Direct API requests require a JWT bearer token to go through
      • The JWT token is validated through the configuration in the jwtConfig in proxy.config.json.
      • The user issues their own JWT using the clientID, authorityURL (and potentially, the expectedAudience) provided in the config
  • If jwt is not enabled OR JWT_SECRET is not present, it works as it used to.

    • All API resources are accessible even to unlogged users
    • No JWT is required

To activate the JWT check, you must fill in the JWT details in proxy.config.json. The following will let you verify against my Google OIDC testing app:

{
  "type": "jwt",
  "enabled": true,
  "jwtConfig": {
    "clientID": "1009968223893-u92qq6itk7ej5008o4174gjubs5lhorg.apps.googleusercontent.com",
    "authorityURL": "https://accounts.google.com"
  }
}

You can manually generate a sample JWT by following these steps:

  1. Access the following link in your browser to get an auth code:
https://accounts.google.com/o/oauth2/auth?client_id=1009968223893-u92qq6itk7ej5008o4174gjubs5lhorg.apps.googleusercontent.com&redirect_uri=http://localhost:8080/api/auth/oidc/callback&response_type=code&scope=openid%20email%20profile

Upon successful login, it will redirect to a URL that contains an authorization code as a query parameter:

image

  1. Copy the following into a terminal and replace AUTHORIZATION_CODE below with the code obtained in 1):
curl -X POST "https://oauth2.googleapis.com/token" \
     -H "Content-Type: application/x-www-form-urlencoded" \
     -d "client_id=1009968223893-u92qq6itk7ej5008o4174gjubs5lhorg.apps.googleusercontent.com" \
     -d "client_secret=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" \
     -d "code=AUTHORIZATION_CODE" \
     -d "grant_type=authorization_code" \
     -d "redirect_uri=http://localhost:8080/api/auth/oidc/callback"

Note that the generated JWT has a 1-hour expiry date.

  1. The JWT is the id_token, which starts with ey. If using Postman, this can be tested by adding an Auth header of type Bearer and pasting the token:

image

If successful, submitting that request will return the list of repos. If there is a problem with either the JWT setup, or the token validity, it will throw an error like this:

image

Note: Although my Google app secrets are exposed, only registered emails can use them. Let me know if you'd like to test it out, and I can add your email to the app!

Changelog

  • Add a jwtAuthHandler middleware for /repo, /user, and /push endpoints
    • JWT check is optional based on config
    • Descriptive 401 errors are returned if failed to verify
      • Missing token
      • Missing configuration
      • Invalid token format/algorithm
      • Expired token

Copy link

netlify bot commented Mar 28, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit 6912ba1
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/683835d4d632a30008722b05

Copy link

codecov bot commented Apr 4, 2025

Codecov Report

Attention: Patch coverage is 38.57143% with 43 lines in your changes missing coverage. Please review.

Project coverage is 48.70%. Comparing base (98f6db8) to head (6912ba1).
Report is 25 commits behind head on main.

Files with missing lines Patch % Lines
src/service/passport/jwtAuthHandler.js 21.15% 41 Missing ⚠️
src/service/passport/oidc.js 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #967      +/-   ##
==========================================
- Coverage   49.14%   48.70%   -0.44%     
==========================================
  Files          52       53       +1     
  Lines        2110     2166      +56     
  Branches      241      242       +1     
==========================================
+ Hits         1037     1055      +18     
- Misses       1036     1074      +38     
  Partials       37       37              

☔ 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.

@jescalada
Copy link
Contributor Author

jescalada commented Apr 4, 2025

Seems there's an issue with the licenses in the passport-jwt package, which depends on jsonwebtoken which itself depends on lodash. It's weird because I inspected every package and all of them have MIT licenses, so I'm not sure where the CC0-1.0 part is coming from:

The following dependencies have incompatible licenses:
  package-lock.json » elliptic@6.6.1 – License: MIT AND OFL-1.1
  package-lock.json » lodash.includes@4.3.0 – License: CC0-1.0 AND MIT
  package-lock.json » lodash.isinteger@4.0.4 – License: CC0-1.0 AND MIT
  package-lock.json » lodash.once@4.1.1 – License: CC0-1.0 AND MIT

Here's an explanation on why the CC0-1.0 AND MIT license combination is incompatible.

Copy link

@sam-holmes2 sam-holmes2 left a comment

Choose a reason for hiding this comment

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

Great contribution, thanks! Left a few minor comments, main concern is the initial hardcoded client ID and secret. Also believe a lot of these changes have already been approved / merged from your other PRs so may be some conflicts to resolve

@jescalada jescalada requested a review from sam-holmes2 May 29, 2025 08:45
@jescalada
Copy link
Contributor Author

Pinging @JamieSlome for thoughts on this and #963 (which is the parent branch).

Thanks!

Copy link
Member

@JamieSlome JamieSlome left a comment

Choose a reason for hiding this comment

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

LGTM! 🍰

@JamieSlome JamieSlome enabled auto-merge May 29, 2025 10:24
@JamieSlome JamieSlome dismissed sam-holmes2’s stale review May 29, 2025 10:27

Follow up review completed :)

@JamieSlome JamieSlome merged commit 0d83223 into finos:main May 29, 2025
14 checks passed
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.

OIDC Login Implementation
3 participants