Skip to content

Move user suspended check for sign-in#9147

Merged
zachmargolis merged 5 commits intomainfrom
margolis-account-suspension-redirect
Sep 6, 2023
Merged

Move user suspended check for sign-in#9147
zachmargolis merged 5 commits intomainfrom
margolis-account-suspension-redirect

Conversation

@zachmargolis
Copy link
Contributor

Background

I had a chat with @aduth about the suspension we implemented in #8755, and we think that the code would catch users signing directly in, not through an SP

What's Changed

  • Moves logic around in application_controller.rb, net result should be about the same
  • Adds an acceptance spec for suspended users (there were none as far as I could tell)
    • check specifically for behavior for signing in from an SP

- This path is more in-line with other post-2fa steps for sign in

changelog: Internal, User suspension, Update suspended user check
@zachmargolis zachmargolis requested review from a team and aduth September 5, 2023 20:50
two_factor_enabled?
end

def confirm_user_is_not_suspended
Copy link
Contributor

Choose a reason for hiding this comment

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

is this method still used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh whoops it still is as a before filter in a few spots 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed in 6bebfb5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok so after chatting with @aduth we think it's good to have this filter too, I brought it back in dfe253b

Copy link
Contributor

Choose a reason for hiding this comment

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

It could probably live in AccountsController instead of ApplicationController? Since it's only used once in that controller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes good call

@zachmargolis zachmargolis merged commit ef33b3c into main Sep 6, 2023
@zachmargolis zachmargolis deleted the margolis-account-suspension-redirect branch September 6, 2023 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants