Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(openapi): not forbidden response on openAPI doc #6886

Open
wants to merge 1 commit into
base: 3.4
Choose a base branch
from

Conversation

vinceAmstoutz
Copy link
Contributor

@vinceAmstoutz vinceAmstoutz commented Dec 24, 2024

Q A
Branch? 3.4
Tickets Closes #6872
License MIT

Add OpenAPI documentation for the forbidden responses (see issue #6872 for more details).

@vinceAmstoutz
Copy link
Contributor Author

vinceAmstoutz commented Dec 24, 2024

@p-pichet This patch should resolve your issue, as shown below:
image

@soyuka Could you help me with adding tests? I'm encountering some issues with the large OpenApiFactoryTest

@vinceAmstoutz vinceAmstoutz force-pushed the fix/openapi-forbidden-response-for-secured-resources branch from 5e272a1 to 1db3893 Compare December 30, 2024 10:16
@vinceAmstoutz
Copy link
Contributor Author

A proposal by message from @vincentchalamon to add Forbidden instead of Forbidden: You must meet following condition(s) to access this resource: .... mainly for security reasons.

What is your opinion? @dunglas @soyuka

Did it suit your needs? @p-pichet

@p-pichet
Copy link

A proposal by message from @vincentchalamon to add Forbidden instead of Forbidden: You must meet following condition(s) to access this resource: .... mainly for security reasons.

What is your opinion? @dunglas @soyuka

Did it suit your needs? @p-pichet

In first thought, i think forbidden is enough and avoid to display to much but in second tought, i don't know.

By the way, do it work with this case

#[ApiResource(
operations : [], // all endpoints 
security: 'is_granted()' 
)]
class MyResource

@vinceAmstoutz vinceAmstoutz force-pushed the fix/openapi-forbidden-response-for-secured-resources branch from 1db3893 to 58faec8 Compare December 30, 2024 10:36
@vinceAmstoutz
Copy link
Contributor Author

vinceAmstoutz commented Dec 30, 2024

A proposal by message from @vincentchalamon to add Forbidden instead of Forbidden: You must meet following condition(s) to access this resource: .... mainly for security reasons.
What is your opinion? @dunglas @soyuka
Did it suit your needs? @p-pichet

In first thought, i think forbidden is enough and avoid to display to much but in second tought, i don't know.

By the way, do it work with this case

#[ApiResource(
operations : [], // all endpoints 
security: 'is_granted()' 
)]
class MyResource

Thanks for your response!

Yes, it should work with your example. I'll add some tests with this scenario to make sure.

Note that all operations are provided by default, adding operations: [] disables all operations.

@vinceAmstoutz vinceAmstoutz force-pushed the fix/openapi-forbidden-response-for-secured-resources branch from 58faec8 to 9275a51 Compare December 30, 2024 13:10
@vinceAmstoutz vinceAmstoutz requested a review from soyuka December 30, 2024 13:11
Copy link

@p-pichet p-pichet left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the work

@vinceAmstoutz vinceAmstoutz force-pushed the fix/openapi-forbidden-response-for-secured-resources branch from 9275a51 to 9c12f73 Compare January 7, 2025 11:24
@vinceAmstoutz
Copy link
Contributor Author

Tests DONE ☑️ @soyuka

@vinceAmstoutz vinceAmstoutz force-pushed the fix/openapi-forbidden-response-for-secured-resources branch from 9c12f73 to c6a756c Compare January 7, 2025 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants