feat: Add OpenAPI documentation on controller OAuth2ProviderController web routes#113
Open
matiasperrone-exo wants to merge 4 commits intomainfrom
Open
feat: Add OpenAPI documentation on controller OAuth2ProviderController web routes#113matiasperrone-exo wants to merge 4 commits intomainfrom
matiasperrone-exo wants to merge 4 commits intomainfrom
Conversation
matiasperrone-exo
force-pushed
the
feat/openapi----web---oauth2providercontroller
branch
3 times, most recently
from
5bf7fad to
dfb8863
Compare
matiasperrone-exo
added
the
documentation
martinquiroga-exo
approved these changes
Feb 18, 2026
martinquiroga-exo
left a comment
martinquiroga-exo
left a comment
There was a problem hiding this comment.
LGTM
@matiasperrone-exo please add clickup card link to this PR
matiasperrone-exo
force-pushed
the
feat/openapi----web---oauth2providercontroller
branch
from
dfb8863 to
1e72293
Compare
|
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 PR adds comprehensive OpenAPI/Swagger documentation to OAuth2/OIDC provider endpoints through PHP attribute annotations and new schema definition classes. No functional logic is modified; changes are purely documentation and API specification related. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 |
Contributor
Author
|
Rebased |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-113/ This page is automatically updated on each push to this PR. |
martinquiroga-exo
suggested changes
Apr 23, 2026
martinquiroga-exo
left a comment
martinquiroga-exo
left a comment
There was a problem hiding this comment.
@matiasperrone-exo please see comments and please sign your commits or PR will be rejected.
Also, as a final note, PUT, PATCH, and DELETE are documented for /oauth2/auth but are not part of OAuth2/OIDC, so I'm not sure if they are needed but I defer to you.
matiasperrone-exo
force-pushed
the
feat/openapi----web---oauth2providercontroller
branch
from
1e72293 to
db9f777
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-113/ This page is automatically updated on each push to this PR. |
1 similar comment
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-113/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Swagger/OAuth2ProviderControllerSchemas.php`:
- Around line 7-172: The response schemas are missing RFC-required "required"
arrays; update the OA\Schema annotations for OAuth2TokenResponseSchema to
include required: ['access_token','token_type'], for
OAuth2IntrospectionResponseSchema add required: ['active'], for
JWKSResponseSchema add required: ['keys'], and for OpenIDDiscoveryResponseSchema
add required:
['issuer','authorization_endpoint','token_endpoint','jwks_uri','response_types_supported','subject_types_supported','id_token_signing_alg_values_supported']
by adding the required key to each corresponding #[OA\Schema(...)] attribute so
the generated docs include the mandated required fields.
In `@app/Swagger/Security/OAuth2ProviderControllerSecuritySchema.php`:
- Line 14: The Swagger OAuth2 security schema is using
L5_SWAGGER_CONST_TOKEN_URL which defaults to http://localhost/oauth/token but
your actual endpoint is /oauth2/token; update the constant value or document the
required env override so the tokenUrl points to /oauth2/token (or the correct
full URL) — locate the usage in OAuth2ProviderControllerSecuritySchema.php
referencing L5_SWAGGER_CONST_TOKEN_URL and change the configuration/ENV (or the
default in the l5-swagger config) so the swagger Authorize flow targets
/oauth2/token.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 601a3165-be20-415f-abe8-e4fa29f5c8c5
📒 Files selected for processing (8)
app/Http/Controllers/OAuth2/OAuth2ProviderController.phpapp/Swagger/OAuth2ProviderControllerSchemas.phpapp/Swagger/Requests/OAuth2AuthorizationRequestSchema.phpapp/Swagger/Requests/OAuth2EndSessionRequestSchema.phpapp/Swagger/Requests/OAuth2TokenIntrospectionRequestSchema.phpapp/Swagger/Requests/OAuth2TokenRequestSchema.phpapp/Swagger/Requests/OAuth2TokenRevocationRequestSchema.phpapp/Swagger/Security/OAuth2ProviderControllerSecuritySchema.php
Contributor
Author
|
@martinquiroga-exo please review again. All PR's requested changes and additional AI comments were added. |
matiasperrone-exo
force-pushed
the
feat/openapi----web---oauth2providercontroller
branch
from
59f315f to
03c7efc
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-113/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Http/Controllers/OAuth2/OAuth2ProviderController.php`:
- Around line 431-440: The operationId value inside the OA\Get for the
'/oauth2/.well-known/openid-configuration' endpoint is garbled
("OAclient_secretuth2OpenIdDiscovery"); replace it with a proper camelCase
identifier such as "oauth2OpenIdDiscovery" in that OA\Get annotation, and also
change the sibling operationId "OpenIdDiscovery" (earlier in the file) to
"openIdDiscovery" to maintain camelCase consistency with other operationIds like
oauth2Token and oauth2RevokeToken.
- Around line 301-320: The OpenAPI security annotation is incorrect for the
token revoke/introspection/end-session endpoints: OAuth2ProviderSecurity (oauth2
clientCredentials) implies a Bearer token but the implementation
(ClientService::getCurrentClientAuthInfo) accepts HTTP Basic or
client_id/client_secret form params and end-session uses
id_token_hint/client_id; fix by adding a new HTTP Basic security scheme named
OAuth2ProviderClientBasic (type=http, scheme=basic, description per RFC 6749
§2.3.1) and update the annotations on the OAuth2ProviderController operations
for /oauth2/token/revoke and /oauth2/token/introspection to allow either the
OAuth2ProviderClientBasic or existing OAuth2ProviderSecurity (i.e., list both in
the security array), and remove the security annotation entirely from the
/oauth2/end-session operation so that authentication remains documented via
id_token_hint/client_id form/query params.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c11200a1-3918-4c70-8ae4-5eb3bc1ca968
📒 Files selected for processing (4)
app/Http/Controllers/OAuth2/OAuth2ProviderController.phpapp/Swagger/OAuth2ProviderControllerSchemas.phpapp/Swagger/Requests/OAuth2AuthorizationRequestSchema.phpapp/Swagger/Requests/OAuth2TokenIntrospectionRequestSchema.php
✅ Files skipped from review due to trivial changes (1)
- app/Swagger/Requests/OAuth2AuthorizationRequestSchema.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Swagger/OAuth2ProviderControllerSchemas.php
Contributor
fixed |
matiasperrone-exo
force-pushed
the
feat/openapi----web---oauth2providercontroller
branch
from
03c7efc to
4ce765c
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-113/ This page is automatically updated on each push to this PR. |
Contributor
Author
|
@martinquiroga-exo please review again |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/Http/Controllers/OAuth2/OAuth2ProviderController.php (1)
426-426:⚠️ Potential issue | 🟡 MinorStale "Also available at …" sentence still present in description.
A previous review asked to remove
Also available at /oauth2/.well-known/openid-configuration.from this description to avoid inconsistency, and the reply was "done" — but the sentence is still here.✏️ Proposed fix
- description: 'Returns the OpenID Provider Configuration document per OpenID Connect Discovery 1.0. Also available at /oauth2/.well-known/openid-configuration.', + description: 'Returns the OpenID Provider Configuration document per OpenID Connect Discovery 1.0.',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/OAuth2/OAuth2ProviderController.php` at line 426, The description string inside OAuth2ProviderController (the OpenID Provider Configuration response) still contains the stale sentence "Also available at /oauth2/.well-known/openid-configuration."; locate the description assignment or array entry (the string shown in the diff) in OAuth2ProviderController.php and remove that trailing sentence so the description reads only "Returns the OpenID Provider Configuration document per OpenID Connect Discovery 1.0." (ensure punctuation remains correct).
🧹 Nitpick comments (1)
app/Http/Controllers/OAuth2/OAuth2ProviderController.php (1)
258-258: Consider allowingOAuth2ProviderClientBasicon/oauth2/tokenfor consistency.The sibling
/oauth2/token/revoke(line 308) and/oauth2/token/introspection(line 358) endpoints list bothOAuth2ProviderClientBasicandOAuth2ProviderSecurity, but/oauth2/tokenlists onlyOAuth2ProviderSecurity. The token endpoint accepts HTTP Basic client authentication per RFC 6749 §2.3.1 as well, so Swagger UI should surface that option here too.🔧 Suggested alignment
- security: [['OAuth2ProviderSecurity' => []]], + security: [['OAuth2ProviderClientBasic' => []], ['OAuth2ProviderSecurity' => []]],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/OAuth2/OAuth2ProviderController.php` at line 258, The OpenAPI security entry for the /oauth2/token operation currently lists only 'OAuth2ProviderSecurity'; update the operation's security array to also include 'OAuth2ProviderClientBasic' so the token endpoint surfaces HTTP Basic client auth like the sibling endpoints (/oauth2/token/revoke and /oauth2/token/introspection). Locate the security declaration for the /oauth2/token operation in OAuth2ProviderController (the entry with security: [['OAuth2ProviderSecurity' => []]]) and add an additional element for 'OAuth2ProviderClientBasic' to the array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/Http/Controllers/OAuth2/OAuth2ProviderController.php`:
- Line 426: The description string inside OAuth2ProviderController (the OpenID
Provider Configuration response) still contains the stale sentence "Also
available at /oauth2/.well-known/openid-configuration."; locate the description
assignment or array entry (the string shown in the diff) in
OAuth2ProviderController.php and remove that trailing sentence so the
description reads only "Returns the OpenID Provider Configuration document per
OpenID Connect Discovery 1.0." (ensure punctuation remains correct).
---
Nitpick comments:
In `@app/Http/Controllers/OAuth2/OAuth2ProviderController.php`:
- Line 258: The OpenAPI security entry for the /oauth2/token operation currently
lists only 'OAuth2ProviderSecurity'; update the operation's security array to
also include 'OAuth2ProviderClientBasic' so the token endpoint surfaces HTTP
Basic client auth like the sibling endpoints (/oauth2/token/revoke and
/oauth2/token/introspection). Locate the security declaration for the
/oauth2/token operation in OAuth2ProviderController (the entry with security:
[['OAuth2ProviderSecurity' => []]]) and add an additional element for
'OAuth2ProviderClientBasic' to the array.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9443d97-061c-43d3-8ad1-f82dd57c2e97
📒 Files selected for processing (2)
app/Http/Controllers/OAuth2/OAuth2ProviderController.phpapp/Swagger/Security/OAuth2ProviderControllerSecuritySchema.php
matiasperrone-exo
force-pushed
the
feat/openapi----web---oauth2providercontroller
branch
from
4ce765c to
ab298f3
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-113/ This page is automatically updated on each push to this PR. |
matiasperrone-exo
force-pushed
the
feat/openapi----web---oauth2providercontroller
branch
from
ab298f3 to
d0bc232
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-113/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/TwoFactorRepositoriesTest.php (1)
69-76: Add a revoked-device negative case.This only proves the happy path. It never asserts that
getActiveByUserAndIdentifier()/getActiveByUser()exclude revoked devices, so a regression in theis_revokedfilter would still pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/TwoFactorRepositoriesTest.php` around lines 69 - 76, Add a negative test asserting revoked devices are excluded: after creating the active device used in the existing assertions, create or mark a second device as revoked for $this->user (ensure isRevoked() returns true for that entity), then call $repo->getActiveByUserAndIdentifier($this->user, $revokedDeviceId) and assert it returns null/false, and call $repo->getActiveByUser($this->user) and assert the revoked device is not present in the returned collection; reference the repository methods getActiveByUserAndIdentifier and getActiveByUser and the entity method isRevoked to locate where to add these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/libs/Auth/Models/User.php`:
- Around line 2431-2434: The setter setTwoFactorEnabled currently ORs the passed
$enabled with shouldRequire2FA(), making the persisted two_factor_enabled sticky
and preventing disable; change setTwoFactorEnabled(bool $enabled) to only
persist the user's intent (assign $this->two_factor_enabled = $enabled) and keep
shouldRequire2FA() as the separate policy check used when deciding whether to
enforce 2FA at runtime (e.g., in isTwoFactorRequired or authentication flows) so
policy and stored state remain distinct.
In `@database/migrations/Version20260416194357.php`:
- Around line 33-39: Update the migration so it checks each 2FA column
individually before altering the users table: replace the single guard that uses
$builder->hasColumn("users", "two_factor_enabled") with separate checks for
"two_factor_enabled", "two_factor_method", and "two_factor_enforced_at" (use
$builder->hasColumn("users", "<column>") for each) in the up() path before
calling $builder->table(...), and mirror that logic in down() so each column is
only dropped if $builder->hasColumn("users", "<column>") returns false/true as
appropriate; keep using the existing $schema->hasTable("users"),
$builder->table(...) and the Table->boolean, Table->string, Table->dateTime
calls but gate each column operation individually.
---
Nitpick comments:
In `@tests/TwoFactorRepositoriesTest.php`:
- Around line 69-76: Add a negative test asserting revoked devices are excluded:
after creating the active device used in the existing assertions, create or mark
a second device as revoked for $this->user (ensure isRevoked() returns true for
that entity), then call $repo->getActiveByUserAndIdentifier($this->user,
$revokedDeviceId) and assert it returns null/false, and call
$repo->getActiveByUser($this->user) and assert the revoked device is not present
in the returned collection; reference the repository methods
getActiveByUserAndIdentifier and getActiveByUser and the entity method isRevoked
to locate where to add these assertions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9671ba06-50da-4fee-a4bd-c759623e9384
📒 Files selected for processing (24)
app/Http/Controllers/OAuth2/OAuth2ProviderController.phpapp/Repositories/DoctrineTwoFactorAuditLogRepository.phpapp/Repositories/DoctrineUserRecoveryCodeRepository.phpapp/Repositories/DoctrineUserTrustedDeviceRepository.phpapp/Repositories/RepositoriesProvider.phpapp/Swagger/OAuth2ProviderControllerSchemas.phpapp/Swagger/Requests/OAuth2AuthorizationRequestSchema.phpapp/Swagger/Requests/OAuth2EndSessionRequestSchema.phpapp/Swagger/Requests/OAuth2TokenIntrospectionRequestSchema.phpapp/Swagger/Requests/OAuth2TokenRequestSchema.phpapp/Swagger/Requests/OAuth2TokenRevocationRequestSchema.phpapp/Swagger/Security/OAuth2ProviderControllerSecuritySchema.phpapp/libs/Auth/Models/TwoFactorAuditLog.phpapp/libs/Auth/Models/User.phpapp/libs/Auth/Models/UserRecoveryCode.phpapp/libs/Auth/Models/UserTrustedDevice.phpapp/libs/Auth/Repositories/ITwoFactorAuditLogRepository.phpapp/libs/Auth/Repositories/IUserRecoveryCodeRepository.phpapp/libs/Auth/Repositories/IUserTrustedDeviceRepository.phpconfig/two_factor.phpdatabase/migrations/Version20260416194357.phpphpunit.xmltests/TwoFactorRepositoriesTest.phptests/unit/UserTwoFactorTest.php
✅ Files skipped from review due to trivial changes (8)
- phpunit.xml
- app/Swagger/Requests/OAuth2TokenIntrospectionRequestSchema.php
- config/two_factor.php
- app/Swagger/Requests/OAuth2TokenRevocationRequestSchema.php
- app/Swagger/Requests/OAuth2TokenRequestSchema.php
- app/Swagger/Security/OAuth2ProviderControllerSecuritySchema.php
- app/Http/Controllers/OAuth2/OAuth2ProviderController.php
- app/Swagger/OAuth2ProviderControllerSchemas.php
🚧 Files skipped from review as they are similar to previous changes (2)
- app/Swagger/Requests/OAuth2EndSessionRequestSchema.php
- app/Swagger/Requests/OAuth2AuthorizationRequestSchema.php
caseylocker
reviewed
Apr 29, 2026
caseylocker
requested changes
Apr 29, 2026
Contributor
caseylocker
left a comment
caseylocker
left a comment
There was a problem hiding this comment.
@matiasperrone-exo Please see unresolved and new comments.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Http/Controllers/OAuth2/OAuth2ProviderController.php`:
- Around line 93-98: The OA\Parameter calls incorrectly include an enum argument
(OA\Parameter(..., enum: [...])), which is unsupported; update the OA\Parameter
invocations for approval_prompt, access_type, response_mode,
code_challenge_method, and display inside the OAuth2ProviderController class to
remove the enum: [...] argument and keep the enum only on the nested
OA\Schema(...) (e.g., new OA\Parameter(..., schema: new OA\Schema(type:
'string', enum: [...]))); ensure no OA\Parameter retains an enum property and
that each corresponding OA\Schema retains the correct enum values.
In `@app/Swagger/Requests/OAuth2AuthorizationRequestSchema.php`:
- Line 20: The OA\Property for 'access_type' in
OAuth2AuthorizationRequestSchema.php allows any string but must match the
controller's constraint (online|offline); update the OA\Property for property:
'access_type' in the OAuth2AuthorizationRequestSchema class to include an enum
with values ['online','offline'] (and keep the default 'online' description) so
the OpenAPI GET/POST schema matches the OAuth2ProviderController's allowed
values.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c247a83-700e-4e27-96cb-a3cc05bf1bc2
📒 Files selected for processing (8)
app/Http/Controllers/OAuth2/OAuth2ProviderController.phpapp/Swagger/OAuth2ProviderControllerSchemas.phpapp/Swagger/Requests/OAuth2AuthorizationRequestSchema.phpapp/Swagger/Requests/OAuth2EndSessionRequestSchema.phpapp/Swagger/Requests/OAuth2TokenIntrospectionRequestSchema.phpapp/Swagger/Requests/OAuth2TokenRequestSchema.phpapp/Swagger/Requests/OAuth2TokenRevocationRequestSchema.phpapp/Swagger/Security/OAuth2ProviderControllerSecuritySchema.php
✅ Files skipped from review due to trivial changes (4)
- app/Swagger/Requests/OAuth2TokenIntrospectionRequestSchema.php
- app/Swagger/Requests/OAuth2EndSessionRequestSchema.php
- app/Swagger/Security/OAuth2ProviderControllerSecuritySchema.php
- app/Swagger/Requests/OAuth2TokenRevocationRequestSchema.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Swagger/OAuth2ProviderControllerSchemas.php
caseylocker
requested changes
Apr 30, 2026
Contributor
caseylocker
left a comment
caseylocker
left a comment
There was a problem hiding this comment.
@matiasperrone-exo please see comments. Also make sure any commits are signed.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-113/ This page is automatically updated on each push to this PR. |
Contributor
Author
|
@caseylocker all your comments were addressed. Thanks for them. |
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
4 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.
Task:
Ref: https://app.clickup.com/t/86b7fjj9a
Summary by CodeRabbit
Release Notes