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.

Sign up for GitHub

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

Jump to bottom

Feat(api-platform): handle scope on endpoints #33756

Merged
merged 3 commits into from
Sep 19, 2023
Merged

Feat(api-platform): handle scope on endpoints #33756

merged 3 commits into from
Sep 19, 2023

Conversation

tleon
Copy link
Contributor

@tleon tleon commented Aug 29, 2023 •
edited
Loading

Questions Answers
Branch? develop
Description?
  • Modified the auth server to return scopes
  • Added thoses scopes to the in memory user
  • Configured api platform to integrate those scopes
  • Decorated an api platform service to transform extra property "scopes" into a security rule
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
How to test?
Fixed issue or discussion? Fixes #33242
Sponsor company PrestaShop SA

More informations on how to get your bearer token :

You need to add a return statement at the start of the onKernelRequest method of the SslMiddleware.php file. If you don't do this postman won't be able to handle the current ssl protocol.

@tleon tleon self-assigned this Aug 29, 2023
@tleon tleon requested a review from a team as a code owner August 29, 2023 14:40
@prestonBot prestonBot added develop Branch Improvement Type: Improvement labels Aug 29, 2023
@tleon tleon force-pushed the Issue-33242-handle-scopes-on-endpoints branch 2 times, most recently from 5394ed4 to bf4e8a5 Compare August 29, 2023 15:23
mflasquin
mflasquin previously requested changes Aug 29, 2023
View reviewed changes
app/config/security_dev.yml Outdated Show resolved Hide resolved
src/PrestaShopBundle/Security/OAuth2/Repository/NullScopeRepository.php Outdated Show resolved Hide resolved
@ps-jarvis ps-jarvis added the Waiting for author Status: action required, waiting for author feedback label Aug 29, 2023
@tleon tleon force-pushed the Issue-33242-handle-scopes-on-endpoints branch from bf4e8a5 to 6f20d74 Compare August 29, 2023 15:52
@tleon tleon requested a review from mflasquin August 29, 2023 16:12
jolelievre
jolelievre reviewed Aug 30, 2023
View reviewed changes
...staShopBundle/ApiPlatform/Decorator/AttributesResourceMetadataCollectionFactoryDecorator.php Show resolved Hide resolved
...staShopBundle/ApiPlatform/Decorator/AttributesResourceMetadataCollectionFactoryDecorator.php Outdated Show resolved Hide resolved
...staShopBundle/ApiPlatform/Decorator/AttributesResourceMetadataCollectionFactoryDecorator.php Outdated Show resolved Hide resolved
...staShopBundle/ApiPlatform/Decorator/AttributesResourceMetadataCollectionFactoryDecorator.php Outdated Show resolved Hide resolved
src/PrestaShopBundle/ApiPlatform/Resources/Hook.php Outdated Show resolved Hide resolved
src/PrestaShopBundle/Security/OAuth2/Repository/NullScopeRepository.php Outdated Show resolved Hide resolved
src/PrestaShopBundle/Security/OAuth2/Repository/NullScopeRepository.php Outdated Show resolved Hide resolved
src/PrestaShopBundle/Security/OAuth2/Repository/ScopeEntity.php Outdated Show resolved Hide resolved
app/config/security_test.yml Outdated Show resolved Hide resolved
@tleon tleon force-pushed the Issue-33242-handle-scopes-on-endpoints branch from 6f20d74 to 5d39bef Compare August 30, 2023 13:27
@tleon tleon requested a review from jolelievre August 30, 2023 13:27
@tleon tleon force-pushed the Issue-33242-handle-scopes-on-endpoints branch 2 times, most recently from 3b9d978 to f714255 Compare September 4, 2023 13:05
jolelievre
jolelievre reviewed Sep 6, 2023
View reviewed changes
app/config/security_test.yml Outdated Show resolved Hide resolved
src/PrestaShopBundle/ApiPlatform/Resources/Hook.php Outdated Show resolved Hide resolved
@tleon tleon force-pushed the Issue-33242-handle-scopes-on-endpoints branch from f714255 to 6b8de8c Compare September 6, 2023 08:20
@tleon tleon requested a review from jolelievre September 6, 2023 08:44
@tleon tleon force-pushed the Issue-33242-handle-scopes-on-endpoints branch from 6b8de8c to a298f72 Compare September 6, 2023 09:07
@tleon tleon force-pushed the Issue-33242-handle-scopes-on-endpoints branch 2 times, most recently from 54ad542 to ef98b68 Compare September 6, 2023 13:14
jolelievre
jolelievre approved these changes Sep 6, 2023
View reviewed changes
Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

Thanks @tleon

@jolelievre jolelievre removed the Waiting for author Status: action required, waiting for author feedback label Sep 7, 2023
boherm
boherm previously approved these changes Sep 8, 2023
View reviewed changes
Copy link
Member

@boherm boherm left a comment

Choose a reason for hiding this comment

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

LGTM, and tested on my side as asked.
All working in the right way!

@ps-jarvis ps-jarvis added the Waiting for QA Status: action required, waiting for test feedback label Sep 8, 2023
@boherm boherm added this to the 9.0.0 milestone Sep 8, 2023
@boherm boherm removed the Waiting for QA Status: action required, waiting for test feedback label Sep 8, 2023
@boherm boherm added the QA ✔️ Status: check done, code approved label Sep 8, 2023
@boherm
Copy link
Member

boherm commented Sep 8, 2023

Before merge, @tleon can you check the failed job in your UI tests: https://github.com/jolelievre/ga.tests.ui.pr/actions/runs/6097740635/job/16579414231 ?

@jolelievre jolelievre removed the QA ✔️ Status: check done, code approved label Sep 8, 2023
jolelievre
jolelievre requested changes Sep 8, 2023
View reviewed changes
Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

@boherm @tleon Indeed as long as these tests are red we can't merge the PR or it will break all the following UI tests
Besides it's the tests related to API and I restarted them a few times so there are probably some stuff to fix

It's in the tests related to Keycloak so my guess is that the token returned by Keycload has no scopes defined (which makes sense since he doesn't know the, it doesn't provide default scopes and we also don't request for scopes in the token request)

@jolelievre jolelievre mentioned this pull request Sep 15, 2023
Closed
@jolelievre jolelievre requested a review from a team as a code owner September 15, 2023 10:00
@jolelievre jolelievre force-pushed the Issue-33242-handle-scopes-on-endpoints branch from 195b443 to b94b2ef Compare September 15, 2023 11:31
jolelievre
jolelievre approved these changes Sep 19, 2023
View reviewed changes
Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

Thanks @tleon

@ps-jarvis ps-jarvis added the Waiting for QA Status: action required, waiting for test feedback label Sep 19, 2023
Progi1984
Progi1984 approved these changes Sep 19, 2023
View reviewed changes
Copy link
Member

@Progi1984 Progi1984 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

OK for UI Tests

(That disables UI Test for external auth server and will enable it in #33946)

(Ping @boubkerbribri)

@florine2623 florine2623 added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Sep 19, 2023
@florine2623
Copy link
Contributor

As discussed with @tleon and @boherm , this PR has been QA by dev and is approved ^^
Thanks all !

@mflasquin
Copy link
Contributor

thanks @tleon !

@jolelievre
Copy link
Contributor

Last run to be sure https://github.com/jolelievre/ga.tests.ui.pr/actions/runs/6234867078

@jolelievre
Copy link
Contributor

Last run green (except sanity tests but not related to the PR)

Thanks @tleon

@jolelievre jolelievre dismissed mflasquin’s stale review September 19, 2023 12:44

Modifications have been addressed

@jolelievre jolelievre merged commit 9f71ee3 into PrestaShop:develop Sep 19, 2023
@jolelievre jolelievre changed the title feat(api-platform): handle scope on endpoints Feat(api-platform): handle scope on endpoints Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boubkerbribri boubkerbribri boubkerbribri approved these changes

@boherm boherm boherm approved these changes

@jolelievre jolelievre jolelievre approved these changes

@matthieu-rolland matthieu-rolland matthieu-rolland approved these changes

@Progi1984 Progi1984 Progi1984 approved these changes

@mflasquin mflasquin Awaiting requested review from mflasquin

Assignees

@tleon tleon

@boherm boherm

Labels
develop Branch Improvement Type: Improvement QA ✔️ Status: check done, code approved
Projects
PR Dashboard
Archived in project
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.

Handle scopes on endpoints
10 participants
@tleon @boherm @florine2623 @mflasquin @jolelievre @Progi1984 @matthieu-rolland @boubkerbribri @ps-jarvis @prestonBot