Skip to content

[Laravel]: Fix authorization swagger ui#6661

Merged
soyuka merged 1 commit into
api-platform:4.0from
toitzi:toitzi/authentication
Sep 23, 2024
Merged

[Laravel]: Fix authorization swagger ui#6661
soyuka merged 1 commit into
api-platform:4.0from
toitzi:toitzi/authentication

Conversation

@toitzi
Copy link
Copy Markdown
Contributor

@toitzi toitzi commented Sep 23, 2024

Q A
Branch? 4.0
Tickets Closes #6662
License MIT
Doc PR #1991

@toitzi toitzi changed the title [Laravel]: Fix authorization to swagger ui [Laravel]: Fix authorization swagger ui Sep 23, 2024
@soyuka
Copy link
Copy Markdown
Member

soyuka commented Sep 23, 2024

Nice! Tyvm! Would you be able to squash your commits and reword it and follow the convetional commits? See https://github.com/api-platform/core/blob/main/CONTRIBUTING.md#sending-a-pull-request it'd could be like this: feat(laravel): swagger UI authorization configuration.

Thanks!

@toitzi
Copy link
Copy Markdown
Contributor Author

toitzi commented Sep 23, 2024

@soyuka yes, once im done i will squash commit.

Different question:
currently bearer tokens are not really supported by api-platform (or at least not fully). There is 2 issues i see when adding bearer token support:

First way / issue, would be adding the option to pass a auth key type via the OpenApiFactory, by adapting this code. It is a non-breaking change (and the way i did it in this PR). But the drawback here is that api-platform already uses type in the configuration, which maps to the in of the security scheme. Since that is already in use, so i had to come up with a new option auth_type. While that works just fine, i can see how this can be confusing to developers / maintainers.

Second way (which removes this confusion a bit) would be to adapt the Options file, with a new property and method (getHttpAuth or smth. like that). And use that additionally in the OpenApiFactory:

image

That would be more consistent tbh.

However to avoid breaking changes in the Options Class, i would need to append those values at the end of the constructor.

What would be the preferred way here?

@toitzi
Copy link
Copy Markdown
Contributor Author

toitzi commented Sep 23, 2024

And also: Is this a feature or a bug? Since it is already somewhat implemented but not really working in laravel - i considered it a bug...if you consider it a feature please let me know (because i think i will also need to merge it to a differen branch then right?)

@soyuka
Copy link
Copy Markdown
Member

soyuka commented Sep 23, 2024

I see 2 different things here. For Laravel we consider that every existing feature that is missing as a bug fix for 4.x and therefore the config patch is fine. For the new option (http auth) I'd see this as a new feature as it changes in Symfony as well. I'd open a new PR to support this.

fix swagger ui authentication in laravel, by providing necessary options

Closes: #6662
@toitzi toitzi force-pushed the toitzi/authentication branch from 4b79c8e to 1d6ee85 Compare September 23, 2024 10:57
@toitzi
Copy link
Copy Markdown
Contributor Author

toitzi commented Sep 23, 2024

ok updated to fix only the config, i will make a new PR as feature to support http auth.

@toitzi toitzi marked this pull request as ready for review September 23, 2024 10:58
@soyuka soyuka merged commit 85306f2 into api-platform:4.0 Sep 23, 2024
@soyuka
Copy link
Copy Markdown
Member

soyuka commented Sep 23, 2024

Many thanks!

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.

2 participants