feat(OIDC): Add Trust Relationship CRUD API#8042
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
github-actions
Bot
added
api
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds a Django trust relationships application with a soft-deletable model, conditional issuer/audience uniqueness, backing master API key lifecycle management, validation, admin registration, and structured logging. It exposes organisation-scoped CRUD endpoints restricted to authenticated organisation administrators and excludes machine credentials. Master API key listings hide relationship backing keys. Integration and unit tests cover validation, authentication, CRUD behaviour, key synchronisation, deletion, uniqueness, and logging. Runtime logger configuration and event documentation are updated. Estimated code review effort: 4 (Complex) | ~45 minutes 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 |
github-actions
Bot
added
feature
Docker builds report
|
4ea4cd6 to
70667c2
Compare
github-actions
Bot
added
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6b5d1f87-d919-473d-8dd9-dfead0062902
📒 Files selected for processing (19)
Dockerfileapi/api_keys/views.pyapi/app/settings/common.pyapi/organisations/urls.pyapi/tests/integration/trust_relationships/__init__.pyapi/tests/integration/trust_relationships/conftest.pyapi/tests/integration/trust_relationships/test_viewset.pyapi/tests/unit/trust_relationships/__init__.pyapi/tests/unit/trust_relationships/test_services.pyapi/trust_relationships/__init__.pyapi/trust_relationships/admin.pyapi/trust_relationships/apps.pyapi/trust_relationships/migrations/0001_initial.pyapi/trust_relationships/migrations/__init__.pyapi/trust_relationships/models.pyapi/trust_relationships/serializers.pyapi/trust_relationships/services.pyapi/trust_relationships/views.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.md
70667c2 to
5220d77
Compare
This comment was marked as outdated.
This comment was marked as outdated.
github-actions
Bot
added
feature
Visual Regression19 screenshots compared. See report for details. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
api/trust_relationships/admin.py (1)
6-10: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDjango admin delete bypasses backing-key revocation, leaving a live, invisible credential.
TrustRelationshipAdmindoesn't override the delete behaviour. The default admin delete flow callsTrustRelationship.delete()(single) orqueryset.delete()(bulk), both of which soft-delete the row directly without going throughservices.delete_trust_relationship(). As a result, the backingMasterAPIKeyis never revoked.Please override
delete_modelanddelete_querysetto calldelete_trust_relationship, or disable the delete permission on this admin entirely.🛡️ Proposed fix routing admin deletes through the service layer
from django.contrib import admin from trust_relationships.models import TrustRelationship +from trust_relationships.services import delete_trust_relationship `@admin.register`(TrustRelationship) class TrustRelationshipAdmin(admin.ModelAdmin[TrustRelationship]): - list_display = ("name", "organisation", "issuer", "audience", "created_at") + list_display = ("name", "organisation", "issuer", "audience", "created_at") + list_select_related = ("organisation",) list_filter = ("issuer",) search_fields = ("name", "issuer", "audience") + + def delete_model(self, request, obj): + delete_trust_relationship(trust_relationship=obj) + + def delete_queryset(self, request, queryset): + for obj in queryset: + delete_trust_relationship(trust_relationship=obj)api/trust_relationships/serializers.py (1)
22-28: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
is_adminshould not default at the serializer field level.A full
PUTthat omits theis_adminfield will injectTrueintovalidated_data, causingupdate_trust_relationship(...)to flip an existing non-admin trust relationship back to admin, even though the caller never explicitly requested the change.Please move the default logic into the
create()method or makeis_adminexplicitly required.🛡️ Proposed fix
class TrustRelationshipSerializer(serializers.ModelSerializer[TrustRelationship]): - is_admin = serializers.BooleanField(default=True) + is_admin = serializers.BooleanField(required=False) claim_rules = ClaimRuleSerializer(many=True, required=False) master_api_key_id = serializers.CharField(read_only=True)And in the
createmethod:def create(self, validated_data: dict[str, Any]) -> TrustRelationship: + validated_data.setdefault("is_admin", True) validated_data.setdefault("claim_rules", []) return create_trust_relationship(**validated_data)Also applies to: 73-83
api/trust_relationships/services.py (1)
48-53: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAudit logs fire even if an enclosing transaction rolls back.
The service functions emit their structured logs immediately after their own inner
transaction.atomic()block exits. If any of these are invoked from within an outer transaction (such asATOMIC_REQUESTS), the inneratomic()block acts merely as a savepoint. If the outer transaction subsequently rolls back, the log line will have already been emitted, producing an audit trail that does not match the persisted state.Please wrap the log emissions in
transaction.on_commit(...)so they only fire once the database changes are durably committed.
api/trust_relationships/services.py#L48-L53: wrap the "created" log emission intransaction.on_commit(...).api/trust_relationships/services.py#L79-L84: wrap the "updated" log emission intransaction.on_commit(...).api/trust_relationships/services.py#L94-L98: wrap the "deleted" log emission intransaction.on_commit(...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 799b0dc7-b3c6-4177-b82d-53138d3683ee
📒 Files selected for processing (21)
Dockerfileapi/api_keys/views.pyapi/app/settings/common.pyapi/organisations/models.pyapi/organisations/urls.pyapi/tests/integration/trust_relationships/__init__.pyapi/tests/integration/trust_relationships/conftest.pyapi/tests/integration/trust_relationships/test_viewset.pyapi/tests/unit/trust_relationships/__init__.pyapi/tests/unit/trust_relationships/test_services.pyapi/trust_relationships/__init__.pyapi/trust_relationships/admin.pyapi/trust_relationships/apps.pyapi/trust_relationships/migrations/0001_initial.pyapi/trust_relationships/migrations/__init__.pyapi/trust_relationships/models.pyapi/trust_relationships/serializers.pyapi/trust_relationships/services.pyapi/trust_relationships/views.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.mdopenapi.yaml
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8042 +/- ##
========================================
Coverage 98.64% 98.65%
========================================
Files 1506 1517 +11
Lines 59573 59877 +304
========================================
+ Hits 58767 59071 +304
Misses 806 806 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
feature
94dff4e to
4a9b748
Compare
github-actions
Bot
added
docs
4a9b748 to
94dff4e
Compare
github-actions
Bot
added
docs
94dff4e to
1c5bc06
Compare
github-actions
Bot
added
docs
… CRUD API Each trust relationship owns a hidden backing MasterAPIKey whose plaintext is discarded at creation: it carries is_admin and RBAC role attachments, and attributes audit records, but can never authenticate a request itself. Backing keys are excluded from the master-api-keys list endpoint, and master API keys cannot manage trust relationships. beep boop
The default admin delete flow calls TrustRelationship.delete() and queryset.delete() directly, soft-deleting the row without revoking the backing MasterAPIKey. The soft-deleted row also keeps the key out of the master-api-keys list (which filters on trust_relationship__isnull=True), making the still-active key un-revocable from the dashboard. Route delete_model and delete_queryset through delete_trust_relationship so the backing key is always revoked. beep boop
The organisation column in list_display triggers a query per row in the changelist. Add list_select_related to fetch it in the initial query. beep boop
The get_authenticators override that drops MasterAPIKeyAuthentication was duplicated in MasterAPIKeyViewSet and TrustRelationshipViewSet. Since it enforces a security-relevant invariant (machine credentials must not manage credentials), extract it into ExcludeMasterAPIKeyAuthenticationMixin so the two viewsets cannot diverge. beep boop
drf-spectacular could not introspect get_queryset (it raises on self.kwargs during schema generation), so organisation_pk and id were emitted as type: string rather than integer. Guard get_queryset with swagger_fake_view, matching the pattern used elsewhere, and regenerate the schema so the path parameters are typed as integer. beep boop
The name, issuer and audience columns had no help_text, which the model-help-text convention flags. Add it so the intent is documented on the model, in the migration, and in the generated OpenAPI schema. beep boop
1c5bc06 to
09c0d03
Compare
github-actions
Bot
added
docs
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18527 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #18527 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #18527 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #18527 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
1 participant
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Contributes to #8040
In this PR, we add a CRUD API for managing Trust Relationships.
Trust relationships are stored as a one-to-one metadata row for a
MasterAPIKey, so the RBAC logic and audit logs are naturally inherited. Trust relationships' master API keys are impossible to use on their own, and excluded from existing master API Key APIs.The CRUD API at
/api/v1/organisations/{id}/trust-relationships/can only be accessed by organisation admins. ThePOSTreponse includes the backing key'sidandprefixso the dashboard can reuse the existing role assignment flow.How did you test this code?
Added new unit and integration tests.