Skip to content

feat(subresource): Link Security - #5290

Merged
soyuka merged 21 commits into
api-platform:mainfrom
KDederichs:feature/link_security
Jan 19, 2024
Merged

feat(subresource): Link Security#5290
soyuka merged 21 commits into
api-platform:mainfrom
KDederichs:feature/link_security

Conversation

@KDederichs

@KDederichs KDederichs commented Dec 27, 2022

Copy link
Copy Markdown
Contributor
Q A
Branch? main
Tickets #4819
License MIT
Doc PR api-platform/docs#1692

Here's an idea how Link Security (as described in the issue) might work.
There's still an open issue with choosing the right provider programmatically and mapping the uri variables automatically but I wanted to ask if that's going in the right direction first before investing more work (and making tests)

@KDederichs
KDederichs force-pushed the feature/link_security branch 2 times, most recently from e39959e to ed0413f Compare January 3, 2023 15:59
@KDederichs
KDederichs marked this pull request as ready for review January 3, 2023 16:24
@soyuka

soyuka commented Jan 5, 2023

Copy link
Copy Markdown
Member

Hello I need to find time to really understand the need behind that, why don't you put the security on the top level?

Also, not a huge fan of the work done in the ReadListener it doesn't look it'll work with all cases. Still I only took a quick look I want to try this.

@KDederichs

Copy link
Copy Markdown
Contributor Author

The issue with toplevel is:
When you have permissions that are based on the parent object you can't enforce them using subresources (at least not as I understand it).

For example lets say you got an an Order that has some transactions (/orders/{id}/transactions) :
You could link it and do some global checks but that won't work if you have requirements that pretain the order object , lets say check the owner or something, since you can't check it in a subresource.

Sure you could define your own custom provider for it but IMO it's something that should be doable with subresources, since that's basically the usecase.

@KDederichs
KDederichs force-pushed the feature/link_security branch from 076f8e2 to d074632 Compare January 14, 2023 15:16
Comment thread src/Symfony/EventListener/DenyAccessListener.php
# Conflicts:
#	tests/Fixtures/TestBundle/Document/SecuredDummy.php
#	tests/Fixtures/TestBundle/Entity/SecuredDummy.php
@stale

stale Bot commented Apr 3, 2023

Copy link
Copy Markdown

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale Bot added the stale label Apr 3, 2023
@KDederichs

Copy link
Copy Markdown
Contributor Author

Not stale, still a thing I'd like to see added in some way

@stale stale Bot removed the stale label Apr 3, 2023
@usu

usu commented Oct 8, 2023

Copy link
Copy Markdown
Contributor

@KDederichs This seems like a very interesting enhancement. Are you still working on this? What do you believe is missing to get this mergeable and is there anything that could be supported with?

@KDederichs

Copy link
Copy Markdown
Contributor Author

@usu I'd still like it done because I think it's a valid use case.
Technically it's done even, if I were to resolve the merge conflicts since my initial PR did have the whole functionality.

I'm just not sure why it wasn't merged back then, maybe the team doesn't like it or wants some changes? Dunno.
If you can get some API-Platform members to tell you why that's what would be missing :)

@soyuka

soyuka commented Oct 9, 2023

Copy link
Copy Markdown
Member

Too much work, forgot that PR sorry! I think it's a valid use case. Only thing is that implementation in the ReadListener that I don't like. Please take a look at what we did in the CreateProvider. I think it's the same logic where we try to find another resource. When doing so, its needed that the user can specify an operation to get no?

@KDederichs

KDederichs commented Oct 9, 2023

Copy link
Copy Markdown
Contributor Author

Hmm yeah I'll try to get it out of there into it's own thing.
I'll probably get on it tomorrow or something and get rid of the merge conflicts and then try to change that.

Edit: Or do you just not like the way I did it there and the place itself is fine?

# Conflicts:
#	features/authorization/deny.feature
#	src/Symfony/EventListener/ReadListener.php
#	tests/Fixtures/TestBundle/Entity/SecuredDummy.php
@soyuka soyuka added this to the 3.3 milestone Oct 17, 2023
@usu

usu commented Dec 26, 2023

Copy link
Copy Markdown
Contributor

@soyuka Is this ready to be merged? Would be awesome to see this in the 3.3 release.

@KDederichs

Copy link
Copy Markdown
Contributor Author

@soyuka
Anything missing? :)

Was wondering what the general status of this is

@soyuka

soyuka commented Jan 16, 2024

Copy link
Copy Markdown
Member

let me review this

@soyuka soyuka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Made a few comments, if something is not clear feel free to pm me on the symfony slack.

Comment thread src/State/Provider/ReadProvider.php Outdated
Comment thread src/Symfony/EventListener/DenyAccessListener.php
Comment thread src/Symfony/EventListener/ReadListener.php Outdated
Comment thread src/State/Provider/ReadProvider.php Outdated
Comment thread src/Symfony/Security/State/AccessCheckerProvider.php Outdated
Comment thread src/Symfony/Security/State/AccessCheckerProvider.php Outdated
Comment thread src/State/Provider/LinkedReadProvider.php Outdated
Comment thread tests/Symfony/Security/State/LinkAccessCheckerProviderTest.php
@soyuka

soyuka commented Jan 17, 2024

Copy link
Copy Markdown
Member

Can you add a configuration option and load the xml file for these providers only when it's enabled? Thanks.

@KDederichs
KDederichs force-pushed the feature/link_security branch 5 times, most recently from 9bce0bc to 16b9558 Compare January 18, 2024 10:43

@soyuka soyuka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Comment thread features/authorization/deny.feature Outdated
Comment thread features/authorization/deny.feature Outdated
Comment thread features/authorization/deny.feature Outdated
Comment thread features/authorization/deny.feature Outdated
Comment thread src/Symfony/Bundle/Resources/config/link_security.xml Outdated
Comment thread src/Symfony/Bundle/Resources/config/link_security.xml Outdated
@KDederichs
KDederichs force-pushed the feature/link_security branch from 16b9558 to 207c8c5 Compare January 18, 2024 18:45
…extract link security into their own providers
@KDederichs
KDederichs force-pushed the feature/link_security branch from 207c8c5 to 896b383 Compare January 18, 2024 18:49
Comment thread src/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php Outdated
Comment thread src/Symfony/Bundle/DependencyInjection/Configuration.php Outdated
Comment thread src/Symfony/Security/State/LinkAccessCheckerProvider.php Outdated
Comment thread src/Symfony/Security/State/LinkedReadProvider.php Outdated
Comment thread features/authorization/deny.feature
@KDederichs
KDederichs force-pushed the feature/link_security branch from ec17cb3 to 0d315f7 Compare January 18, 2024 19:36
@soyuka
soyuka merged commit b488366 into api-platform:main Jan 19, 2024
@soyuka

soyuka commented Jan 19, 2024

Copy link
Copy Markdown
Member

thanks! please also add a documentation entry on api-platform.com/docs

@KDederichs

Copy link
Copy Markdown
Contributor Author

@soyuka There's a docs PR already, the only thing that's missing there is the enable_link_security flag I guess :)
It's linked here so maybe give it a quick look if something else is missing in there

@ghost

ghost commented Sep 6, 2024

Copy link
Copy Markdown

I'm looking for YAML configuration of this feature. The docs provide an example using attributes only.

Adding the security option as a sibling of a link's fromClass/toProperty/etc is ignored in YAML. @KDederichs can I request a docs update for YAML and/or XML config? Or if you tell me how to configure it, I'd be happy to make the docs update myself.

@KDederichs

Copy link
Copy Markdown
Contributor Author

There's no YAML/XML config for this.
You'd have to make a PR for that, it should be fairly simple though? Not completely sure never really dug into how Api Platform loads config values from XML/YAML.
Maybe @soyuka can tell you how much work that'd be

@ytilotti

ytilotti commented Apr 4, 2025

Copy link
Copy Markdown
Contributor

@amacrobert-meq same issue here! And it's really blocking me. Don't find why is not set in the config load.

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.

5 participants