Add external application api endpoints #734
Conversation
lebaudantoine
force-pushed
the
external-api
branch
2 times, most recently
from
2859055 to
3306862
Compare
| - create: Create a new room owned by the user (requires 'rooms:create' scope) | ||
| """ | ||
|
|
||
| authentication_classes = [authentication.ApplicationJWTAuthentication] |
There was a problem hiding this comment.
I plan to support Resource Server as an alternative authentication on this ViewSet.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/core/external_api/viewsets.py (1)
142-183: Restrict RoomViewSet queryset to delegated user's rooms.Line 144 defines a global queryset, and
retrieve(and any future detail actions) callget_object()on it. A token with therooms:retrievescope can therefore hit/rooms/{id}on any room UUID and get data even when the delegated user has no membership, which is a high-severity data leak. Please scope the base queryset to the authenticated user just likelistdoes. For example:serializer_class = serializers.RoomSerializer + + def get_queryset(self): + queryset = super().get_queryset() + user = getattr(self.request, "user", None) + if user and user.is_authenticated: + return queryset.filter(users=user).distinct() + return queryset.none()This ensures all object lookups (retrieve/update/delete) stay within the delegated user's accessible rooms.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
docs/openapi.yaml(1 hunks)env.d/development/common.dist(1 hunks)src/backend/core/admin.py(2 hunks)src/backend/core/external_api/__init__.py(1 hunks)src/backend/core/external_api/authentication.py(1 hunks)src/backend/core/external_api/permissions.py(1 hunks)src/backend/core/external_api/serializers.py(1 hunks)src/backend/core/external_api/viewsets.py(1 hunks)src/backend/core/fields.py(1 hunks)src/backend/core/migrations/0015_application_and_more.py(1 hunks)src/backend/core/models.py(3 hunks)src/backend/core/urls.py(3 hunks)src/backend/core/utils.py(2 hunks)src/backend/meet/settings.py(3 hunks)src/helm/meet/Chart.yaml(1 hunks)src/helm/meet/templates/ingress.yaml(1 hunks)
🔇 Additional comments (13)
src/helm/meet/Chart.yaml (1)
4-4: LGTM!Version bump aligns with the new external API ingress path and backend changes.
src/backend/core/external_api/__init__.py (1)
1-1: LGTM!Clean package initialization with descriptive docstring.
src/helm/meet/templates/ingress.yaml (1)
77-90: LGTM, but verify consistency across host sections.The
/external-api/path implementation correctly mirrors the/api/pattern with proper Kubernetes version compatibility. However, note that this path is only added to the primary host section (lines 46-94). The additional hosts section (lines 95-130) does not include the/external-api/path, which may be intentional or an oversight.Verify whether
/external-api/should also be exposed for additional hosts in the second section (lines 95-130). If so, add the corresponding path block after line 126.src/backend/core/urls.py (3)
10-10: LGTM!Clean import of external API viewsets.
21-33: LGTM!Proper separation of external API routing with distinct basenames prevents conflicts with internal API.
46-53:EXTERNAL_API_VERSIONsetting verified.
settings.EXTERNAL_API_VERSIONis defined insrc/backend/meet/settings.py(line 72).src/backend/core/fields.py (2)
14-21: LGTM!Clear documentation with appropriate attribution to django-oauth-toolkit.
1-11: LGTM!Clean imports and logger initialization.
src/backend/core/external_api/permissions.py (2)
12-18: LGTM!Clear documentation of scope-based access control approach.
20-27: LGTM!Comprehensive action-to-scope mapping with proper use of scope constants.
src/backend/core/migrations/0015_application_and_more.py (3)
35-51: LGTM!The ApplicationDomain model is well-structured with proper domain validation and unique constraints. The
accept_idna=Falsesetting (line 41) correctly restricts domains to ASCII, which aligns with many email validation systems.
1-13: LGTM!Standard Django migration structure with correct dependency on previous migration.
16-34: Confirm client_secret blank flag and PostgreSQL dependency
- client_secret is blank=True—ensure allowing blank aligns with your authentication requirements.
- ArrayField requires PostgreSQL—document DB support or provide fallback.
- generate_client_id/secret use SystemRandom for cryptographic security.
| operations = [ | ||
| migrations.CreateModel( | ||
| name='Application', | ||
| fields=[ | ||
| ('id', models.UUIDField(default=uuid.uuid4, editable=False, help_text='primary key for the record as UUID', primary_key=True, serialize=False, verbose_name='id')), | ||
| ('created_at', models.DateTimeField(auto_now_add=True, help_text='date and time at which a record was created', verbose_name='created on')), | ||
| ('updated_at', models.DateTimeField(auto_now=True, help_text='date and time at which a record was last updated', verbose_name='updated on')), | ||
| ('name', models.CharField(help_text='Descriptive name for this application.', max_length=255, verbose_name='Application name')), | ||
| ('active', models.BooleanField(default=True)), | ||
| ('client_id', models.CharField(db_index=True, default=core.utils.generate_client_id, max_length=100, unique=True)), | ||
| ('client_secret', core.fields.SecretField(blank=True, db_index=True, default=core.utils.generate_client_secret, help_text='Hashed on Save. Copy it now if this is a new secret.', max_length=255)), | ||
| ('scopes', django.contrib.postgres.fields.ArrayField(base_field=models.CharField(choices=[('rooms:create', 'Create rooms'), ('rooms:list', 'List rooms'), ('rooms:retrieve', 'Retrieve room details'), ('rooms:update', 'Update rooms'), ('rooms:delete', 'Delete rooms')], max_length=50), blank=True, default=list, null=True, size=None)), | ||
| ], | ||
| options={ | ||
| 'verbose_name': 'Application', | ||
| 'verbose_name_plural': 'Applications', | ||
| 'db_table': 'meet_application', | ||
| 'ordering': ('-created_at',), | ||
| }, | ||
| ), | ||
| migrations.CreateModel( | ||
| name='ApplicationDomain', | ||
| fields=[ | ||
| ('id', models.UUIDField(default=uuid.uuid4, editable=False, help_text='primary key for the record as UUID', primary_key=True, serialize=False, verbose_name='id')), | ||
| ('created_at', models.DateTimeField(auto_now_add=True, help_text='date and time at which a record was created', verbose_name='created on')), | ||
| ('updated_at', models.DateTimeField(auto_now=True, help_text='date and time at which a record was last updated', verbose_name='updated on')), | ||
| ('domain', models.CharField(help_text='Email domain this application can act on behalf of.', max_length=253, validators=[django.core.validators.DomainNameValidator(accept_idna=False, message='Enter a valid domain')], verbose_name='Domain')), | ||
| ('application', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='allowed_domains', to='core.application')), | ||
| ], | ||
| options={ | ||
| 'verbose_name': 'Application domain', | ||
| 'verbose_name_plural': 'Application domains', | ||
| 'db_table': 'meet_application_domain', | ||
| 'ordering': ('domain',), | ||
| 'unique_together': {('application', 'domain')}, | ||
| }, | ||
| ), | ||
| ] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider rollback strategy.
The migration cleanly creates two new models with proper relationships. However, consider documenting the rollback strategy if this needs to be reversed in production, especially since external systems may depend on these tables once deployed.
🤖 Prompt for AI Agents
In src/backend/core/migrations/0015_application_and_more.py around lines 15 to
52, add explicit rollback guidance and any reversible cleanup to the migration:
insert a short top-of-file docstring that documents the production rollback
steps and impact (what drops/renames will occur, related external systems to
notify), verify the CreateModel operations are reversible (Django will reverse
CreateModel by deleting the tables) and, if external services or data
transformations must be undone, add a reversible migrations.RunPython operation
that performs necessary cleanup on forward and a safe reverse_code that restores
or compensates as required; ensure the RunPython is reversible and include notes
on any manual steps required in the docstring.
| user.token_client_id = client_id | ||
| user.token_scopes = payload.get( | ||
| "scope", "" | ||
| ).split() # Convert space-separated string to list | ||
| user.is_delegated = is_delegated |
There was a problem hiding this comment.
You are adding property not present on the user object ? If yes I think it is not a good practice. As you are returning the user and the token, both will be present in the request after, so you can use the token to have this information.
There was a problem hiding this comment.
Yes, could be fixed after merge if you want to deploy fast.
There was a problem hiding this comment.
nice, will probably do your option @qbey
lebaudantoine
force-pushed
the
external-api
branch
3 times, most recently
from
8156600 to
ada096c
Compare
| """ | ||
| # Get the current action (e.g., 'list', 'create') | ||
| action = getattr(view, "action", None) | ||
| if not action: |
There was a problem hiding this comment.
Could you just add a comment to say why we need to manage this case ? (automatic scheme documentation if I remember correctly?)
There was a problem hiding this comment.
I have updated this check to return false, if I cannot determine the action being made.
| user.token_client_id = client_id | ||
| user.token_scopes = payload.get( | ||
| "scope", "" | ||
| ).split() # Convert space-separated string to list | ||
| user.is_delegated = is_delegated |
There was a problem hiding this comment.
Yes, could be fixed after merge if you want to deploy fast.
| """Create room with secure defaults for application delegation.""" | ||
|
|
||
| # Set secure defaults | ||
| validated_data["name"] = utils.generate_room_slug() |
There was a problem hiding this comment.
What if the slug is already existing?
There was a problem hiding this comment.
I have set this case aside for now. Why? With the current slug pattern, there are approximately 10^14 possible combinations.
To put this in perspective: you would need to generate roughly 1.7 million slugs to have just a 1% chance of encountering at least one collision. For a 50% chance, the number rises to around 14 million slugs.
It's relevant, however I think it can wait for later.
lebaudantoine
force-pushed
the
external-api
branch
from
ada096c to
7daff12
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughIntroduces an external API for room management with JWT-based application authentication. Adds OpenAPI v3 spec, new models (Application, ApplicationDomain, ApplicationScope), secrets field, token and room endpoints (ApplicationViewSet, RoomViewSet), authentication and scope-based permissions, serializers, utilities for token/slug generation, admin integration (forms, inlines), URLs for external-api/v1.0, settings and environment variables for JWT and base URL, migration for new models, factories, comprehensive tests for token and rooms, and Helm updates (ingress path /external-api/, chart version bump). Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 21
♻️ Duplicate comments (3)
src/backend/core/tests/test_external_api_rooms.py (1)
75-91: Avoid flakiness in “expired token” testUsing exp = now with 0s can be timing‑sensitive. Prefer a past expiration (e.g., -1s) or a time freezer to make it deterministic.
Apply this minimal tweak:
- settings.APPLICATION_JWT_EXPIRATION_SECONDS = 0 + settings.APPLICATION_JWT_EXPIRATION_SECONDS = -1src/backend/core/external_api/serializers.py (1)
50-53: Avoid broken/duplicate slashes and provide URL fallback.If base URL is unset,
urlis omitted; provide a safe fallback. Also rstrip the base.- if settings.APPLICATION_BASE_URL: - output["url"] = f"{settings.APPLICATION_BASE_URL}/{instance.slug}" + if settings.APPLICATION_BASE_URL: + output["url"] = f"{settings.APPLICATION_BASE_URL.rstrip('/')}/{instance.slug}" + elif request: + output["url"] = request.build_absolute_uri(f"/{instance.slug}")src/backend/core/admin.py (1)
163-176: ModelForm missing Meta; admin will crash.Add Meta to bind the form to
Applicationand include editable fields.class ApplicationAdminForm(forms.ModelForm): """Custom form for Application admin with multi-select scopes.""" @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) if self.instance.pk and self.instance.scopes: self.fields["scopes"].initial = self.instance.scopes + class Meta: + model = models.Application + fields = ["name", "scopes", "client_id", "client_secret"] + widgets = { + "client_secret": forms.PasswordInput(render_value=True), + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
docs/openapi.yaml(1 hunks)env.d/development/common.dist(1 hunks)src/backend/core/admin.py(2 hunks)src/backend/core/external_api/__init__.py(1 hunks)src/backend/core/external_api/authentication.py(1 hunks)src/backend/core/external_api/permissions.py(1 hunks)src/backend/core/external_api/serializers.py(1 hunks)src/backend/core/external_api/viewsets.py(1 hunks)src/backend/core/factories.py(2 hunks)src/backend/core/fields.py(1 hunks)src/backend/core/migrations/0015_application_and_more.py(1 hunks)src/backend/core/models.py(2 hunks)src/backend/core/tests/test_external_api_rooms.py(1 hunks)src/backend/core/tests/test_external_api_token.py(1 hunks)src/backend/core/tests/test_models_applications.py(1 hunks)src/backend/core/urls.py(3 hunks)src/backend/core/utils.py(2 hunks)src/backend/meet/settings.py(2 hunks)src/helm/meet/Chart.yaml(1 hunks)src/helm/meet/templates/ingress.yaml(2 hunks)
🔇 Additional comments (20)
src/helm/meet/Chart.yaml (1)
4-4: LGTM!The version bump to
0.0.13-beta.1correctly follows semantic versioning with a pre-release identifier, as previously discussed. This aligns with the introduction of the external API feature.src/helm/meet/templates/ingress.yaml (2)
77-90: LGTM!The
/external-api/path correctly mirrors the existing/api/path configuration, with appropriate Kubernetes version checks and backend service routing.
127-140: LGTM!The duplicate
/external-api/path for theingress.hostsiteration is necessary and correctly implemented, maintaining parity with the primary host configuration.src/backend/meet/settings.py (2)
72-72: LGTM!The
EXTERNAL_API_VERSIONconstant is well-defined and aligns with the external API routing structure.
707-711: APPLICATION_BASE_URL usage is safe
Serializers checksettings.APPLICATION_BASE_URLbefore constructing URLs, so a missing value won’t cause runtime errors.src/backend/core/external_api/__init__.py (1)
1-1: LGTM!Standard package initialization with a clear docstring.
src/backend/core/urls.py (3)
10-10: LGTM!Clean import of external API viewsets.
21-33: LGTM!The external router is properly configured with clear basename choices for the application and room endpoints.
46-53: LGTM!The external API path is correctly mounted using the
EXTERNAL_API_VERSIONsetting, maintaining consistency with the main API routing pattern.src/backend/core/factories.py (3)
122-145: LGTM!The
ApplicationFactoryis well-structured with:
- Lazy function calls for credential generation to ensure unique values per instance
- A useful
with_all_scopestrait for testing full permissions- Clear field definitions aligned with the Application model
148-155: LGTM!The
ApplicationDomainFactorycorrectly uses the SubFactory pattern to associate domains with applications.
12-12: No missing credential utilities
Foundgenerate_client_idandgenerate_client_secretdefined in src/backend/core/utils.py; import ofutilsis valid.src/backend/core/fields.py (1)
22-43: LGTM with minor observation.The
pre_saveimplementation correctly:
- Detects already-hashed values to prevent double-hashing
- Hashes plain-text secrets using Django's
make_password- Sets the hashed value back on the model instance
Note: The debug logging (lines 29-31, 37-38) logs the field name and model instance but not the secret value itself, which is acceptable. However, consider using
logging.INFOor higher in production to reduce log volume.Optional refinement for production logging:
- logger.debug( + logger.info( "%s: %s is already hashed with %s.", model_instance, self.attname, hasher, )env.d/development/common.dist (1)
69-72: APPLICATION_JWT_AUDIENCE port is correct
Port 8071 is consistently used for the external-API audience (see compose.yml, VITE_API_BASE_URL, keycloak.values), while port 8072 is reserved for the meet service. No changes required.docs/openapi.yaml (1)
350-375: Spec guarantees Room.url, but serializer may omit it.Either make url nullable in the spec or ensure it’s always present in code. Minimal doc fix:
url: type: string format: uri readOnly: true + nullable: true description: Full URL to access the roomAlternatively, update the serializer to always provide a URL (e.g., fallback to request.build_absolute_uri). See serializers.py comment.
src/backend/core/external_api/serializers.py (2)
18-22: LGTM on switchingscopeto CharField.Matches OAuth2 conventions while allowing custom semantics.
66-73: No changes required: slug uniqueness and auto-generation are already implemented via a unique SlugField and clean_fields, with duplicate slugs correctly rejected.src/backend/core/external_api/viewsets.py (2)
179-194: LGTM: Excellent audit logging.The implementation correctly assigns ownership and includes comprehensive audit logging with room_id, user_id, and client_id. This is essential for tracking external application actions.
94-101: Fix the NotFound exception argument.
drf_exceptions.NotFoundexpects a string message, not a dictionary. Passing a dict will cause a runtime error.Apply this diff to fix the exception:
- except models.User.DoesNotExist as e: - raise drf_exceptions.NotFound( - { - "error": "User not found.", - } - ) from e + except models.User.DoesNotExist as e: + raise drf_exceptions.NotFound("User not found.") from eLikely an incorrect or invalid review comment.
src/backend/core/migrations/0015_application_and_more.py (1)
15-52: Migration structure is correct.The migration properly creates both models with appropriate field types, constraints, and relationships. The
unique_togetherconstraint on ApplicationDomain prevents duplicate domains per application, and the CASCADE delete ensures referential integrity.Note: The past review comment about documenting rollback strategy is valid, but since Django migrations are automatically reversible for CreateModel operations (they generate DROP TABLE statements), explicit rollback documentation is optional for this straightforward case.
We need to integrate with external applications. Objective: enable them to securely generate room links with proper ownership attribution. Proposed solution: Following the OAuth2 Machine-to-Machine specification, we expose an endpoint allowing external applications to exchange a client_id and client_secret pair for a JWT. This JWT is valid only within a well-scoped, isolated external API, served through a dedicated viewset. This commit introduces a model to persist application records in the database. The main challenge lies in generating a secure client_secret and ensuring it is properly stored. The restframework-apikey dependency was discarded, as its approach diverges significantly from OAuth2. Instead, inspiration was taken from oauthlib and django-oauth-toolkit. However, their implementations proved either too heavy or not entirely suitable for the intended use case. To avoid pulling in large dependencies for minimal utility, the necessary components were selectively copied, adapted, and improved. A generic SecretField was introduced, designed for reuse and potentially suitable for upstream contribution to Django. Secrets are exposed only once at object creation time in the Django admin. Once the object is saved, the secret is immediately hashed, ensuring it can never be retrieved again. One limitation remains: enforcing client_id and client_secret as read-only during edits. At object creation, marking them read-only excluded them from the Django form, which unintentionally regenerated new values. This area requires further refinement. The design prioritizes configurability while adhering to the principle of least privilege. By default, new applications are created without any assigned scopes, preventing them from performing actions on the API until explicitly configured. If no domain is specified, domain delegation is not applied, allowing tokens to be issued for any email domain.
Prepare for the introduction of new endpoints reserved for external applications. Configure the required router and update the Helm chart to ensure that the Kubernetes ingress properly routes traffic to these new endpoints. It is important to support independent versioning of both APIs. Base route’s name aligns with PR #195 on lasuite/drive, opened by @lunika
lebaudantoine
force-pushed
the
external-api
branch
from
7daff12 to
d1daf3f
Compare
This endpoint does not strictly follow the OAuth2 Machine-to-Machine specification, as we introduce the concept of user delegation (instead of using the term impersonation). Typically, OAuth2 M2M is used only to authenticate a machine in server-to-server exchanges. In our case, we require external applications to act on behalf of a user in order to assign room ownership and access. Since these external applications are not integrated with our authorization server, a workaround was necessary. We treat the delegated user’s email as a form of scope and issue a JWT to the application if it is authorized to request it. Using the term scope for an email may be confusing, but it remains consistent with OAuth2 vocabulary and allows for future extension, such as supporting a proper M2M process without any user delegation. It is important not to confuse the scope in the request body with the scope in the generated JWT. The request scope refers to the delegated email, while the JWT scope defines what actions the external application can perform on our viewset, matching Django’s viewset method naming. The viewset currently contains a significant amount of logic. I did not find a clean way to split it without reducing maintainability, but this can be reconsidered in the future. Error messages are intentionally vague to avoid exposing sensitive information to attackers.
Enforce the principle of least privilege by granting viewset permissions only based on the scopes included in the token. JWTs should never be issued without controlling which actions the application is allowed to perform. The first and minimal scope is to allow creating a room link. Additional actions on the viewset will only be considered after this baseline scope is in place.
From a security perspective, the list endpoint should be limited to return only rooms created by the external application. Currently, there is a risk of exposing public rooms through this endpoint. I will address this in upcoming commits by updating the room model to track the source of generation. This will also provide useful information for analytics. The API viewset was largely copied and adapted. The serializer was heavily restricted to return a response more appropriate for external applications, providing ready-to-use information for their users (for example, a clickable link). I plan to extend the room information further, potentially aligning it with the Google Meet API format. This first draft serves as a solid foundation. Although scopes for delete and update exist, these methods have not yet been implemented in the viewset. They will be added in future commits.
Document the external API using a simple Swagger file that can be opened in any Swagger editor. The content was mostly generated with the help of an LLM and has been human- reviewed. Corrections or enhancements to the documentation are welcome. Currently, my professional email address is included as a contact. A support email will be added later once available. The documentation will also be expanded as additional endpoints are added.
lebaudantoine
force-pushed
the
external-api
branch
from
d1daf3f to
1cdcdf5
Compare
Introduce ENABLE_EXTERNAL_API setting (defaults to False) to allow administrators to disable external API endpoints, preventing unintended exposure for self-hosted instances where such endpoints aren't needed or desired.
lebaudantoine
force-pushed
the
external-api
branch
from
1cdcdf5 to
bb4a818
Compare
|
3 participants
(Please refer to my commits)
Introduce integration with external applications that are not yet connected to our
authorization server, requiring an alternative approach for authentication and
authorization.
These integrations are limited in scope and should be considered a last resort.
The principle of least privilege is applied whenever possible, restricting actions
based on token scopes and, optionally, domains.
In the future, we plan to support full resource server integration to enable more
secure connections for smaller external applications.
References :