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

[Symfony 6.2] Improve SecurityAttributeToIsGrantedAttributeRector #640

Merged
merged 4 commits into from
Jul 5, 2024

Conversation

TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented Jul 4, 2024

Follow up to #622

Covered cases:

-#[Security("is_granted('ROLE_ADMIN')")]
+#[\Symfony\Component\Security\Http\Attribute\IsGranted(attribute: 'ROLE_ADMIN')]
-#[Security("is_granted('ROLE_ADMIN', someArea)")]
+#[\Symfony\Component\Security\Http\Attribute\IsGranted(attribute: 'ROLE_ADMIN', subject: 'someArea')]

I've looked at the other cases, but due to it's stringy nature, we can't parse it and covert it into different structures.
This is the best we can automate, the rest would have to be dealt with manually.


Closes #393

@TomasVotruba TomasVotruba changed the title tv sensio security [Symfony 6.1] Improve SecurityAttributeToIsGrantedAttributeRector Jul 4, 2024
@TomasVotruba TomasVotruba changed the title [Symfony 6.1] Improve SecurityAttributeToIsGrantedAttributeRector [Symfony 6.2] Improve SecurityAttributeToIsGrantedAttributeRector Jul 4, 2024
@TomasVotruba
Copy link
Member Author

cc @wkania I think this is the best we can get :)

@TomasVotruba
Copy link
Member Author

I've noticed array of roles in the Symofny PR: symfony/symfony#46907

Would this help? Is this correct conversion?

-#[Security("is_granted('ROLE_ADMIN') or is_granted('ROLE_FRIENDLY_USER')")]
+#[IsGranted(attributes: ['ROLE_ADMIN', 'ROLE_FRIENDLY_USER'])]

@wkania
Copy link
Contributor

wkania commented Jul 5, 2024

I've looked at the other cases, but due to it's stringy nature, we can't parse it and covert it into different structures.

From the code I see that we do have access to this value so we can modify it? Is parsing complexity the problem?

Would this help? Is this correct conversion?

There are a lot cases with or/and etc so I would be against such changes.

@stefantalen
Copy link
Contributor

I've noticed array of roles in the Symofny PR: symfony/symfony#46907

Would this help? Is this correct conversion?

-#[Security("is_granted('ROLE_ADMIN') or is_granted('ROLE_FRIENDLY_USER')")]

+#[IsGranted(attributes: ['ROLE_ADMIN', 'ROLE_FRIENDLY_USER'])]

Using arrays for roles has been deprecated, from what I know each role needs its own #[IsGranted]

I've recently made some changes in a rule for that

@TomasVotruba
Copy link
Member Author

Using arrays for roles has been deprecated, from what I know each role needs its own #[IsGranted]
I've recently made some changes in a rule for that

What kind of changes? Maybe we can includ them here.

@TomasVotruba
Copy link
Member Author

@wkania

From the code I see that we do have access to this value so we can modify it? Is parsing complexity the problem?

The parsing is not possible, because it's not PHP. It's only regex on string :) e.g: https://github.com/rectorphp/rector-symfony/pull/640/files#diff-56e5b788455db473092a5cc9b737f8301fe89a7c60863fd483d0fc71a84e8049R51

@stefantalen
Copy link
Contributor

#583

@TomasVotruba
Copy link
Member Author

@stefantalen I see. That's probably out of scope of this attribute, right?

@TomasVotruba
Copy link
Member Author

The Attribute seems to support multiple roles, at least during it's first PR :)

https://github.com/symfony/symfony/pull/46907/files#diff-60f77e573fc91a3cfc2540e301a990850e2de17216050c55ce1d8b0aaeeccd58R16

@stefantalen
Copy link
Contributor

True, but maybe some logic could be extracted

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Jul 5, 2024

I think without before/after tested in the wild, there would be lot of guessing how these work.

Merging as this best we can get at the moment :)

Thanks for feedback everyone 👍

@TomasVotruba TomasVotruba merged commit 415d80f into main Jul 5, 2024
5 checks passed
@TomasVotruba TomasVotruba deleted the tv-sensio-security branch July 5, 2024 09:15
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.

[Symfony62] Replace Sensio Security
3 participants