Skip to content

Conversation

@matiasperrone-exo
Copy link
Contributor

@matiasperrone-exo matiasperrone-exo commented Feb 5, 2026

Task:

Ref: https://app.clickup.com/t/86b7fj5wh

Endpoints affected:

Verbs Endpoint Method
GET,HEAD api/v2/users/{id} getV2

@matiasperrone-exo matiasperrone-exo self-assigned this Feb 5, 2026
@matiasperrone-exo matiasperrone-exo added the documentation Improvements or additions to documentation label Feb 5, 2026
@matiasperrone-exo matiasperrone-exo changed the base branch from feat/openapi---initial to main February 6, 2026 20:26
Copy link

@martinquiroga-exo martinquiroga-exo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@matiasperrone-exo matiasperrone-exo changed the title Feature | Add OpenAPI documentation controller OAuth2UserApiController Feature | Add OpenAPI documentation controller OAuth2UserApiController v2 routes Feb 11, 2026
@matiasperrone-exo matiasperrone-exo force-pushed the feat/openapi----api-v2---oauth2userapicontroller branch from e06252a to bba5793 Compare February 11, 2026 22:38
Copy link
Contributor

@caseylocker caseylocker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. CRITICAL
    app/Swagger/Security/UsersOAuth2Schema.php uses hardcoded relative paths:
  authorizationUrl: '/oauth2/auth',
  tokenUrl: '/oauth2/token',

Per project conventions like in summit-api, these must use the L5-Swagger constants:

  authorizationUrl: L5_SWAGGER_CONST_AUTH_URL,
  tokenUrl: L5_SWAGGER_CONST_TOKEN_URL,

The .env.example already defines L5_SWAGGER_CONST_AUTH_URL and L5_SWAGGER_CONST_TOKEN_URL, and the config registers them under defaults.constants. Hardcoding defeats the purpose of environment-specific URLs.

app/Http/Controllers/Api/OAuth2/OAuth2UserApiController.php
The getV2 route in routes/api_v2.php is wrapped in service.account middleware:

Route::get('', ['middleware' => 'service.account', 'uses' => 'OAuth2UserApiController@getV2']);

The OpenAPI attribute on getV2 has summary: 'Get a user by ID' but no description mentioning the middleware requirement. Per conventions, it should include something like:

description: 'Requires service account authentication (middleware: service.account)',

UsersOAuth2Schema.php only defines one scope:

IUserScopes::ReadAll => 'Read All Users Data',

The security scheme definition should include all scopes that will be referenced across User endpoints (not just the one used by this PR's endpoint). Looking at IUserScopes, the scheme should also include at minimum: MeRead, Write, UserGroupWrite, and Registration — any scope that future User/Group endpoints will reference.

config/l5-swagger.php Copy/Paste

'title' => 'Summit API Swagger UI',

Should be 'OpenStackID API Swagger UI' or similar. This is a copy-paste from the summit-api project.

@matiasperrone-exo matiasperrone-exo force-pushed the feat/openapi----api-v2---oauth2userapicontroller branch from 396c732 to 9e829a1 Compare February 12, 2026 19:15
@matiasperrone-exo
Copy link
Contributor Author

Thanks @caseylocker for the comments.

The response to each point:

  1. I've replaced the string by the env variables as requested, this change was also done in all the other PRs: Feat | API OAuth2UserApiController routes v1 #106, Feat | Add OpenAPI documentation for OAuth2UserRegistrationRequestApiController #108, Feat | Add OpenAPI documentation for OAuth2GroupApiController #109, Feat | Add OpenAPI documentation for OAuth2StreamChatSSOApiController #110 that are out there so far.
  2. Let me talk with Martin about this, I am not sure if I follow, we never documented middleware in Summit API before, it seems unrelated to OpenAPI documentation standards.
  3. The rest of the scopes are present in a different branch feat/openapi----api-v1---oauth2userapicontroller (PR Feat | API OAuth2UserApiController routes v1 #106) as this branch only covers one endpoint.
  4. This was fixed on PR Chore | Initial setup for OpenAPI 3.1.2 documentation #103 and rebased in this PR.
  1. CRITICAL
    app/Swagger/Security/UsersOAuth2Schema.php uses hardcoded relative paths:
  authorizationUrl: '/oauth2/auth',
  tokenUrl: '/oauth2/token',

Per project conventions like in summit-api, these must use the L5-Swagger constants:

  authorizationUrl: L5_SWAGGER_CONST_AUTH_URL,
  tokenUrl: L5_SWAGGER_CONST_TOKEN_URL,

The .env.example already defines L5_SWAGGER_CONST_AUTH_URL and L5_SWAGGER_CONST_TOKEN_URL, and the config registers them under defaults.constants. Hardcoding defeats the purpose of environment-specific URLs.

app/Http/Controllers/Api/OAuth2/OAuth2UserApiController.php
The getV2 route in routes/api_v2.php is wrapped in service.account middleware:

Route::get('', ['middleware' => 'service.account', 'uses' => 'OAuth2UserApiController@getV2']);

The OpenAPI attribute on getV2 has summary: 'Get a user by ID' but no description mentioning the middleware requirement. Per conventions, it should include something like:

description: 'Requires service account authentication (middleware: service.account)',

UsersOAuth2Schema.php only defines one scope:

IUserScopes::ReadAll => 'Read All Users Data',

The security scheme definition should include all scopes that will be referenced across User endpoints (not just the one used by this PR's endpoint). Looking at IUserScopes, the scheme should also include at minimum: MeRead, Write, UserGroupWrite, and Registration — any scope that future User/Group endpoints will reference.

config/l5-swagger.php Copy/Paste

'title' => 'Summit API Swagger UI',

Should be 'OpenStackID API Swagger UI' or similar. This is a copy-paste from the summit-api project.

@matiasperrone-exo
Copy link
Contributor Author

@caseylocker I checked and added the legend "(only for service accounts)" to the description

Signed-off-by: matiasperrone-exo <matias.perrone@exomindset.co>
Signed-off-by: matiasperrone-exo <matias.perrone@exomindset.co>
…els schemas

Signed-off-by: matiasperrone-exo <matias.perrone@exomindset.co>
Signed-off-by: matiasperrone-exo <matias.perrone@exomindset.co>
Signed-off-by: matiasperrone-exo <matias.perrone@exomindset.co>
Signed-off-by: matiasperrone-exo <matias.perrone@exomindset.co>
Signed-off-by: matiasperrone-exo <matias.perrone@exomindset.co>
Signed-off-by: matiasperrone-exo <matias.perrone@exomindset.co>
… v2 routes

Signed-off-by: matiasperrone-exo <matias.perrone@exomindset.co>
…for URLs

Signed-off-by: matiasperrone-exo <matias.perrone@exomindset.co>
Signed-off-by: matiasperrone-exo <matias.perrone@exomindset.co>
@matiasperrone-exo matiasperrone-exo force-pushed the feat/openapi----api-v2---oauth2userapicontroller branch from b1f6b47 to 429d85e Compare February 12, 2026 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants