8000 [Worflow] Fixed GuardListener when using the new Security system by lyrixx · Pull Request #39671 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Worflow] Fixed GuardListener when using the new Security system #39671

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 15, 2021
Merged

[Worflow] Fixed GuardListener when using the new Security system #39671

merged 1 commit into from
Feb 15, 2021

Conversation

lyrixx
Copy link
Member
@lyrixx lyrixx commented Dec 31, 2020
Q A
Branch? 5.2
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #39505
License MIT
Doc PR

@lyrixx lyrixx requested review from chalasr and wouterj as code owners December 31, 2020 14:45 8000
@carsonbot carsonbot added this to the 5.2 milestone Dec 31, 2020
nicolas-grekas
nicolas-grekas reviewed Jan 4, 2021
View reviewed changes
@lyrixx
Copy link
Member Author
lyrixx commented Feb 4, 2021

I choose a totally different approach, like suggested by Wouter there

@fabpot
Copy link
Member
fabpot commented Feb 5, 2021

@wouterj Can you review this one please?

@wouterj
Copy link
Member
wouterj commented Feb 5, 2021

I'm not so sure about the 5.2 target for this PR:

  • It's changing behavior, so it does need a CHANGELOG entry
  • It's remove an (exception) class, I don't think we are allowed to do so in any 5.x release?

We may be able to work around (1) by only doing this in the new system. Maybe we should set a parameter (e.g. security.exception_on_no_token) that is true when the new system is used and false otherwise. We already have some services depending on this knowledge, by using a parameter we can also depend on it in the GuardListener.

@lyrixx
Copy link
Member Author
lyrixx commented Feb 6, 2021

I'm not sure we must do something more about the BC. We open a door here, we don't close one.
Previously, a user MUST put a token in the token storage to make it work.
So we can be 100% sure a token is present.
So removing this check is dead code for current code. We only allow new security system to work

But maybe, I should let the removed exception, and deprecate it...

@chalasr
Copy link
Member
chalasr commented Feb 7, 2021 •
edited
Loading

We open a door here, we don't close one.

Looking at the code, I agree here. This stops throwing an undocumented exception, which is fine for me as a bugfix.

But maybe, I should let the removed exception, and deprecate it...

I think so :) it's a hard break, we can do it smoothly on 5.x.

@wouterj
Copy link
Member
wouterj commented Feb 7, 2021

What I was meaning is that expressions now need to handle token and user being null, whereas it always was TokenInterface and UserInterface before the changes in this PR, right? I haven't used Workflow at all, so I can't judge whether this is a blocker. On a second thought, as it only happens when using the experimental system (and it doesn't work at all for the new system), it's probably OK for a patch release.

@lyrixx
Copy link
Member Author
lyrixx commented Feb 7, 2021

yes and yes :) an anyway, without talking about the new security system, I think the patch is valid anyway.
When we look at why this exception has been added, when understand that this exception is not really required, and the new code seems more natural

@lyrixx lyrixx merged commit b15bfc4 into symfony:5.2 Feb 15, 2021
@lyrixx lyrixx deleted the workflow-secu branch February 15, 2021 14:23
@chalasr chalasr mentioned this pull request Feb 15, 2021
Merged
@chalasr
Copy link
Member
chalasr commented Feb 15, 2021

no approval, not good! :) See #40201

@lyrixx
Copy link
Member Author
lyrixx commented Feb 15, 2021 •
edited
Loading

@chalasr I know there are no approval, but the more than one month, without a clear direction of what I must do. So I do what I think is the best and I merged it 11 days later :)

Anyway, thanks for taking care of the BC

chalasr added a commit that referenced this pull request Feb 15, 2021
@chalasr
minor #40201 [Workflow] Re-add InvalidTokenConfigurationException for…
7078382 
… BC (chalasr)

This PR was merged into the 5.2 branch.

Discussion
----------

[Workflow] Re-add InvalidTokenConfigurationException for BC

| Q             | A
| ------------- | ---
| Branch?       | 5.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Reverts the BC breaking part of #39671

Commits
-------

b596568 [Workflow] Re-add InvalidTokenConfigurationException for BC
@fabpot fabpot mentioned this pull request Mar 4, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@chalasr chalasr Awaiting requested review from chalasr

@wouterj wouterj Awaiting requested review from wouterj

Assignees
No one assigned
Labels
Bug Status: Needs Work
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
6 participants
@lyrixx @fabpot @wouterj @chalasr @nicolas-grekas @carsonbot
0