-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[HttpKernel] Add #[IsSignatureValid] attribute
#60395
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
base: 7.4
Are you sure you want to change the base?
[HttpKernel] Add #[IsSignatureValid] attribute
#60395
Conversation
4730169 to
c72ef83
Compare
src/Symfony/Component/Security/Http/EventListener/IsSignatureValidAttributeListener.php
Outdated
Show resolved
Hide resolved
|
This is looking great @santysisi! I'll give it a more thorough review after 7.3 is released. |
c72ef83 to
85320cf
Compare
|
We could also consider adding an extra argument to the attribute called |
85320cf to
c000b69
Compare
ready these 2 changes 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really great, nice work @santysisi!
c000b69 to
24fca48
Compare
24fca48 to
524b686
Compare
|
I believe the error is not relevant 🤔 |
src/Symfony/Component/Security/Http/Attribute/IsSignatureValid.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/Attribute/IsSignatureValid.php
Outdated
Show resolved
Hide resolved
| @@ -37,7 +37,8 @@ | |||
| "symfony/security-csrf": "^6.4|^7.0|^8.0", | |||
| "symfony/translation": "^6.4|^7.0|^8.0", | |||
| "psr/log": "^1|^2|^3", | |||
| "web-token/jwt-library": "^3.3.2|^4.0" | |||
| "web-token/jwt-library": "^3.3.2|^4.0", | |||
| "symfony/dependency-injection": "^6.4|^7.0|^8.0" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the symfony/dependency-injection component as a dev dependency because I need the ContainerBuilder to test the new CompilerPass.
2290ba3 to
5d3a69a
Compare
| public function process(ContainerBuilder $container): void | ||
| { | ||
| $locateableServices = []; | ||
| foreach ($container->getDefinitions() as $id => $definition) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think looping over all the definitions is the correct method to handle this. @nicolas-grekas wdyt?
Also, UriSigner isn't final so it could be an sub-class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi 👋 Thanks a lot for your comment!
You're absolutely right, that was totally my mistake 🙏 .
Since UriSigner isn’t final, it can indeed be subclassed. I’ve updated the condition in the if statement to use is_a to correctly check if it's an instance of UriSigner or one of its subclasses.
I also added a test to cover this behavior and ensure it works as expected.
Thanks again for catching that ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 You might be right and apologies if this isn’t the proper way to handle it.
I based my implementation on existing methods in ContainerBuilder, like findTaggedServiceIds, findTaggedResourceIds, and findTags, which also loop through definitions. But again, that might not be ideal in this case, sorry about that!
I'd really appreciate your input on a better approach. If you can suggest one, I’ll be happy to update the code to follow your recommendation.
Thanks again!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! 👋
I’ve tried a new approach to create the ServiceLocator.
I added autoconfiguration for UriSigner with a security.uri_signer tag, and I’m now using findTaggedServiceIds instead of looping through all the definitions.
Please let me know if this method looks better to you 😄
Thanks again! ❤️
Edit: I was also thinking , maybe we could introduce a UriSignerInterface instead. That way, instead of autoconfiguring the concrete UriSigner, we could autoconfigure services implementing UriSignerInterface. This change would allow developers to pass not only the UriSigner service or ones that extend it, but any custom service that implements the interface.
Wdyt? If you agree, I’d be happy to open another PR to add the interface 😄
30c02bd to
f20400b
Compare
…h argument (santysisi) This PR was merged into the 7.4 branch. Discussion ---------- [HttpKernel][Security] Refactor: use `getAttributes` with argument | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | no | License | MIT Refactored code to consistently use `$event->getAttributes($className)` instead of accessing the full array. This simplifies attribute retrieval and improves readability. [Reference](#60395 (comment)) Commits ------- 6769c9a [HttpKernel][Security] Refactor: use `getAttributes` with argument
…h argument (santysisi) This PR was merged into the 7.4 branch. Discussion ---------- [HttpKernel][Security] Refactor: use `getAttributes` with argument | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | no | License | MIT Refactored code to consistently use `$event->getAttributes($className)` instead of accessing the full array. This simplifies attribute retrieval and improves readability. [Reference](symfony/symfony#60395 (comment)) Commits ------- 6769c9a8287 [HttpKernel][Security] Refactor: use `getAttributes` with argument
| * | ||
| * @author Santiago San Martin <sanmartindev@gmail.com> | ||
| */ | ||
| class UriSignerLocatorPass implements CompilerPassInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the tagged service is the way to go. If so, we don't need this compiler pass. You can register controller.is_signature_valid_attribute_listener in config and use tagged_locator() for its argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your help! ❤️ I've made the changes.
Edit: There are some errors, I'll try to resolve them tonight.
a7d8c95 to
1d8202c
Compare
|
Awesome, let's wait for some others to take a look. As I said, I think tagged services is the way to go but maybe someone has a different idea. |
1d8202c to
d2ff216
Compare
|
I did not go over all comments, but for me it looks false this live inside The UriSigner I can use currently without any SecurityBundle / Security services, so from my point of view the attribute should work also without any security related service and be more part of the FrameworkBundle like the UriSigner service itself. |
d2ff216 to
35cbbc0
Compare
Hi 👋 First of all, thank you very much for your comment 😊 You're right. When I started working on this feature, I saw this new attribute as something similar to However, you're right that Thanks again for the comment! What do you think, @kbond? |
|
Yeah, this is a good point. I wonder if |
|
@kbond HttpKernel sounds good for me if I look at the attributes there: https://github.com/symfony/symfony/tree/7.4/src/Symfony/Component/HttpKernel/Attribute |
35cbbc0 to
0c3ec83
Compare
|
Moved the attribute to the |
#[IsSignatureValid] attribute#[IsSignatureValid] attribute
0c3ec83 to
5b0b74e
Compare
5b0b74e to
c304021
Compare
✨ New Feature:
#[IsSignatureValid]AttributeThis attribute enables declarative signature validation on controller actions. It ensures that requests using specific HTTP methods are validated using
UriSigner. If the signature is invalid, aSignedUriExceptionis thrown.✅ Usage Examples