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

@vincentchalamon vincentchalamon 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 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
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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

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

@dunglas
Copy link
Member

dunglas commented Aug 19, 2019

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

Copy link
Member

@soyuka soyuka left a comment

Choose a reason for hiding this comment

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

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 issue/1646 branch 3 times, most recently from 8e6dcb2 to 558971f Compare August 19, 2019 12:29
@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
@teohhanhui
Copy link
Contributor

teohhanhui 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 issue/1646 branch 2 times, most recently from ec43f9e to e354287 Compare August 19, 2019 15:13
@vincentchalamon vincentchalamon changed the base branch from 2.4 to master August 19, 2019 15:13
@vincentchalamon vincentchalamon force-pushed the issue/1646 branch 2 times, most recently from d57a903 to 4725000 Compare August 19, 2019 15:25
src/Annotation/ApiResource.php Outdated Show resolved Hide resolved
@dunglas dunglas merged commit 23d0f8f into api-platform:master Aug 20, 2019
@dunglas
Copy link
Member

dunglas commented Aug 20, 2019

Awesome! Thanks @vincentchalamon

@c-lambert
Copy link

c-lambert commented Mar 8, 2022

Hello,
i'm not sure to understand the fix applied.
I use API Platform 2.6.4 with security annotation on itemOperation. But when i call this endpoint with unauthorized user, API Platform responds 404 when entity doesn't exist, and 403 when entity exist.

security attribute is not early by default ?

@norkunas
Copy link
Contributor

norkunas commented Mar 8, 2022

security is executed only after reading data, so you could have dynamic security by object data, if you want to not expose that item exist you could use security acl for that

@c-lambert
Copy link

security is executed only after reading data, so you could have dynamic security by object data, if you want to not expose that item exist you could use security acl for that

Ok thank you, i'll do that.
I was hopping that we can bypass this behavior with a parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants