Remove redundant entity-level certificates#3475
Open
Yathusiga27 wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughRemoves application-level certificate handling, moves certificate data to OAuth app config, and updates inbound-client, DCR, frontend, API, docs, and tests to use the new certificate placement and reference types. ChangesApplication and OAuth certificate migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Bundle ReportChanges will increase total bundle size by 36 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: console-esmAssets Changed:
|
Yathusiga27
force-pushed
the
remove-entity-level-certificates
branch
4 times, most recently
from
f2d9304 to
ef1daf1
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
thiva-k
changed the title
Contributor
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/internal/inboundclient/service.go (1)
219-233: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefer old OAuth certificate cleanup until the entity update succeeds.
Line 221 deletes the certificate for the currently stored client ID before the application/agent callers update their entity system attributes. If that later entity update fails, the entity still points at the old
client_idbut its certificate has been removed, breaking the existing OAuth app. Move this cleanup to a post-entity-update step, add compensation, or make the entity/config/certificate update atomic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/inboundclient/service.go` around lines 219 - 233, The old OAuth certificate is being deleted via the s.deleteCertificate call before the entity update completes, which can leave the entity in an inconsistent state if the update fails. Move the certificate cleanup logic that checks oldOAuthClientID and calls s.deleteCertificate to execute after the entity update succeeds (after the s.syncCertificate call and any subsequent entity update operations complete), ensuring that the old certificate is only removed once the entity has been safely updated with the new OAuth client ID.tests/integration/oauth/dcr/dcr_test.go (1)
563-572: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win🔴 Documentation Required
This PR introduces user-facing changes that are not covered by documentation updates underdocs/.
Please update the relevant documentation before merging.Missing documentation:
- Application/agent API schema updates: document removal of legacy top-level certificate fields and the new certificate location at
inboundAuthConfig[].config.certificateindocs/content/apis.mdx.- DCR/OIDC API behavior updates: document JWKS/JWKS URI handling and
token_endpoint_auth_methodexpectations (private_key_jwtpath) indocs/content/apis.mdxand relevant guides underdocs/content/guides/.- Migration guidance for SDK/integration consumers: add payload/response migration notes for clients relying on the removed top-level certificate fields under
docs/content/sdks/(and/or a dedicated migration guide).As per path instructions, changes that modify public API/auth/user-facing behavior must include a single consolidated docs-gap comment when docs updates are missing.
Also applies to: 598-606, 628-671, 971-985, 1027-1036
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/oauth/dcr/dcr_test.go` around lines 563 - 572, This PR introduces user-facing API changes including removal of legacy top-level certificate fields, new certificate location at inboundAuthConfig[].config.certificate, and JWKS/JWKS URI handling for DCR/OIDC with private_key_jwt token_endpoint_auth_method. Update docs/content/apis.mdx to document the certificate field migration and JWKS/JWKS URI behavior expectations, add relevant guides under docs/content/guides/ explaining the DCR/OIDC API changes, and include migration guidance in docs/content/sdks/ for SDK consumers affected by the removed top-level certificate fields.Source: Path instructions
tests/integration/application/application_api_test.go (1)
874-1041: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift🔴 Documentation Required
This PR introduces user-facing changes that are not covered by documentation updates underdocs/.
Please update the relevant documentation before merging.Missing documentation:
- Application/Agent API schema migration: document removal of legacy top-level certificate fields and the new certificate location under
inboundAuthConfig[].oauthAppConfig.certificateindocs/content/apis.mdx.- Auth flow behavior: document certificate lifecycle now being OAuth-app scoped (including
private_key_jwtcertificate expectations) indocs/content/guides/migration/usage guidance.- Client/SDK contract impact: document model changes for certificate fields in
docs/content/sdks/so generated/manual SDK consumers update request/response handling.As per path instructions, "If ANY of the above are detected and the PR does NOT include corresponding updates under
docs/, post a single consolidated PR-level comment."Also applies to: 1350-1402, 2202-2228
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/application/application_api_test.go` around lines 874 - 1041, Add documentation updates to cover the API schema changes introduced by this PR. Create or update docs/content/apis.mdx to document the removal of legacy top-level certificate fields from the Application model and the new certificate location under inboundAuthConfig[].oauthAppConfig.certificate. Add migration and usage guidance to docs/content/guides/ explaining that certificate lifecycle is now OAuth-app scoped and covering private_key_jwt certificate expectations (including supported certificate types like JWKS_URI and JWKS). Finally, update docs/content/sdks/ to document the certificate field model changes so SDK consumers understand how to update their request and response handling for the new certificate structure shown in the TestApplicationCreationWithPrivateKeyJWT test cases.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/internal/cert/service_test.go`:
- Around line 794-795: The test table in TestIsValidReferenceType has a
duplicate reference type for the "Application type" test case, as both the first
and second rows currently use CertificateReferenceTypeIDP. Replace the
CertificateReferenceTypeIDP in the "Application type" test case (the first row)
with the appropriate reference type constant for OAuth applications (likely
CertificateReferenceTypeOAuthApp or similar) to restore coverage of the second
supported reference type.
---
Outside diff comments:
In `@backend/internal/inboundclient/service.go`:
- Around line 219-233: The old OAuth certificate is being deleted via the
s.deleteCertificate call before the entity update completes, which can leave the
entity in an inconsistent state if the update fails. Move the certificate
cleanup logic that checks oldOAuthClientID and calls s.deleteCertificate to
execute after the entity update succeeds (after the s.syncCertificate call and
any subsequent entity update operations complete), ensuring that the old
certificate is only removed once the entity has been safely updated with the new
OAuth client ID.
In `@tests/integration/application/application_api_test.go`:
- Around line 874-1041: Add documentation updates to cover the API schema
changes introduced by this PR. Create or update docs/content/apis.mdx to
document the removal of legacy top-level certificate fields from the Application
model and the new certificate location under
inboundAuthConfig[].oauthAppConfig.certificate. Add migration and usage guidance
to docs/content/guides/ explaining that certificate lifecycle is now OAuth-app
scoped and covering private_key_jwt certificate expectations (including
supported certificate types like JWKS_URI and JWKS). Finally, update
docs/content/sdks/ to document the certificate field model changes so SDK
consumers understand how to update their request and response handling for the
new certificate structure shown in the TestApplicationCreationWithPrivateKeyJWT
test cases.
In `@tests/integration/oauth/dcr/dcr_test.go`:
- Around line 563-572: This PR introduces user-facing API changes including
removal of legacy top-level certificate fields, new certificate location at
inboundAuthConfig[].config.certificate, and JWKS/JWKS URI handling for DCR/OIDC
with private_key_jwt token_endpoint_auth_method. Update docs/content/apis.mdx to
document the certificate field migration and JWKS/JWKS URI behavior
expectations, add relevant guides under docs/content/guides/ explaining the
DCR/OIDC API changes, and include migration guidance in docs/content/sdks/ for
SDK consumers affected by the removed top-level certificate fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 340c0fa0-a0d6-422c-a122-3bbdc1543e79
⛔ Files ignored due to path filters (1)
backend/tests/mocks/inboundclientmock/InboundClientServiceInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (28)
backend/internal/agent/service.gobackend/internal/agent/service_test.gobackend/internal/application/declarative_resource.gobackend/internal/application/handler.gobackend/internal/application/handler_test.gobackend/internal/application/init_test.gobackend/internal/application/model/application.gobackend/internal/application/service.gobackend/internal/application/service_test.gobackend/internal/cert/cache_backed_store_test.gobackend/internal/cert/constants.gobackend/internal/cert/service.gobackend/internal/cert/service_test.gobackend/internal/cert/store_test.gobackend/internal/inboundclient/InboundClientServiceInterface_mock_test.gobackend/internal/inboundclient/model/inbound_client.gobackend/internal/inboundclient/service.gobackend/internal/inboundclient/service_test.gobackend/internal/oauth/oauth2/dcr/service.gobackend/internal/oauth/oauth2/dcr/service_test.gobackend/internal/oauth/oauth2/userinfo/service_jwe_test.gobackend/internal/system/importer/service.gofrontend/apps/console/src/features/agents/models/agent.tsfrontend/apps/console/src/features/agents/pages/AgentEditPage.tsxfrontend/apps/console/src/features/applications/models/__tests__/application.test.tsfrontend/apps/console/src/features/applications/pages/ApplicationEditPage.tsxtests/integration/application/application_api_test.gotests/integration/oauth/dcr/dcr_test.go
💤 Files with no reviewable changes (9)
- backend/internal/inboundclient/model/inbound_client.go
- backend/internal/application/declarative_resource.go
- frontend/apps/console/src/features/applications/models/tests/application.test.ts
- backend/internal/application/model/application.go
- backend/internal/system/importer/service.go
- backend/internal/application/handler.go
- backend/internal/application/init_test.go
- backend/internal/cert/constants.go
- frontend/apps/console/src/features/agents/models/agent.ts
Contributor
|
Let's check the OpenAPI specs and other docs too for any references and remove them |
Yathusiga27
force-pushed
the
remove-entity-level-certificates
branch
from
ef1daf1 to
d767c77
Compare
Contributor
Author
Done, removed stale top-level application/agent certificate references from OpenAPI/docs and kept only the valid OAuth client certificate paths. |
Yathusiga27
requested review from
DonOmalVindula,
ThaminduDilshan,
hwupathum and
thiva-k
as code owners
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/internal/inboundclient/service_test.go (1)
394-452: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the OAuth-app reference in
syncCertificatetests.These expectations use
mock.Anythingfor the reference type/ref ID, so the tests would still pass ifsyncCertificateregressed to a non-OAuth-app reference. Since this PR hardcodes OAuth-app certificate handling, make those expectations explicit.Example tightening
mockCert.EXPECT(). - GetCertificateByReference(mock.Anything, mock.Anything, mock.Anything). + GetCertificateByReference(mock.Anything, cert.CertificateReferenceTypeOAuthApp, "ref-1"). Return(nil, &cert.ErrorCertificateNotFound)mockCert.EXPECT(). - DeleteCertificateByReference(mock.Anything, mock.Anything, mock.Anything). + DeleteCertificateByReference(mock.Anything, cert.CertificateReferenceTypeOAuthApp, "ref-1"). Return(nil)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/inboundclient/service_test.go` around lines 394 - 452, The syncCertificate tests are too loose because the GetCertificateByReference, CreateCertificate, UpdateCertificateByID, and DeleteCertificateByReference expectations use mock.Anything for the reference type and ref ID, so a regression away from the OAuth-app reference would still pass. Tighten the assertions in the InboundClientServiceTestSuite syncCertificate cases by explicitly matching the OAuth-app reference values used by syncCertificate and the new hardcoded OAuth-app certificate handling, so the tests verify the correct reference is passed through in each path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/internal/inboundclient/service.go`:
- Around line 153-156: Reject OAuth certificate input when oauthClientID is
missing: in service.go, the oauthProfile.Certificate handling inside the
certificate creation flow currently skips persistence when oauthClientID == "",
which can leave an invalid OAuth profile state. Update the relevant validation
path in the certificate persistence logic around createCertificate so that if
oauthProfile.Certificate is present but oauthClientID is empty, the method
returns a validation error instead of continuing; apply the same guard in both
affected certificate-handling blocks referenced by oauthProfile and
createCertificate.
---
Nitpick comments:
In `@backend/internal/inboundclient/service_test.go`:
- Around line 394-452: The syncCertificate tests are too loose because the
GetCertificateByReference, CreateCertificate, UpdateCertificateByID, and
DeleteCertificateByReference expectations use mock.Anything for the reference
type and ref ID, so a regression away from the OAuth-app reference would still
pass. Tighten the assertions in the InboundClientServiceTestSuite
syncCertificate cases by explicitly matching the OAuth-app reference values used
by syncCertificate and the new hardcoded OAuth-app certificate handling, so the
tests verify the correct reference is passed through in each path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e0cab08-de05-42e1-bf1b-672acc0f8cd4
⛔ Files ignored due to path filters (1)
backend/tests/mocks/inboundclientmock/InboundClientServiceInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (33)
api/agent.yamlapi/application.yamlbackend/internal/agent/service.gobackend/internal/agent/service_test.gobackend/internal/application/declarative_resource.gobackend/internal/application/handler.gobackend/internal/application/handler_test.gobackend/internal/application/init_test.gobackend/internal/application/model/application.gobackend/internal/application/service.gobackend/internal/application/service_test.gobackend/internal/cert/cache_backed_store_test.gobackend/internal/cert/constants.gobackend/internal/cert/service.gobackend/internal/cert/service_test.gobackend/internal/cert/store_test.gobackend/internal/inboundclient/InboundClientServiceInterface_mock_test.gobackend/internal/inboundclient/model/inbound_client.gobackend/internal/inboundclient/service.gobackend/internal/inboundclient/service_test.gobackend/internal/oauth/oauth2/dcr/service.gobackend/internal/oauth/oauth2/dcr/service_test.gobackend/internal/oauth/oauth2/userinfo/service_jwe_test.gobackend/internal/system/importer/service.godocs/content/guides/guides/applications.mdxdocs/content/guides/guides/applications/application-settings.mdxdocs/content/guides/guides/protocols/oauth-oidc/client-authentication-methods.mdxdocs/content/guides/guides/protocols/oauth-oidc/token-formats.mdxdocs/content/guides/guides/protocols/oauth-oidc/userinfo.mdxfrontend/apps/console/src/features/agents/models/agent.tsfrontend/apps/console/src/features/agents/pages/AgentEditPage.tsxfrontend/apps/console/src/features/applications/models/__tests__/application.test.tsfrontend/apps/console/src/features/applications/pages/ApplicationEditPage.tsx
💤 Files with no reviewable changes (18)
- backend/internal/cert/constants.go
- docs/content/guides/guides/protocols/oauth-oidc/userinfo.mdx
- backend/internal/application/model/application.go
- frontend/apps/console/src/features/applications/pages/ApplicationEditPage.tsx
- backend/internal/system/importer/service.go
- frontend/apps/console/src/features/agents/models/agent.ts
- backend/internal/inboundclient/model/inbound_client.go
- docs/content/guides/guides/applications/application-settings.mdx
- frontend/apps/console/src/features/agents/pages/AgentEditPage.tsx
- frontend/apps/console/src/features/applications/models/tests/application.test.ts
- docs/content/guides/guides/protocols/oauth-oidc/token-formats.mdx
- docs/content/guides/guides/protocols/oauth-oidc/client-authentication-methods.mdx
- backend/internal/application/declarative_resource.go
- backend/internal/application/handler.go
- docs/content/guides/guides/applications.mdx
- backend/internal/application/init_test.go
- api/application.yaml
- api/agent.yaml
✅ Files skipped from review due to trivial changes (1)
- backend/internal/inboundclient/InboundClientServiceInterface_mock_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- backend/internal/cert/service.go
- backend/internal/application/handler_test.go
- backend/internal/oauth/oauth2/dcr/service_test.go
- backend/internal/oauth/oauth2/userinfo/service_jwe_test.go
- backend/internal/cert/store_test.go
- backend/internal/oauth/oauth2/dcr/service.go
- backend/internal/agent/service_test.go
- backend/internal/application/service_test.go
- backend/internal/application/service.go
- backend/internal/agent/service.go
- backend/internal/cert/cache_backed_store_test.go
- backend/internal/cert/service_test.go
Yathusiga27
force-pushed
the
remove-entity-level-certificates
branch
2 times, most recently
from
c9261b2 to
b5ebb79
Compare
Yathusiga27
force-pushed
the
remove-entity-level-certificates
branch
from
b5ebb79 to
7062756
Compare
Thumimku
reviewed
Jun 25, 2026
Yathusiga27
force-pushed
the
remove-entity-level-certificates
branch
from
7062756 to
616ac43
Compare
Contributor
|
Let's add breaking change section to the PR description |
thiva-k
added
the
breaking change
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Remove the unused entity-level certificate path for applications and agents.
ThunderID currently has two certificate references: OAuth-app certificates keyed by OAuth client ID, and entity-level certificates keyed by application/agent entity ID. The entity-level certificate is only stored and returned in top-level API fields, but runtime OAuth client authentication, token encryption, userinfo encryption, and JWT validation use the OAuth-app certificate path only.
This PR removes the dead top-level
application.certificate/agent.certificatepath so certificates are managed only throughinboundAuthConfig[].config.certificate.🔧 Summary of Breaking Changes
Removed the deprecated entity-level certificate field from applications and agents. The top-level
application.certificate/agent.certificatefield is no longer accepted in create/update requests or returned in GET responses.💥 Impact
Clients that send or read the old top-level
certificatefield must update their payload handling. OAuth runtime certificate behavior is unchanged because the supported certificate path remainsinboundAuthConfig[].config.certificate.🔄 Migration Guide
Move certificate configuration to the OAuth inbound auth config:
Approach
CertificateReferenceTypeApplication.inboundAuthConfig[].config.certificate.Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
ErrorInvalidOAuthConfigurationwhen an OAuth client certificate is missing a resolvable OAuth client ID.JWKSUrivalidation (must be a validhttpsURL).certificatesupport from API contracts and console models; certificate handling is now driven by OAuth client configuration only.AllowedUserTypes) rather than certificate presence.