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

IBX-2880: Added PasswordExpiredException #309

Merged
merged 4 commits into from
May 24, 2022

Conversation

mateuszbieniek
Copy link
Contributor

@mateuszbieniek mateuszbieniek commented May 17, 2022

Question Answer
JIRA issue IBX-2880
Type bug
Target Ibexa version v3.3
BC breaks no

CredentialsExpiredListener expects CredentialsExpiredException which will never happen, as the Exception is immediately replaced with BadCredentialsException in https://github.com/symfony/security-core/blob/5.4/Authentication/Provider/UserAuthenticationProvider.php#L90

This PR adds a new PasswordExpiredException exception which extends CustomUserMessageAccountStatusException which allows for it to be propagated to the top so CredentialsExpiredListener can act properly when it happens.

AdminUI part of PR: ezsystems/ezplatform-admin-ui#2045

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Checked that target branch is set correctly (master for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping @ezsystems/engineering-team).

@mateuszbieniek mateuszbieniek requested a review from a team May 17, 2022 12:57
@mateuszbieniek mateuszbieniek added Bug Something isn't working Ready for review labels May 17, 2022
@mikadamczyk mikadamczyk changed the title IBX-2880: Added PasswordExpredException IBX-2880: Added PasswordExpiredException May 17, 2022
@Steveb-p
Copy link
Contributor

To be frank I'm not convinced that we should introduce a dedicated exception for something that already has it's own exception in the framework. Developers will need to respect both after this patch is added, which adds to the complexity and knowledge required to properly handle our library.

Instead I would propose to check Exception::getPreviousException() type as a "fallback" measure, in the admin-ui package itself.

We also need to ensure that we are not exposing user account existence when credentials are expired. That's the purpose of the Symfony code. If we're always redirecting to the password refresh page, we might introduce a security issue.

@mateuszbieniek
Copy link
Contributor Author

Unfortunately, we cannot use previous on last authentication exception, as for this case it is always null - I guess when an exception is serialized and stored in session this data is lost.
Also, there is no security issue, as the exception is thrown at the "postAuth" stage - therefore, for expired password info to be presented, first, proper login and password have to be provided.

Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

Following private discussion we concluded that this is actually the proper solution.

@Steveb-p Steveb-p requested a review from a team May 19, 2022 10:11
Copy link
Member

@konradoboza konradoboza left a comment

Choose a reason for hiding this comment

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

+1 apart from @Steveb-p suggestions.

@konradoboza konradoboza requested a review from a team May 19, 2022 10:16
mateuszbieniek and others added 2 commits May 19, 2022 13:52
…n.php

Co-authored-by: Paweł Niedzielski <pawel.tadeusz.niedzielski@gmail.com>
…n.php

Co-authored-by: Paweł Niedzielski <pawel.tadeusz.niedzielski@gmail.com>
@sonarcloud
Copy link

sonarcloud bot commented May 19, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@bogusez bogusez added QA approved Ready for MERGE To be set by author or maintainer and removed Ready for QA labels May 24, 2022
@adamwojs adamwojs merged commit 67dc2b6 into ezsystems:1.3 May 24, 2022
@mateuszbieniek mateuszbieniek deleted the ibx-2880 branch May 24, 2022 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working QA approved Ready for MERGE To be set by author or maintainer
9 participants