-
Notifications
You must be signed in to change notification settings - Fork 90
Support resource server authentification #781
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
Conversation
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
qbey
left a comment
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.
I think you are right in the way you want to manage this, from what I see, I think we should:
- Rewrite our OIDC authentication backend to make the introspection earlier.
- Pass the introspected payload to the get_or_create user directly
=> needs django-lasuite update (or local dev in meet while the lib is not ready)
| return None | ||
|
|
||
| try: | ||
| token = auth_header[1].decode("utf-8") |
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.
I've just discovered DRF uses .decode(authentication.HTTP_HEADER_ENCODING) maybe we should do the same.
0770fdb to
9201057
Compare
9201057 to
533a977
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/backend/core/external_api/authentication.py (1)
42-45: Align token decoding with DRF’s header encoding constantConsider decoding with DRF’s
HTTP_HEADER_ENCODINGinstead of hard‑coding"utf-8"for consistency:- try: - token = auth_header[1].decode("utf-8") + try: + token = auth_header[1].decode(authentication.HTTP_HEADER_ENCODING)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/meet.yml(1 hunks)compose.yml(3 hunks)docker/resource-server/compose.yml(1 hunks)env.d/development/common.dist(2 hunks)src/backend/core/external_api/authentication.py(4 hunks)src/backend/core/external_api/viewsets.py(2 hunks)src/backend/core/tests/test_external_api_rooms.py(3 hunks)src/backend/meet/settings.py(4 hunks)src/backend/meet/urls.py(1 hunks)src/backend/pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/backend/core/tests/test_external_api_rooms.py (2)
src/backend/core/factories.py (2)
RoomFactory(62-70)UserFactory(17-28)src/backend/core/models.py (2)
RoomAccessLevel(90-95)User(135-225)
🔇 Additional comments (19)
src/backend/meet/urls.py (1)
18-18: LGTM!The inclusion of OIDC resource server URLs at the root path is appropriate and aligns with the PR objectives to add OAuth Resource Server authentication.
.github/workflows/meet.yml (1)
186-189: LGTM!The OIDC Resource Server environment variables are correctly configured for testing with appropriate example values. This aligns with the new OIDC resource server authentication feature.
env.d/development/common.dist (2)
35-36: LGTM!The OIDC introspection endpoint and OP URL are necessary for the resource server authentication flow introduced in this PR. The configuration correctly points to the local Keycloak instance used in development.
Note: Regarding @qbey's comment about usefulness in dev mode - the test suite in
test_external_api_rooms.pyactively uses these settings to test OIDC authentication flows, so they are indeed necessary for development and testing.
50-51: LGTM!The OIDC Resource Server client credentials are properly configured for development. These values align with the Keycloak setup and are required for testing the resource server authentication flow.
compose.yml (3)
93-95: LGTM!Adding the resource-server network to app-dev enables proper network isolation for OIDC resource server communication while maintaining connectivity on the default network.
151-153: LGTM!Adding the resource-server network to nginx is appropriate as it acts as a reverse proxy for Keycloak, which needs to be accessible on the resource server network.
308-310: Network naming is consistent across compose files.The verification confirms the original concern was addressed correctly:
compose.ymldefinesresource-server(auto-prefixed tomeet_resource-server)docker/resource-server/compose.ymlreferences the external networkmeet_resource-serverBoth files are referencing the same network with consistent naming. The architecture is sound: the primary compose file creates the network, and the resource-server compose file consumes it as external.
src/backend/core/external_api/viewsets.py (2)
12-12: LGTM!The import of ResourceServerAuthentication from django-lasuite is appropriate for enabling OIDC resource server authentication.
153-156: LGTM!Adding ResourceServerAuthentication to the authentication classes enables dual authentication for the RoomViewSet. DRF will try each authentication class in order, allowing the API to accept both Application JWT tokens and OIDC resource server tokens. This aligns perfectly with the PR objectives.
src/backend/core/tests/test_external_api_rooms.py (6)
13-13: LGTM!Adding the
responseslibrary import is necessary for mocking HTTP responses in the OIDC introspection tests.
20-20: LGTM!Adding imports for
RoomAccessLevelandUsermodels is necessary for the new resource server authentication tests.
94-117: LGTM!The test correctly simulates the ResourceServerAuthentication behavior when an invalid token is provided. The status code change from 401 to 400 is properly explained: ResourceServerAuthentication raises
SuspiciousOperationwhen theissclaim is missing from an inactive token, which Django converts to a 400 Bad Request.This demonstrates proper handling of the authentication chain where ResourceServerAuthentication is tried after ApplicationJWTAuthentication fails.
354-409: LGTM!This test comprehensively validates the user creation flow during first-time OIDC authentication:
- Verifies user doesn't exist initially
- Confirms correct backend class is configured
- Mocks the OIDC introspection endpoint appropriately
- Validates successful authentication (200 response)
- Confirms user is created with correct
subidentifier- Verifies email is None (as noted in PR objectives, email is not in introspection response)
The test aligns with the PR's stated user creation behavior.
411-472: LGTM!This test thoroughly validates the resource server authentication flow:
- Creates a user with a specific sub identifier
- Creates multiple rooms with different access levels and user associations
- Mocks the OIDC introspection response correctly
- Verifies that only the user's accessible room is returned (proper filtering)
- Validates the expected room ID in the response
The test correctly exercises the interaction between authentication and authorization layers.
474-514: LGTM!This test properly validates scope-based authorization:
- Mocks an OIDC introspection response with insufficient scopes (missing
rooms:list)- Verifies the request is denied with 403 Forbidden
- Ensures authorization checks are enforced even with valid authentication
This is an important security test that confirms the permission layer works correctly with resource server authentication.
src/backend/pyproject.toml (1)
35-35: No issues found. The dependency version is current and secure.Verification confirms that django-lasuite 0.0.19 is the latest stable version on PyPI and has no known security vulnerabilities.
src/backend/core/external_api/authentication.py (2)
7-11: Imports and aliases look consistentUsing
jwtaspyJwtand importingLaSuiteBackend/SuspiciousOperationaligns with the usages below; no issues spotted here.
31-39: Authorization header parsing and fallback behavior are soundOnly well‑formed
Bearer <token>headers are handled here; missing/non‑Bearer headers correctly defer to other authentication classes, while malformed Bearer headers fail fast withAuthenticationFailed.src/backend/meet/settings.py (1)
13-14: Localtoo-many-linessuppression is acceptable for this monolithic settings moduleDisabling
too-many-linesat the top of this central settings file is a pragmatic choice; no change requested here.
| If token is invalid, defer to next authentication backend. | ||
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.
🧹 Nitpick | 🔵 Trivial
Revisit behavior on generic InvalidTokenError and add logging
Right now any pyJwt.InvalidTokenError (e.g. bad signature, malformed token) silently returns None, deferring to the next backend, while only specific errors raise AuthenticationFailed. This can make debugging harder and slightly blur the distinction between “no credentials” and “invalid credentials”.
I suggest:
- At least log these invalid tokens (debug or warning) before returning
None. - Double‑check that, for endpoints where this backend is the expected mechanism (e.g. external app JWTs), deferring instead of raising doesn’t weaken your intended security/UX semantics.
Also applies to: 65-83
🤖 Prompt for AI Agents
In src/backend/core/external_api/authentication.py around lines 52-53 (and
similarly for the block covering lines 65-83), update the exception handling for
pyJwt.InvalidTokenError to log the incident before returning None: catch
InvalidTokenError, write a structured log entry (warning or debug) that includes
the exception message and minimal token/context info (avoid logging full
sensitive token), then return None to defer to the next backend; additionally,
add a TODO or comment noting to re-evaluate raising AuthenticationFailed for
endpoints that rely solely on this backend.
Define Docker network enabling external service providers to share Keycloak instance with local development stack, supporting OIDC authentication flow where services obtain tokens from shared Keycloak then pass to Meet for introspection and validation. Prepares Meet infrastructure for multi-service authentication architecture though external service provider Docker Compose integration changes remain in separate repository.
533a977 to
2b2b6d4
Compare
Upgrade django-lasuite to v0.0.19 to benefit from the latest resource server authentication backend. Thanks @qbey for your work. For my needs, @qbey refactored the class in #46 on django-lasuite. Integrate ResourceServerAuthentication in the relevant viewset. The integration is straightforward since most heavy lifting was done in the external-api viewset when introducing the service account. Slightly modify the existing service account authentication backend to defer to ResourceServerAuthentication if a token is not recognized. Override user provisioning behavior in ResourceServerBackend: now, a user is automatically created if missing, based on the 'sub' claim (email is not yet present in the introspection response). Note: shared/common implementation currently only retrieves users, failing if the user does not exist.
da79d06 to
a4ff466
Compare
|


Purpose
We want to add OAuth Resource Server authentication to allow any application connected to our SSO to securely perform CRUD operations on our external viewset. This enables server-to-server generation of meeting links that include user identification, without requiring user-initiated auth flows.
Our first integration will be with a major client application with over 50k DAUs, enabling users to generate meeting links directly from their calendar.
Proposal
From @qbey's work on django-lasuite, declare a resource server authentification class and backend, that receive an access token, introspect it and authenticate the user on our API.
If the user doesn’t exist yet, we create a new User using the only reliable identifier available to us, the sub claim. While creating a user without an email address is not ideal, our current SSO implementation does not include email in the introspection response, leaving us with no alternative for now.
Additionally, the OAuth spec does not require the sub claim to be included in the introspection response, it’s entirely optional. Email isn’t mentioned at all among the recommended optional claims, which further limits the identifying information we can rely on.
Tests are missing, I am looking for some early reviews.