Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

#21571 updated "Authentication Success and Failure Events" section#11457

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
javiereguiluz merged 1 commit intosymfony:4.4fromoleg-andreyev:issue-21571
Sep 24, 2019

Conversation

@oleg-andreyev
Copy link
Contributor

@oleg-andreyevoleg-andreyev commentedApr 19, 2019
edited
Loading

Updated "Authentication Success and Failure Events"

security.authentication.success can be dispatched in the following cases:

  • ifalways_authenticate_before_granting is enabled andisGranted is called
  • if a token is not authenticated beforeAccessListener is invoked
  • if customer submitted credentials (actual authentication)

symfony/symfony#21571

@OskarStark
Copy link
Contributor

Please target4.4 branch here, too. Thank you 🙏

@oleg-andreyevoleg-andreyev changed the base branch frommaster to4.4June 11, 2019 17:36
@oleg-andreyev
Copy link
ContributorAuthor

Please target4.4 branch here, too. Thank you 🙏

Changed

OskarStark reacted with thumbs up emoji

javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull requestJun 14, 2019
…ment branch instead of master for new features. (oleg-andreyev)This PR was merged into the 3.4 branch.Discussion----------Adding "special case" when need to select development branch instead of master for new features.As per discussion insymfony#11457 andsymfony/symfony#31177 (comment) updated section on how to select branchCommits-------1c2bae2 added "special case"
@wouterjwouterj removed the Waiting Code MergeDocs for features pending to be merged labelAug 16, 2019
@terjebraten-certua
Copy link
Contributor

Will this be merged soon?

It should be since@chalasr closedsymfony/symfony#21571

Copy link
Contributor

@terjebraten-certuaterjebraten-certua left a comment

Choose a reason for hiding this comment

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

Looks ok to me.

Copy link
Contributor

@OskarStarkOskarStark left a comment

Choose a reason for hiding this comment

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

@chalasr could you give us a Review ? Thanks

@TerjeBr
Copy link
Contributor

Any progress on this?

@TerjeBr
Copy link
Contributor

Would it not be better to just merge this, and then accept a new PR if something in it is wrong?

As it is now, the documentation is misleading.

fabpot added a commit to symfony/symfony that referenced this pull requestSep 6, 2019
…ged (oleg-andreyev)This PR was merged into the 4.4 branch.Discussion----------#21571 Comparing roles to detected that users has changed| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | yes| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| License       | MIT| Fixed tickets |#21571 (comment)| Docs |symfony/symfony-docs#11457**Case 1:**User A has roles `foo, bar and admin`, User A is signed-in into application and token is persisted, later another User B with role `admin`, decided to restrict role `admin` for User A, so User A won't lose it's privileges until session is expired or logout, because token is persisted with `roles` and `authenticated=true` and roles are not compared.Ref. to the previous attempt:#27121Commits-------4f4c30d - updated AbstractToken to compare Roles - Updated isEqualTo method to match roles as default User implements EquatableInterface - added test case - bumped symfony/security-core to 4.4
@chalasr
Copy link
Member

@TerjeBr This aims to fix something wrong, let's not replace it with something equally wrong :)
Also this is going deep in internal details (thebefore AccessListener is called sentence especially), I need to take the time to try it out and confirm that it's all right (if someone else can do it before me I would be more than happy).
I'm also wondering what's the point of having bothInteractiveLoginEvent andAuthenticationSuccessEvent on the code side since, reading the docs after this PR, both seem to handle the same very use case. I'd like to figure out what's the difference as I'm pretty sure there is one.

@terjebraten-certua
Copy link
Contributor

@chalasr I only suggested it because there seemed to be no progress (two weeks had gone by with no activity).
And now almost a month has gone by since@OskarStark asked for a review.

But I am glad to hear that you are working on it. Always nice to have some feedback se we know the issue has not just been forgotten. I hope this piece of documentation can be fixed soon.

javiereguiluz added a commit that referenced this pull requestSep 24, 2019
…s" section (oleg-andreyev)This PR was merged into the 4.4 branch.Discussion----------#21571 updated "Authentication Success and Failure Events" sectionUpdated "Authentication Success and Failure Events"`security.authentication.success` can be dispatched in the following cases:- if `always_authenticate_before_granting` is enabled and `isGranted` is called- if a token is not authenticated before `AccessListener` is invoked- if customer submitted credentials (actual authentication)symfony/symfony#21571Commits-------dc91bfd#21571 updated "Authentication Success and Failure Events" section
@javiereguiluzjaviereguiluz merged commitdc91bfd intosymfony:4.4Sep 24, 2019
@javiereguiluz
Copy link
Member

@oleg-andreyev thanks for this contribution and we're sorry it took us so long to merge.

@chalasr I assumed this was OK "as is" because we didn't receive more comments from the community, but we can create new PRs to tweak or fix minor issues. Thanks.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@OskarStarkOskarStarkOskarStark left review comments

+1 more reviewer

@terjebraten-certuaterjebraten-certuaterjebraten-certua approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

8 participants

@oleg-andreyev@OskarStark@terjebraten-certua@TerjeBr@chalasr@javiereguiluz@wouterj@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp