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

[SD-375] bypass tfa during pass reset use prlp #532

Merged

Conversation

MdNadimHossain
Copy link
Contributor

Jira

https://digital-vic.atlassian.net/browse/SD-375
https://digital-vic.atlassian.net/browse/SD-421

Problem/Motivation

The password reset workflow in our SDP projects, managed by the PRLP module, is being deprioritized due to a route subscriber defined in the TFA module. This conflict is disrupting the current password reset process for SDP projects.

Fix

  1. Override the TFA method and introduce a new route subscriber so that the password reset workflow is correctly handled by the PRLP module.
  2. If the PRLP module is not enabled, fall back to the default password reset functionality provided by the TFA module.
  3. By default, disable TFA for users with the previewer and secure_file_user roles.

Related PRs

Screenshots

TODO

@MdNadimHossain MdNadimHossain force-pushed the feature/SD-375-bypass-tfa-during-pass-reset-use-prlp branch from e9e8c64 to 6a6736c Compare October 29, 2024 00:43
---------

Co-authored-by: Md Nadim Hossain <[email protected]>

---------

Co-authored-by: Md Nadim Hossain <[email protected]>
@MdNadimHossain MdNadimHossain force-pushed the feature/SD-375-bypass-tfa-during-pass-reset-use-prlp branch from 0ca100b to 9be573a Compare October 29, 2024 00:48
@anthony-malkoun
Copy link
Contributor

Why does this still have circleCI builds in it? Should the project be disconnected from Circle? Also, I don't think the failing GHA items ever worked?

Copy link
Contributor

@vincent-gao vincent-gao left a comment

Choose a reason for hiding this comment

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

hey @MdNadimHossain
thanks for the changes

  1. If possible, please use Dependency Injection to inject prlpController instead of directly instantiating it. This approach enhances modularity and testability.
  2. $request ? $request->getSession()->set('pass_reset_' . $uid, $token) : $_SESSION['pass_reset_' . $uid] = $token; can be simplified to $request->getSession()->set('pass_reset_' . $uid, $token);. This change aligns with Drupal’s session handling practices, ensuring session consistency and better compatibility with symfony's request stack
  3. If convenient, please replace $request = \Drupal::request() with $this->requestStack->getCurrentRequest(). Using the injected RequestStack service follows Drupal’s dependency injection practices and improves maintainability

@vincent-gao
Copy link
Contributor

My comments don’t prevent this PR from being moved to testing.

@MdNadimHossain
Copy link
Contributor Author

  • $request ? $request->getSession()->set('pass_reset_' . $uid, $token) : $_SESSION['pass_reset_' . $uid] = $token; can be simplified to $request->getSession()->set('pass_reset_' . $uid, $token);. This change aligns with Drupal’s session handling practices, ensuring session consistency and better compatibility with symfony's request stack
  • If convenient, please replace $request = \Drupal::request() with $this->requestStack->getCurrentRequest(). Using the injected RequestStack service follows Drupal’s dependency injection practices and improves maintainability

hi @vincent-gao
yup agree, dependency injection would have been the right approach. The problem here is, prlp module does not have any service defined for the PrlpController and that is why I have to instantiate it here. I could have added a service in the tfa custom module referring to the PrlpController class, but tfa really shouldn't have a dependency on that module, so avoiding adding unnecessary dependency.

I have added the other two changes you have requested. Please have a look now. thanks :)

@MdNadimHossain
Copy link
Contributor Author

MdNadimHossain commented Oct 30, 2024

Why does this still have circleCI builds in it? Should the project be disconnected from Circle? Also, I don't think the failing GHA items ever worked?

@anthony-malkoun yes and needs to be cleaned up and also from the submodules. I will do it in a separate PR.

@MdNadimHossain MdNadimHossain force-pushed the feature/SD-375-bypass-tfa-during-pass-reset-use-prlp branch from a9f0203 to 28dcc4a Compare October 30, 2024 01:51
Copy link
Contributor

@vincent-gao vincent-gao left a comment

Choose a reason for hiding this comment

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

lint! @MdNadimHossain 😃

Copy link
Contributor

@anthony-malkoun anthony-malkoun left a comment

Choose a reason for hiding this comment

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

This looks fine to me. Thanks for cleaning up the other stuff.

Copy link
Contributor

@sharmasahil sharmasahil left a comment

Choose a reason for hiding this comment

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

Review changes approved.

@MdNadimHossain MdNadimHossain force-pushed the feature/SD-375-bypass-tfa-during-pass-reset-use-prlp branch from 28dcc4a to cda6f10 Compare October 30, 2024 02:09
@MdNadimHossain MdNadimHossain merged commit 67eab3a into develop Nov 14, 2024
1 check passed
@MdNadimHossain MdNadimHossain deleted the feature/SD-375-bypass-tfa-during-pass-reset-use-prlp branch November 14, 2024 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants