Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 1 addition & 16 deletions .reek
Original file line number Diff line number Diff line change
Expand Up @@ -95,22 +95,7 @@ UtilityFunction:
exclude:
- complete_idv_session
DuplicateMethodCall:
exclude:
- complete_2fa_confirmation
- complete_idv_profile
- expect_current_address_in_profile
- stub_subject
- stub_idv_session
- saml_settings
- sign_up_and_2fa
- raw_xml_response
- rotate_attribute_encryption_key
- rotate_attribute_encryption_key_with_invalid_queue
- rotate_hmac_key
- sign_in_user
- stub_auth
- stub_sign_in
- stub_verify_steps_one_and_two
enabled: false
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.

hallelujah! 🎉

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.

woooooo! Care to comment on why this now?

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.

Because I was getting a bunch of Reek errors locally, and instead of adding a bunch of methods to the exclusion list, I figured we'd turn it off since we'll probably keep getting more of these in the future.

FeatureEnvy:
enabled: false
NestedIterators:
Expand Down
6 changes: 5 additions & 1 deletion app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,11 @@ def decorated_user
end

def after_sign_in_path_for(user)
stored_location_for(user) || sp_session[:request_url] || profile_path
stored_location_for(user) || sp_session[:request_url] || signed_in_path
end

def signed_in_path
user_fully_authenticated? ? profile_path : user_two_factor_authentication_path
end

def render_401
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/sign_up/completions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def update
Analytics::USER_REGISTRATION_AGENCY_HANDOFF_COMPLETE,
service_provider_attributes
)
redirect_to after_sign_in_path_for(current_user)
redirect_to sp_session[:request_url]
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.

would someone ever get to this point without being referred by an SP?

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.

No. The show action requires session[:sp] to be present.

end

private
Expand Down
24 changes: 17 additions & 7 deletions app/controllers/users/sessions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,11 @@ def new
def create
track_authentication_attempt(params[:user][:email])

if current_user && user_locked_out?(current_user)
render 'two_factor_authentication/shared/max_login_attempts_reached'
sign_out
return
end
return process_locked_out_user if current_user && user_locked_out?(current_user)

super
cache_active_profile
store_sp_metadata_in_session unless request_id.empty?
end

def active
Expand Down Expand Up @@ -53,6 +50,11 @@ def check_user_needs_redirect
end
end

def process_locked_out_user
render 'two_factor_authentication/shared/max_login_attempts_reached'
sign_out
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.

do you think we should sign out then render? Not sure if it matters, just wondering if the page might render different content based on signed in / out status.

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.

Not sure. I only refactored this to please Code Climate. I didn't change the behavior.

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.

However, I believe this order is like that for a reason. I recall this fixed a bug at one time.

end

def now
@_now ||= Time.zone.now
end
Expand Down Expand Up @@ -91,13 +93,21 @@ def cache_active_profile
end

def user_signed_in_and_not_locked_out?(user)
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.

do we need the user to get passed in here? We should have access to current_user within this method, right?

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.

I didn't change this behavior. This method existed before this PR. Can we revisit later if necessary?

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.

sure thing!

return false unless current_user.present?

return false unless current_user
!user_locked_out?(user)
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.

based on the name of this method, I'd expect this to read current_user && !user_locked_out?(user)

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.

This is not new to this PR. I didn't want to make any changes that were not related. I can add the .present? back if that makes things clearer.

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.

that's fine, we can keep as-is

end

def user_locked_out?(user)
UserDecorator.new(user).blocked_from_entering_2fa_code?
end

def store_sp_metadata_in_session
return if sp_session[:issuer]
StoreSpMetadataInSession.new(session: session, request_id: request_id).call
end

def request_id
params[:user].fetch(:request_id, '')
end
end
end
27 changes: 27 additions & 0 deletions app/services/store_sp_metadata_in_session.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
class StoreSpMetadataInSession
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.

having the name of a class be a verb seems like an odd pattern to introduce.

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.

This is not the first introduction of this pattern. See CreateVerifiedAccountEvent, RequestPasswordReset, UpdateUser, and UpdateUserPassword.

Micropurchase does this as well.

I think it's fine to have some classes be verbs and others nouns, depending on the best way to describe them.

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.

I don't mind verb names in service class methods. I do feel like some of the service classes in this app could perhaps be classified as something else...eg RandomPhrase...but that's for a separate PR :)

def initialize(session:, request_id:)
@session = session
@request_id = request_id
end

def call
session[:sp] = {
issuer: sp_request.issuer,
loa3: loa3_requested?,
request_url: sp_request.url,
request_id: sp_request.uuid,
}
end

private

attr_reader :session, :request_id

def sp_request
@sp_request ||= ServiceProviderRequest.find_by(uuid: request_id)
end

def loa3_requested?
sp_request.loa == Saml::Idp::Constants::LOA3_AUTHN_CONTEXT_CLASSREF
end
end
1 change: 1 addition & 0 deletions app/views/devise/sessions/new.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ h1.h3.my0 = decorated_session.new_session_heading
html: { autocomplete: 'off', role: 'form' }) do |f|
= f.input :email, required: true, input_html: { class: 'mb4' }
= f.input :password, required: true
= f.input :request_id, as: :hidden, input_html: { value: params[:request_id] }
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.

params[:_request_id] ?

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.

The underscore is only present in the confirmation email.

= f.button :submit, t('links.next'), class: 'btn-wide'

- link = link_to t('notices.sign_in_consent.link'), MarketingSite.privacy_url, target: '_blank'
Expand Down
10 changes: 8 additions & 2 deletions spec/controllers/sign_up/completions_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,10 @@
context 'LOA1' do
it 'tracks analytics' do
stub_sign_in
subject.session[:sp] = { loa3: false }
subject.session[:sp] = {
loa3: false,
request_url: 'http://example.com',
}

patch :update

Expand All @@ -78,7 +81,10 @@
it 'tracks analytics' do
user = create(:user, profiles: [create(:profile, :verified, :active)])
stub_sign_in(user)
subject.session[:sp] = { loa3: true }
subject.session[:sp] = {
loa3: true,
request_url: 'http://example.com',
}

patch :update

Expand Down
16 changes: 16 additions & 0 deletions spec/features/saml/loa1_sso_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,22 @@

expect(current_path).to eq sign_up_completed_path
end

it 'after session timeout, signing in takes user back to SP' do
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.

👍

allow(FeatureManagement).to receive(:prefill_otp_codes?).and_return(true)

user = create(:user, :signed_up)
saml_authn_request = auth_request.create(saml_settings)

visit saml_authn_request
sp_request_id = ServiceProviderRequest.last.uuid
page.set_rack_session(sp: {})
visit new_user_session_url(request_id: sp_request_id)
fill_in_credentials_and_submit(user.email, user.password)
click_submit_default

expect(current_url).to eq saml_authn_request
end
end

context 'fully signed up user is signed in with email and password only' do
Expand Down
2 changes: 2 additions & 0 deletions spec/support/controller_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ def sign_in_before_2fa(user = create(:user, :signed_up))
sign_in_as_user(user)
controller.current_user.send_new_otp
allow(controller).to receive(:user_fully_authenticated?).and_return(false)
allow(controller).to receive(:signed_in_path).and_return(profile_path)
end

def stub_sign_in(user = build(:user, password: VALID_PASSWORD))
Expand All @@ -42,6 +43,7 @@ def stub_sign_in_before_2fa(user = build(:user, password: VALID_PASSWORD))
allow(request.env['warden']).to receive(:session).and_return(user: {})
allow(controller).to receive(:current_user).and_return(user)
allow(controller).to receive(:user_fully_authenticated?).and_return(false)
allow(controller).to receive(:signed_in_path).and_return(profile_path)
end

def stub_verify_steps_one_and_two(user)
Expand Down