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

Deprecate access_control attribute, add security + security_post_denormalize attributes #2992

Merged
merged 1 commit into from Aug 20, 2019

Conversation

@vincentchalamon
Copy link
Contributor

commented Aug 19, 2019

Q A
Waiting for PR? #2990
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #1646, api-platform/api-platform#1218
License MIT
Doc PR api-platform/docs#859

@vincentchalamon vincentchalamon added the bug label Aug 19, 2019

@vincentchalamon vincentchalamon requested review from dunglas and alanpoulain Aug 19, 2019

@vincentchalamon vincentchalamon self-assigned this Aug 19, 2019

@vincentchalamon vincentchalamon changed the title [WIP] Fix #1646 [WIP] Deprecate access_control attribute, add security + late_security attributes Aug 19, 2019

@vincentchalamon vincentchalamon force-pushed the vincentchalamon:issue/1646 branch from 7ca234d to d58a96a Aug 19, 2019

@dunglas
Copy link
Member

left a comment

Can you also add a legacy test for access_control?

src/EventListener/ReadListener.php Outdated Show resolved Hide resolved
src/GraphQl/Resolver/ResourceAccessCheckerTrait.php Outdated Show resolved Hide resolved
@teohhanhui

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

If we're really doing this, I'd suggest:

"access_control"={
    "precondition"="...",
    "postcondition"="...",
},

@vincentchalamon vincentchalamon force-pushed the vincentchalamon:issue/1646 branch from d58a96a to 4665e2c Aug 19, 2019

@dunglas

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

Sub-attributes are too verbose, I prefer this approach (see my comment in the related issue).

@vincentchalamon vincentchalamon force-pushed the vincentchalamon:issue/1646 branch from 4665e2c to 77fc7f3 Aug 19, 2019

@soyuka
soyuka approved these changes Aug 19, 2019
Copy link
Member

left a comment

Once tests pass 👍

We need to dig through older issues, I'm sure that this solves more then 2 issues :p.

@vincentchalamon vincentchalamon force-pushed the vincentchalamon:issue/1646 branch 3 times, most recently from 8e6dcb2 to 558971f Aug 19, 2019

@vincentchalamon vincentchalamon changed the title [WIP] Deprecate access_control attribute, add security + late_security attributes Deprecate access_control attribute, add security + late_security attributes Aug 19, 2019

@vincentchalamon vincentchalamon force-pushed the vincentchalamon:issue/1646 branch from 558971f to 7b61c47 Aug 19, 2019

@teohhanhui

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

"security" and "late_security" are really confusing names.

If you don't want nested attributes, then perhaps:

  • access_control_precondition
  • access_control_postcondition

Or:

  • authorize_precondition
  • authorize_postcondition

The second option would fit nicely with my suggestion of renaming DenyAccessListener to AuthorizeListener. And of course we could support "authorize"=false.

I don't buy the argument that Symfony uses the term "security" too. It's not related to our case.

@vincentchalamon vincentchalamon changed the title Deprecate access_control attribute, add security + late_security attributes Deprecate access_control attribute, add security + security_post_denormalize attributes Aug 19, 2019

@vincentchalamon vincentchalamon force-pushed the vincentchalamon:issue/1646 branch 2 times, most recently from ec43f9e to e354287 Aug 19, 2019

@vincentchalamon vincentchalamon changed the base branch from 2.4 to master Aug 19, 2019

@vincentchalamon vincentchalamon force-pushed the vincentchalamon:issue/1646 branch 2 times, most recently from d57a903 to 4725000 Aug 19, 2019

src/Annotation/ApiResource.php Outdated Show resolved Hide resolved

@vincentchalamon vincentchalamon force-pushed the vincentchalamon:issue/1646 branch 3 times, most recently from d265e42 to 705fd64 Aug 20, 2019

@vincentchalamon vincentchalamon force-pushed the vincentchalamon:issue/1646 branch from 705fd64 to 1f06b4e Aug 20, 2019

@dunglas dunglas merged commit 23d0f8f into api-platform:master Aug 20, 2019

11 of 13 checks passed

codecov/patch 85.93% of diff hit (target 91.42%)
Details
codecov/project 90.17% (-1.26%) compared to 3c98fdb
Details
Scrutinizer Analysis: 4 new issues, 9 updated code elements – Tests: passed
Details
SymfonyInsight: dunglas / API Platform Core Code quality OK.
Details
ci/circleci: behat-coverage Your tests passed on CircleCI!
Details
ci/circleci: behat-elasticsearch-coverage Your tests passed on CircleCI!
Details
ci/circleci: behat-mongodb-coverage Your tests passed on CircleCI!
Details
ci/circleci: php-cs-fixer Your tests passed on CircleCI!
Details
ci/circleci: phpstan Your tests passed on CircleCI!
Details
ci/circleci: phpunit-coverage Your tests passed on CircleCI!
Details
ci/circleci: phpunit-mongodb-coverage Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dunglas

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

Awesome! Thanks @vincentchalamon

@vincentchalamon vincentchalamon deleted the vincentchalamon:issue/1646 branch Aug 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.