Feat | API OAuth2UserApiController routes v1#106
Feat | API OAuth2UserApiController routes v1#106matiasperrone-exo wants to merge 2 commits intomainfrom
Conversation
matiasperrone-exo
added
the
documentation
matiasperrone-exo
force-pushed
the
feat/openapi----api-v1---oauth2userapicontroller
branch
from
e5e28a9 to
54025c8
Compare
smarcet
force-pushed
the
main
branch
2 times, most recently
from
ae79f5e to
4b5b726
Compare
matiasperrone-exo
force-pushed
the
feat/openapi----api-v1---oauth2userapicontroller
branch
9 times, most recently
from
ebfe31c to
28e31ae
Compare
matiasperrone-exo
force-pushed
the
feat/openapi----api-v1---oauth2userapicontroller
branch
from
28e31ae to
2c37852
Compare
martinquiroga-exo
left a comment
martinquiroga-exo
left a comment
There was a problem hiding this comment.
LGTM
@matiasperrone-exo please add the clickup card link to this PR please
📝 WalkthroughWalkthroughThis pull request introduces comprehensive OpenAPI documentation and exposes new REST API endpoints in OAuth2UserApiController. Eight new Swagger schema classes are added to define request payloads, response structures, and security configurations. The existing controller logic remains unchanged; new public methods and routes surface previously internal functionality through enhanced HTTP method bindings and security scope annotations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-106/ This page is automatically updated on each push to this PR. |
| #[ | ||
| OA\SecurityScheme( | ||
| type: 'oauth2', | ||
| securityScheme: 'user_oauth2', |
There was a problem hiding this comment.
@matiasperrone-exo this security schema dupes the one defined at PR 105 ( OAuth2UserSecurity) please review and unify schemas ( use OAuth2UserSecurity )
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/Api/OAuth2/OAuth2UserApiController.php`:
- Around line 499-504: The OpenAPI response for the update operation in
OAuth2UserApiController currently uses HttpResponse::HTTP_CREATED (201); change
it to HttpResponse::HTTP_OK (200) so the OA\Response for the update (the block
creating new OA\Response in OAuth2UserApiController, around the
profile-picture/update endpoint) correctly reflects an update operation
returning 200 instead of 201.
- Around line 801-805: The OpenAPI annotation in OAuth2UserApiController
currently documents an update operation with response:
HttpResponse::HTTP_CREATED (201); change that OA\Response to use
HttpResponse::HTTP_OK (200) or HttpResponse::HTTP_NO_CONTENT (204) instead and
update the controller method that performs the user group assignment (the method
containing this OA\Response) so the actual HTTP response status it returns
matches the new code.
- Around line 397-402: The OpenAPI annotation in OAuth2UserApiController is
incorrectly using HttpResponse::HTTP_CREATED for a PUT update response; update
the OA\Response entry in the controller's update annotation to use
HttpResponse::HTTP_OK (200) so the documented status matches the update
semantics and returned User payload in the OA\JsonContent.
- Around line 450-455: The OpenAPI response annotation in
OAuth2UserApiController uses HttpResponse::HTTP_CREATED for an update endpoint;
change the response status to HttpResponse::HTTP_OK (200) to match an update
operation. Locate the OA\Response entry in the controller annotation (the block
that currently has response: HttpResponse::HTTP_CREATED and description
'Updated') and replace HttpResponse::HTTP_CREATED with HttpResponse::HTTP_OK
and, if desired, update the description to 'OK' or keep 'Updated' for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9a428ab-13a3-4653-a1b3-9b59842b0f6b
📒 Files selected for processing (9)
app/Http/Controllers/Api/OAuth2/OAuth2UserApiController.phpapp/Swagger/Models/UserInfoResponseSchema.phpapp/Swagger/OAuth2UserApiControllerSchemas.phpapp/Swagger/Requests/CreateUserRequestSchema.phpapp/Swagger/Requests/UpdateUserGroupsRequestSchema.phpapp/Swagger/Requests/UpdateUserPicRequestSchema.phpapp/Swagger/Requests/UpdateUserRequestSchema.phpapp/Swagger/Requests/UserFieldsSchema.phpapp/Swagger/Security/UsersOAuth2Schema.php
| responses: [ | ||
| new OA\Response( | ||
| response: HttpResponse::HTTP_CREATED, | ||
| description: 'Updated', | ||
| content: new OA\JsonContent(ref: '#/components/schemas/User') | ||
| ), |
There was a problem hiding this comment.
Incorrect HTTP status code for update operation.
The OpenAPI annotation documents HTTP_CREATED (201) for a PUT update operation. Per REST semantics, updates should return HTTP_OK (200) when returning the updated resource.
Suggested fix
responses: [
new OA\Response(
- response: HttpResponse::HTTP_CREATED,
+ response: HttpResponse::HTTP_OK,
description: 'Updated',
content: new OA\JsonContent(ref: '#/components/schemas/User')
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| responses: [ | |
| new OA\Response( | |
| response: HttpResponse::HTTP_CREATED, | |
| description: 'Updated', | |
| content: new OA\JsonContent(ref: '#/components/schemas/User') | |
| ), | |
| responses: [ | |
| new OA\Response( | |
| response: HttpResponse::HTTP_OK, | |
| description: 'Updated', | |
| content: new OA\JsonContent(ref: '#/components/schemas/User') | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Http/Controllers/Api/OAuth2/OAuth2UserApiController.php` around lines 397
- 402, The OpenAPI annotation in OAuth2UserApiController is incorrectly using
HttpResponse::HTTP_CREATED for a PUT update response; update the OA\Response
entry in the controller's update annotation to use HttpResponse::HTTP_OK (200)
so the documented status matches the update semantics and returned User payload
in the OA\JsonContent.
| ), | ||
| responses: [ | ||
| new OA\Response( | ||
| response: HttpResponse::HTTP_CREATED, | ||
| description: 'Updated', | ||
| content: new OA\JsonContent(ref: '#/components/schemas/User') |
There was a problem hiding this comment.
Incorrect HTTP status code for update operation.
Same issue as updateMe - should use HTTP_OK (200) instead of HTTP_CREATED (201) for the update response.
Suggested fix
responses: [
new OA\Response(
- response: HttpResponse::HTTP_CREATED,
+ response: HttpResponse::HTTP_OK,
description: 'Updated',
content: new OA\JsonContent(ref: '#/components/schemas/User')
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ), | |
| responses: [ | |
| new OA\Response( | |
| response: HttpResponse::HTTP_CREATED, | |
| description: 'Updated', | |
| content: new OA\JsonContent(ref: '#/components/schemas/User') | |
| ), | |
| responses: [ | |
| new OA\Response( | |
| response: HttpResponse::HTTP_OK, | |
| description: 'Updated', | |
| content: new OA\JsonContent(ref: '#/components/schemas/User') | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Http/Controllers/Api/OAuth2/OAuth2UserApiController.php` around lines 450
- 455, The OpenAPI response annotation in OAuth2UserApiController uses
HttpResponse::HTTP_CREATED for an update endpoint; change the response status to
HttpResponse::HTTP_OK (200) to match an update operation. Locate the OA\Response
entry in the controller annotation (the block that currently has response:
HttpResponse::HTTP_CREATED and description 'Updated') and replace
HttpResponse::HTTP_CREATED with HttpResponse::HTTP_OK and, if desired, update
the description to 'OK' or keep 'Updated' for clarity.
| responses: [ | ||
| new OA\Response( | ||
| response: HttpResponse::HTTP_CREATED, | ||
| description: 'Updated', | ||
| content: new OA\JsonContent(ref: '#/components/schemas/User') | ||
| ), |
There was a problem hiding this comment.
Incorrect HTTP status code for update operation.
Same issue - should use HTTP_OK (200) instead of HTTP_CREATED (201) for updating the profile picture.
Suggested fix
responses: [
new OA\Response(
- response: HttpResponse::HTTP_CREATED,
+ response: HttpResponse::HTTP_OK,
description: 'Updated',
content: new OA\JsonContent(ref: '#/components/schemas/User')
),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Http/Controllers/Api/OAuth2/OAuth2UserApiController.php` around lines 499
- 504, The OpenAPI response for the update operation in OAuth2UserApiController
currently uses HttpResponse::HTTP_CREATED (201); change it to
HttpResponse::HTTP_OK (200) so the OA\Response for the update (the block
creating new OA\Response in OAuth2UserApiController, around the
profile-picture/update endpoint) correctly reflects an update operation
returning 200 instead of 201.
| responses: [ | ||
| new OA\Response( | ||
| response: HttpResponse::HTTP_CREATED, | ||
| description: 'Updated' | ||
| ), |
There was a problem hiding this comment.
Incorrect HTTP status code for update operation.
Same issue - should use HTTP_OK (200) or HTTP_NO_CONTENT (204) instead of HTTP_CREATED (201) for updating user group assignments.
Suggested fix
responses: [
new OA\Response(
- response: HttpResponse::HTTP_CREATED,
+ response: HttpResponse::HTTP_OK,
description: 'Updated'
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| responses: [ | |
| new OA\Response( | |
| response: HttpResponse::HTTP_CREATED, | |
| description: 'Updated' | |
| ), | |
| responses: [ | |
| new OA\Response( | |
| response: HttpResponse::HTTP_OK, | |
| description: 'Updated' | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Http/Controllers/Api/OAuth2/OAuth2UserApiController.php` around lines 801
- 805, The OpenAPI annotation in OAuth2UserApiController currently documents an
update operation with response: HttpResponse::HTTP_CREATED (201); change that
OA\Response to use HttpResponse::HTTP_OK (200) or HttpResponse::HTTP_NO_CONTENT
(204) instead and update the controller method that performs the user group
assignment (the method containing this OA\Response) so the actual HTTP response
status it returns matches the new code.
| */ | ||
| #[OA\Put( | ||
| path: '/api/v1/users/{id}/groups', | ||
| summary: 'Update user group assignments (only for account type "SERVICE")', |
There was a problem hiding this comment.
@matiasperrone-exo please remove this and follow the pattern from this PR for service accountshttps://github.com/OpenStackweb/openstackid/commit/446696cead8b4bfa2aa389d039e5aa7fc25469e1#diff-ea26379160b49edfe9456c46a5b96b7258fa1d1192e91ba66d1dd44ebd99f8deR354
| content: new OA\JsonContent(ref: '#/components/schemas/UpdateUserGroupsRequest') | ||
| ), | ||
| responses: [ | ||
| new OA\Response( |
There was a problem hiding this comment.
@matiasperrone-exo please add the 403 response check 446696c#diff-ea26379160b49edfe9456c46a5b96b7258fa1d1192e91ba66d1dd44ebd99f8deR386
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@matiasperrone-exo please review comments and rebase with main to get the proper preview for the swagger doc many thanks
| security: [ | ||
| [ | ||
| 'user_oauth2' => [ | ||
| IUserScopes::Profile, |
There was a problem hiding this comment.
@matiasperrone-exo
Scope semantics : AND vs OR
In OpenAPI, this means all three scopes are required simultaneously. In practice, the UserInfo endpoint returns different claims based on which individual scopes the token has (per the OIDC
spec). If any single scope is sufficient to access the endpoint, the correct representation is:
security: [
['user_oauth2' => [IUserScopes::Profile]],
['user_oauth2' => [IUserScopes::Email]],
['user_oauth2' => [IUserScopes::Address]],
]
This tells Swagger UI "any one of these scopes grants access." Verify against the actual ApiEndpoint database configuration for what's enforced at runtime.
| security: [ | ||
| [ | ||
| 'user_oauth2' => [ | ||
| IUserScopes::Profile, |
There was a problem hiding this comment.
4 participants
Task:
Ref: https://app.clickup.com/t/86b8e6k87
Endpoints:
Summary by CodeRabbit