Skip to content

Move user suspended check for sign-in #9147

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.

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

Merged
zachmargolis merged 5 commits into from
Sep 6, 2023
Merged

Move user suspended check for sign-in #9147

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
16e0c02
Move user suspended check for sign-in
zachmargolis Sep 5, 2023
6bebfb5
Finish removing before_filter
zachmargolis Sep 5, 2023
b8492e0
Really remove the filter
zachmargolis Sep 5, 2023
dfe253b
Bring back before_filter in accounts controller
zachmargolis Sep 6, 2023
cd4e889
Merge remote-tracking branch 'origin/main' into margolis-account-susp…
zachmargolis Sep 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions app/controllers/application_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ def fix_broken_personal_key_url

def after_sign_in_path_for(_user)
accept_rules_of_use_url ||
user_suspended_url ||
service_provider_mfa_setup_url ||
add_piv_cac_setup_url ||
fix_broken_personal_key_url ||
Expand All @@ -228,7 +229,6 @@ def after_sign_in_path_for(_user)

def signed_in_url
return user_two_factor_authentication_url unless user_fully_authenticated?
return user_please_call_url if current_user.suspended?
return reactivate_account_url if user_needs_to_reactivate_account?
return url_for_pending_profile_reason if user_has_pending_profile?
return backup_code_reminder_url if user_needs_backup_code_reminder?
Expand Down Expand Up @@ -291,10 +291,6 @@ def user_fully_authenticated?
two_factor_enabled?
end

def confirm_user_is_not_suspended

@mitchellhenke mitchellhenke Sep 5, 2023

Copy link
Copy Markdown
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?

@zachmargolis zachmargolis Sep 5, 2023

Copy link
Copy Markdown
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 😬

@zachmargolis zachmargolis Sep 5, 2023

Copy link
Copy Markdown
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

@zachmargolis zachmargolis Sep 6, 2023

Copy link
Copy Markdown
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

@aduth aduth Sep 6, 2023

Copy link
Copy Markdown
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.

@zachmargolis zachmargolis Sep 6, 2023

Copy link
Copy Markdown
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

redirect_to user_please_call_url if current_user.suspended?
end

def confirm_two_factor_authenticated
authenticate_user!(force: true)

Expand Down Expand Up @@ -350,6 +346,10 @@ def prompt_to_verify_sp_required_mfa
redirect_to sp_required_mfa_verification_url
end

def user_suspended_url
user_please_call_url if current_user.suspended?
end

def sp_required_mfa_verification_url
return login_two_factor_piv_cac_url if service_provider_mfa_policy.piv_cac_required?

Expand Down
15 changes: 15 additions & 0 deletions spec/features/users/sign_in_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,21 @@
expect(current_path).to eq account_path
end

scenario 'user is suspended, gets show please call page after 2fa' do
user = create(:user, :fully_registered, :suspended)
service_provider = ServiceProvider.find_by(issuer: OidcAuthHelper::OIDC_IAL1_ISSUER)
IdentityLinker.new(user, service_provider).link_identity(
verified_attributes: %w[openid email],
)

visit_idp_from_sp_with_ial1(:oidc)
fill_in_credentials_and_submit(user.email, user.password)
fill_in_code_with_last_phone_otp
click_submit_default

expect(current_path).to eq(user_please_call_path)
end

scenario 'user opts to add piv/cac card' do
perform_steps_to_get_to_add_piv_cac_during_sign_up
nonce = piv_cac_nonce_from_form_action
Expand Down