8000 [Security] Add a Not_Full_Fledged_handler in ExceptionListener from security login by eltharin · Pull Request #57661 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Security] Add a Not_Full_Fledged_handler in ExceptionListener from security login #57661

New issu 10000 e

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
Open
wants to merge 8 commits into
base: 7.3
Choose a base branch
from
Open

[Security] Add a Not_Full_Fledged_handler in ExceptionListener from security login #57661

wants to merge 8 commits into from

Conversation

eltharin
Copy link
Contributor
@eltharin eltharin commented Jul 5, 2024 •
edited
Loading
Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #54318
License MIT

My proposal for behaviour for acces Denied When User is logged with a remeberme token

Without set anything, the behaviour is the same as actually.
In the Firewall config adding a not_full_fledged_handler waiting a service handler or a predefined value :

  • predefined values :
    • "original" "redirect" for the same behavior than actual replaced by Symfony\Component\Security\Http\Authorization\NotFullFledgedRedirectToStartAuthenticationHandler
    • "same" "equal" for if a user is logged but not not full fledged (and the attribute is not IS_AUTHENTICATED_FULLY) he has the same acces than if he was full logged. this value is replaced by Symfony\Component\Security\Http\Authorization\NotFullFledgedEqualNormalLoginHandler
  • user can create his own service too, class have to implements Symfony\Component\Security\Http\Authorization\NotFullFledgedHandlerInterface with one method "handle"
    this method can return a reponse to change original Exception Response or null for continue to throw the original access denied exception.

@eltharin eltharin requested a review from chalasr as a code owner July 5, 2024 14:34
@carsonbot carsonbot added this to the 7.2 milestone Jul 5, 2024
@eltharin eltharin changed the title Notfullfletched handler Add a Not Full Fletched handler in Security config Jul 5, 2024
@eltharin eltharin force-pushed the notfullfletchedHandler branch 3 times, most recently from 64b0bdd to 7e36014 Compare July 6, 2024 16:17
@carsonbot carsonbot changed the title Add a Not Full Fletched handler in Security config [Security] Add a Not Full Fletched handler in Security config Jul 8, 2024
@eltharin eltharin force-pushed the notfullfletchedHandler branch from 7e36014 to a7ace88 Compare July 8, 2024 10:12
@eltharin eltharin marked this pull request as draft July 9, 2024 11:30
@eltharin eltharin marked this pull request as ready for review July 9, 2024 13:17
@eltharin eltharin force-pushed the notfullfletchedHandler branch 3 times, most recently from bdb9552 to f64b805 Compare July 11, 2024 11:05
@eltharin eltharin changed the title [Security] Add a Not Full Fletched handler in Security config [Security] Add a Not_Full_Fledged_handler in ExceptionListener from security login Jul 11, 2024
@eltharin eltharin requested a review from OskarStark July 12, 2024 15:32
@eltharin eltharin force-pushed the notfullfletchedHandler branch 4 times, most recently from a5ff172 to 05ca8dd Compare July 26, 2024 11:36
stof
stof reviewed Aug 6, 2024
View reviewed changes
Copy link
Member
@stof stof left a comment

Choose a reason for hiding this comment

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

I'm not convinced by this PR. To me, it does not actually solve the issue it claims to solve (it solves only a subset of cases, bringing an invalid behavior for the other cases).
So for now, it gets a -1 vote from me.

src/Symfony/Component/Security/Http/Authorization/SameAsNotFullFledgedHandler.php Outdated
8000
}

foreach($accessDeniedException->getAttributes() as $attribute) {
if(in_array($attribute, [AuthenticatedVoter::IS_AUTHENTICATED_FULLY])) {
Copy link
Member

Choose a reason for hiding this comment

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

This is not enough to identify all cases where an access denied exception might depend on the trust level of a token (see my comment on the issue).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it totally a case whitch can be set in a custom handler.
SameAsNotFullFledgedHandler is here to anwser to many person, when PR will be merged (yes I beleive on it) if many persons have the same idea it can be possible to propose another PR for add core handlers

@eltharin
Copy link
Contributor Author
eltharin commented Aug 6, 2024

I don't undestand why you say it does not actually solve the issue. the problem is to obtain a control of AccessDenied Exception when RememberMe login is active, this PR allow a custom handler to choose:

  • original beahviour,
  • behaviour asked by many person : treat remeberme as normal login except if attribute is IS_AUTHENTICATED_FULLY
  • and if user want something specific he can make his own handler to get what he want.

I don't see whitch case is not cover ?

@stof
Copy link
Member
stof commented Aug 6, 2024

@eltharin have you read my comment on the issue ?

@eltharin
Copy link
Contributor Author
eltharin commented Aug 9, 2024

I do but I don't understand where you find a problem,
You seem to think that I want the new handler that I provide to do the job of all the projects even the most difficult, but I just provide a solution to simple requests, the biggest ones are to be handled by the developers themselves with the customs.

@eltharin eltharin force-pushed the notfullfletchedHandler branch from 60cb97e to 2c019ce Compare August 12, 2024 15:48
@eltharin eltharin force-pushed the notfullfletchedHandler branch 2 times, most recently from 818ff65 to 91dc0c2 Compare August 13, 2024 21:17
@eltharin eltharin requested a review from OskarStark August 14, 2024 09:54
@eltharin eltharin requested a review from stof September 2, 2024 10:11
@eltharin eltharin force-pushed the notfullfletchedHandler branch from 0815414 to 3e1d1ea Compare September 16, 2024 08:17
@eltharin eltharin force-pushed the notfullfletchedHandler branch from 3e1d1ea to a96aecb Compare September 20, 2024 15:22
@eltharin
Copy link
Contributor Author

What can we do to advance this PR ?

@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
@eltharin eltharin force-pushed the notfullfletchedHandler branch from a96aecb to 0fb0b24 Compare January 9, 2025 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

@OskarStark OskarStark Awaiting requested review from OskarStark

@stof stof Awaiting requested review from stof

@nicolas-grekas nicolas-grekas Awaiting requested review from nicolas-grekas

Assignees
No one assigned
Labels
Feature Security Status: Needs Review
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.

Improve behaviour for Access Denied with Remember Me
6 participants
@eltharin @stof @nicolas-grekas @OskarStark @fabpot @carsonbot
0