Repository navigation
feat(dynamic-registration): add full oid compliance - #11
tugascript wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It is an explicitly WIP, security-sensitive change spanning schema, OAuth/OIDC registration logic, and an unmitigated SSRF vector in the new sector_identifier_uri fetch, so it needs human review.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
This WIP PR moves the IdP toward fuller OpenID Connect Dynamic Client Registration compliance. It removes the custom transport concept and collapses the previous seven application types (web/native/spa/backend/device/service/mcp) down to web and native, folding SPA/service/backend behavior into web (public vs. confidential, distinguished by token_endpoint_auth_method and grant_types). It also adds OIDC sector_identifier_uri handling, the implicit grant type, and error_description on OAuth error responses. The corresponding tables (app_related_apps, app_service_configs) and their service/test code are removed.
Changes:
- Remove
transportand the device/service/backend/spa/mcp app types; route public/service clients through thewebtype viagrant_types/token_endpoint_auth_method. - Add OIDC
sector_identifier_urifetching/validation and theimplicitgrant type. - Add
error_descriptionto OAuth error responses and update registration validation, DTOs, schema, and tests accordingly.
| File | Description |
|---|---|
| project.md | Updates roadmap to reflect the simplified app-type model. |
| idp/internal/controllers/oauth_dynamic_registration_account.go | Adds error_description passthrough; introduces the isAuthenticaed misspelled variable. |
| idp/internal/services/registration_metadata.go | New sector_identifier_uri fetch/validation and reworked redirect/URI validation. |
| idp/internal/controllers/bodies/oauth_dynamic_registration.go | Adds implicit to accepted grant types. |
| idp/internal/services/app_dynamic_registration.go | New grant-type/auth-method validation, AuthMethodNone handling, tx threading. |
| idp/internal/controllers/helpers.go | Makes oauthErrorResponse variadic for descriptions; generic server-error logging. |
| idp/internal/exceptions/controllers.go | Adds ErrorDescription field and NewOAuthErrorWithDescription. |
| idp/tests/apps_test.go | Drops spa/backend/device/service/mcp cases; adds public-web and web-service cases. |
| idp/tests/dynamic_registration_test.go | Threads DekKid into credential key creation. |
| idp/tests/oauth_test.go, idp/tests/account_credentials_test.go | Remove transport fields from fixtures. |
| idp/tests/common_test.go | Raises test request timeout from 2s to 60s. |
Files not reviewed (4)
- idp/internal/providers/database/account_credentials.sql.go: Generated file
- idp/internal/providers/database/apps.sql.go: Generated file
- idp/internal/providers/database/models.go: Generated file
- idp/internal/providers/database/registered_apps.sql.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: tugascript <64930104+tugascript@users.noreply.github.com>
tugascript
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdd4f61aed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🔵 Needs a closer look
It rewrites the initial database schema/migration and security-sensitive dynamic-registration logic while removing several app types, a breadth of high-risk change that warrants final human review.
0 open findings
2 resolved since last review
Files not reviewed (4)
- idp/internal/providers/database/account_credentials.sql.go: Generated file
- idp/internal/providers/database/apps.sql.go: Generated file
- idp/internal/providers/database/models.go: Generated file
- idp/internal/providers/database/registered_apps.sql.go: Generated file
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8255784c55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return exceptions.NewError(exceptions.OAuthErrorInvalidClientMetadata, "sector_identifier_uri host must resolve to a public address") | ||
| } | ||
|
|
||
| sectorRedirects, err := fetchSectorIdentifierURIs(ctx, sectorIdentifierURI) |
There was a problem hiding this comment.
When app registration does not require an initial access token, an unauthenticated registrant can provide a public sector_identifier_uri whose HTTPS response stalls. CreateAppCredentialsRegistration enters registrationTransaction before metadata preparation reaches this fetch, so every request holds a database connection and transaction for up to the client's 10-second timeout; concurrent requests can exhaust the pool and block unrelated database work. Fetch and validate remote metadata before opening the transaction, or otherwise avoid holding a connection during this network request.
Useful? React with 👍 / 👎.
| TokenEndpointAuthMethod string `json:"token_endpoint_auth_method,omitempty" validate:"omitempty,oneof=none client_secret_basic client_secret_post client_secret_jwt private_key_jwt"` | ||
| ResponseTypes []string `json:"response_types,omitempty" validate:"omitempty,dive,oneof=code 'code id_token'"` | ||
| GrantTypes []string `json:"grant_types,omitempty" validate:"omitempty,min=1,dive,oneof=authorization_code refresh_token client_credentials urn:ietf:params:oauth:grant-type:jwt-bearer"` | ||
| ResponseTypes []string `json:"response_types,omitempty" validate:"omitempty,dive,oneof=code id_token 'code id_token'"` |
There was a problem hiding this comment.
An implicit-only registration with grant_types: ["implicit"] and response_types: ["id_token"] now passes request binding and metadata normalization, but still fails when mapped for persistence. Fresh evidence in the current tree is that mapResponseTypesWithDefault and the database response_type enum still support only code and code id_token, so the mapper returns invalid response type: id_token; add the standalone response type throughout the mapper/model/schema path.
Useful? React with 👍 / 👎.
| if firstHost == "" { | ||
| firstHost = parsed.Host | ||
| } else if !strings.EqualFold(firstHost, parsed.Host) { |
There was a problem hiding this comment.
For a pairwise client without a sector URI, redirects such as https://client.example/callback and https://client.example:8443/callback have the same host component but are rejected because url.URL.Host includes the port. This unnecessarily forces otherwise valid clients with multiple ports to publish a sector document; compare parsed.Hostname() values instead.
Useful? React with 👍 / 👎.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Has a bunch of custom application types
What is the new behavior?
Removes the application type from