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
33 changes: 28 additions & 5 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
class ApplicationController < ActionController::Base
class ApplicationController < ActionController::Base # rubocop:disable Metrics/ClassLength
include BrandedExperience
include UserSessionContext

Expand Down Expand Up @@ -47,10 +47,33 @@ def create_user_event(event_type, user = current_user)
private

def redirect_on_timeout
request_params = request.query_parameters
return unless request_params[:timeout]
flash[:timeout] = t('notices.session_cleared')
redirect_to url_for(request_params.except(:timeout))
params = request.query_parameters
return unless params[:timeout]

params[:issuer].present? ? redirect_with_sp : redirect_without_sp
end

def sp_metadata
ServiceProvider.from_issuer(request.query_parameters[:issuer]).metadata
end

def sp_name
sp_metadata[:friendly_name] || sp_metadata[:agency]
end

def redirect_with_sp # rubocop:disable Metrics/AbcSize
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 personally am happy to disable rubocop everywhere, but I know that @monfresh and @pkarman sometimes try to appease it rather than disable. Any strong opinions here?

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 personally don't think there's ever a good reason to disable AbcSize. It's a sign the method is too complex. A method can always be refactored to eliminate the offense. I'd be happy to take a look to see how we can improve this code.

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.

Yeah we have the max size for a method set to 15 in rubocop and this particular method was at 15.07... couldn't think of a way to restructure it any further but definitely open to suggestions.

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'm fine with this for now. I'll refactor after this gets merged. There are a lot of things that need to be cleaned up now that we are storing Service Providers in the database.

flash[:timeout] = t(
'notices.session_cleared_with_sp',
link: view_context.link_to(sp_name, sp_metadata[:return_to_sp_url]),
minutes: Figaro.env.session_timeout_in_minutes,
sp: sp_name
)
redirect_to url_for(request.query_parameters.except(:timeout))
end

def redirect_without_sp
flash[:timeout] = t('notices.session_cleared', minutes: Figaro.env.session_timeout_in_minutes)
redirect_to url_for(request.query_parameters.except(:issuer, :timeout))
end

def decorated_user
Expand Down
3 changes: 2 additions & 1 deletion app/controllers/concerns/saml_idp_auth_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,10 @@ def validate_service_provider_and_authn_context
render nothing: true, status: :unauthorized
end

def add_sp_metadata_to_session
def add_sp_metadata_to_session # rubocop:disable Metrics/AbcSize
session[:sp] = { loa3: loa3_requested?,
logo: current_sp_metadata[:logo],
issuer: saml_request.service_provider.identifier,
return_url: current_sp_metadata[:return_to_sp_url],
name: current_sp_metadata[:friendly_name] ||
current_sp_metadata[:agency],
Expand Down
1 change: 1 addition & 0 deletions app/controllers/openid_connect/authorization_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ def add_sp_metadata_to_session
current_sp_metadata = @authorize_form.service_provider.metadata
session[:sp] = { loa3: @authorize_form.loa3_requested?,
logo: current_sp_metadata[:logo],
issuer: @authorize_form.client_id,
name: current_sp_metadata[:friendly_name] ||
current_sp_metadata[:agency],
show_start_page: true }
Expand Down
7 changes: 6 additions & 1 deletion app/helpers/session_timeout_warning_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,15 @@ def warning
def timeout_refresh_url
URIService.add_params(
request.original_url,
timeout: true
timeout: true,
issuer: request.query_parameters[:issuer] || sp_issuer
).html_safe # rubocop:disable Rails/OutputSafety
end

def sp_issuer
sp_session[:issuer] if sp_session.present?
end

def auto_session_timeout_js
nonced_javascript_tag do
render partial: 'session_timeout/ping',
Expand Down
7 changes: 6 additions & 1 deletion config/locales/notices/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,12 @@ en:
message_html: >
For your security, in %{time_left_in_session} we will automatically
cancel your sign in.
session_cleared: We cleared your information for your security.
session_cleared: >
For your security, we refresh the page and clear out any information you typed into the
form fields if you don't submit the form within %{minutes} minutes.
session_cleared_with_sp: >
For your security, your request from %{sp} expires after %{minutes} minutes if you don't
submit the form. Then you'll have to start again from %{link}.
sign_in_consent:
link: Security Consent & Privacy Act Statement.
text: By signing in, you agree to %{app}’s %{link}
Expand Down
1 change: 1 addition & 0 deletions config/locales/notices/es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ es:
sign_out: NOT TRANSLATED YET
message_html: NOT TRANSLATED YET
session_cleared: NOT TRANSLATED YET
session_cleared_with_sp: NOT TRANSLATED YET
sign_in_consent:
link: NOT TRANSLATED YET
text: NOT TRANSLATED YET
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@
expect(session[:sp]).to eq(
loa3: false,
logo: 'generic.svg',
issuer: 'urn:gov:gsa:openidconnect:test',
name: 'Example iOS App',
show_start_page: true
)
Expand Down
1 change: 1 addition & 0 deletions spec/controllers/saml_idp_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,7 @@
expect(session[:sp]).to eq(
loa3: false,
logo: 'generic.svg',
issuer: 'http://localhost:3000',
return_url: 'http://localhost:3000',
name: 'Your friendly Government Agency',
request_url: @saml_request.request.original_url,
Expand Down
12 changes: 9 additions & 3 deletions spec/features/users/sign_in_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,9 @@
visit sign_up_email_path(foo: 'bar')
fill_in 'Email', with: 'test@example.com'

expect(page).to have_content(t('notices.session_cleared'))
expect(page).to have_content(
t('notices.session_cleared', minutes: Figaro.env.session_timeout_in_minutes)
)
expect(page).to have_field('Email', with: '')
expect(current_url).to match Regexp.escape(sign_up_email_path(foo: 'bar'))
end
Expand All @@ -124,7 +126,9 @@
allow(Devise).to receive(:timeout_in).and_return(60)

visit root_path
expect(page).to_not have_content(t('notices.session_cleared'))
expect(page).to_not have_content(
t('notices.session_cleared', minutes: Figaro.env.session_timeout_in_minutes)
)
end
end

Expand Down Expand Up @@ -159,7 +163,9 @@
fill_in 'Email', with: user.email
fill_in 'Password', with: user.password

expect(page).to have_content(t('notices.session_cleared'))
expect(page).to have_content(
t('notices.session_cleared', minutes: Figaro.env.session_timeout_in_minutes)
)
expect(find_field('Email').value).to be_blank
expect(find_field('Password').value).to be_blank
end
Expand Down
27 changes: 20 additions & 7 deletions spec/helpers/session_timeout_warning_helper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,29 +20,42 @@ def time_between_warning_and_timeout
end

describe '#timeout_refresh_url' do
before { expect(helper).to receive(:request).and_return(double(original_url: original_url)) }
before do
allow(helper).to receive(:request).and_return(
double(original_url: original_url, query_parameters: query_parameters)
)
end

let(:query_parameters) { { issuer: 'http://localhost:3000' } }

context 'with no query in the request url' do
let(:original_url) { 'http://test.host/foo/bar' }

it 'adds timeout=true params' do
expect(helper.timeout_refresh_url).to eq('http://test.host/foo/bar?timeout=true')
it 'adds timeout=true and issuer=http%3A%2F%2Flocalhost%3A3000 params' do
expect(helper.timeout_refresh_url).to eq(
'http://test.host/foo/bar?issuer=http%3A%2F%2Flocalhost%3A3000&timeout=true'
)
end
end

context 'with params request url' do
let(:original_url) { 'http://test.host/foo/bar?key=value' }

it 'adds timeout=true param' do
expect(helper.timeout_refresh_url).to eq('http://test.host/foo/bar?key=value&timeout=true')
expect(helper.timeout_refresh_url).to eq(
'http://test.host/foo/bar?issuer=http%3A%2F%2Flocalhost%3A3000&key=value&timeout=true'
)
end
end

context 'with timeout=true in the query params already' do
let(:original_url) { 'http://test.host/foo/bar?timeout=true' }
context 'with timeout=true and issuer=http%3A%2F%2Flocalhost%3A3000 \
in the query params already' do
let(:original_url) { 'http://test.host/foo/bar?timeout=true&issuer=http://localhost:3000' }

it 'is the same' do
expect(helper.timeout_refresh_url).to eq('http://test.host/foo/bar?timeout=true')
expect(helper.timeout_refresh_url).to eq(
'http://test.host/foo/bar?issuer=http%3A%2F%2Flocalhost%3A3000&timeout=true'
)
end
end
end
Expand Down