Skip to content

Commit

Permalink
fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
CatalinVoineag committed Jan 2, 2025
1 parent 46f139e commit b2e2674
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 42 deletions.
9 changes: 0 additions & 9 deletions app/controllers/concerns/authentication.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
module Authentication
extend ActiveSupport::Concern

# can we reformat the one login private key as the google private key?

included do
before_action :require_authentication, if: -> { one_login_enabled? }
helper_method :authenticated?
Expand All @@ -22,11 +20,6 @@ def authenticated?

def require_authentication
current_candidate || resume_session || request_authentication
#current_candidate is here to make the one login feature flag activation seamless
# So if a candidate is loged in with magic link, they will still be logged in after switching the one login on.
# The reverse is not true, if we switch off one login feature flag, we will log out the current candidates
# It's also here for impersonation. Current_candidate will be used for impersonation.
# So db backed session is used for candidates but support users impersonation will still be using cookies/session
end

def resume_session
Expand All @@ -38,9 +31,7 @@ def find_session_by_cookie
end

def request_authentication
#redirect_to auth_one_login_sign_out_path
redirect_to candidate_interface_create_account_or_sign_in_path
## Does this need to go to one login sign_out or candidate_sign_in?
end

def start_new_session_for(candidate:, id_token_hint: nil)
Expand Down
17 changes: 3 additions & 14 deletions app/controllers/one_login_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,7 @@ class OneLoginController < ApplicationController
include Authentication

before_action :redirect_to_candidate_sign_in_unless_one_login_enabled
allow_unauthenticated_access# only: %i[callback bypass_callback]
# We need to allow requests to these endpoints without requiering authentication to be able to login the candidate and also log out the candidate by just calling these endpoints.
# There are cases where the user is logged in in one admin but not in our system because of an error in our system, so we need to be able to call sign-out endpoint to log them out of one login

## after sign in candidate contoller needs redirect if one login?
allow_unauthenticated_access

def callback
auth = request.env['omniauth.auth']
Expand All @@ -20,7 +16,6 @@ def callback

redirect_to candidate_interface_interstitial_path
rescue StandardError => e
# We can't attach an error message to the session because in some cases we don't create a session if there's an error
session_error = SessionError.create!(
candidate: OneLoginUser.find_candidate(auth),
id_token_hint:,
Expand All @@ -31,7 +26,7 @@ def callback
if e.is_a?(OneLoginUser::Error)
session[:session_error_id] = session_error.id
Sentry.capture_message(
"#{session_error.body} - check session_error record #{session_error.id}",
"One login session error, check session_error record #{session_error.id}",
level: :error,
)

Expand Down Expand Up @@ -67,9 +62,6 @@ def sign_out

terminate_session

## how do you sign out when users are logged in and we switch off the
## one login feature flag

session[:session_error_id] = session_error.id if session_error.present?
if OneLogin.bypass? || id_token_hint.nil?
redirect_to candidate_interface_create_account_or_sign_in_path
Expand All @@ -90,13 +82,10 @@ def sign_out_complete

def failure
session_error = SessionError.create!(
candidate: OneLoginUser.find_candidate(auth),
id_token_hint:,
body: "One login failure with #{params[:message]}",
omniauth_hash: auth&.to_h,
)
Sentry.capture_message(
"#{session_error.body} - check session_error record #{session_error.id}",
"#{session_error.body}, check session_error record #{session_error.id}",
level: :error,
)

Expand Down
50 changes: 48 additions & 2 deletions config/brakeman.ignore
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,29 @@
],
"note": ""
},
{
"warning_type": "Redirect",
"warning_code": 18,
"fingerprint": "783f66b9e6119c7b65c794a57f29b291e256d49267c511bff375d031ef83ea96",
"check_name": "Redirect",
"message": "Possible unprotected redirect",
"file": "app/controllers/one_login_controller.rb",
"line": 71,
"link": "https://brakemanscanner.org/docs/warning_types/redirect/",
"code": "redirect_to(logout_one_login((Current.session.id_token_hint or SessionError.find_by(:id => session[:session_error_id]).id_token_hint)), :allow_other_host => true)",
"render_path": null,
"location": {
"type": "method",
"class": "OneLoginController",
"method": "sign_out"
},
"user_input": "Current.session.id_token_hint",
"confidence": "Weak",
"cwe_id": [
601
],
"note": ""
},
{
"warning_type": "SQL Injection",
"warning_code": 0,
Expand Down Expand Up @@ -276,6 +299,29 @@
],
"note": ""
},
{
"warning_type": "Unscoped Find",
"warning_code": 82,
"fingerprint": "bb99c47602bf4e5e8c658f10645d3d0af3f1ac7e78e590db5e66d8986ba1642a",
"check_name": "UnscopedFind",
"message": "Unscoped call to `Session#find_by`",
"file": "app/controllers/concerns/authentication.rb",
"line": 32,
"link": "https://brakemanscanner.org/docs/warning_types/unscoped_find/",
"code": "Session.find_by(:id => cookies.signed[:session_id])",
"render_path": null,
"location": {
"type": "method",
"class": "Authentication",
"method": "find_session_by_cookie"
},
"user_input": "cookies.signed[:session_id]",
"confidence": "Weak",
"cwe_id": [
285
],
"note": ""
},
{
"warning_type": "SQL Injection",
"warning_code": 0,
Expand Down Expand Up @@ -346,6 +392,6 @@
"note": ""
}
],
"updated": "2024-08-27 15:35:00 +0100",
"brakeman_version": "6.1.2"
"updated": "2025-01-02 12:10:42 +0000",
"brakeman_version": "6.2.2"
}
53 changes: 36 additions & 17 deletions spec/requests/one_login_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,24 +35,29 @@
context 'when there is no omniauth_hash' do
let(:omniauth_hash) { nil }

it 'returns unprocessable_entity' do
it 'redirects to internal_server_error' do
get auth_one_login_callback_path

expect(response).to have_http_status(:unprocessable_entity)
expect(response).to redirect_to internal_server_error_path
end
end

context 'when candidate has a different one login token than the one returned by one login' do
it 'redirects to auth_one_login_sign_out_path' do
candidate = create(:candidate, email_address: '[email protected]')
create(:one_login_auth, candidate:, token: '456')
allow(Sentry).to receive(:capture_message)

get auth_one_login_callback_path
expect {
get auth_one_login_callback_path
}.to change(SessionError, :count).by(1)

expect(response).to redirect_to(auth_one_login_sign_out_path)
expect(session[:one_login_error]).to eq(
"Candidate #{candidate.id} has a different one login token than the " \
'user trying to login. Token used to auth 123',
expect(session[:session_error_id]).to eq(SessionError.last.id)

expect(Sentry).to have_received(:capture_message).with(
"One login session error, check session_error record #{SessionError.last.id}",
level: :error,
)
end
end
Expand Down Expand Up @@ -132,16 +137,21 @@
it 'redirects to one_login logout url and persists the session error message' do
candidate = create(:candidate, email_address: '[email protected]')
create(:one_login_auth, candidate:, token: '456')
allow(Sentry).to receive(:capture_message)

expect {
get auth_one_login_callback_path # set the session variables
}.to change(SessionError, :count).by(1)

get auth_one_login_callback_path # set the session variables
get auth_one_login_sign_out_path

expect(session[:one_login_id_token]).to be_nil
expect(session[:one_login_error]).to eq(
"Candidate #{candidate.id} has a different one login token than the " \
'user trying to login. Token used to auth 123',
expect(Sentry).to have_received(:capture_message).with(
"One login session error, check session_error record #{SessionError.last.id}",
level: :error,
)

expect(session[:session_error_id]).to eq(SessionError.last.id)

params = {
post_logout_redirect_uri: URI(auth_one_login_sign_out_complete_url),
id_token_hint: 'id_token',
Expand Down Expand Up @@ -170,6 +180,7 @@
end
end
end
# 137

describe 'GET /auth/one-login/sign-out-complete' do
context 'when candidate has a different one login token than the one returned by one login' do
Expand All @@ -178,12 +189,14 @@
create(:one_login_auth, candidate:, token: '456')
allow(Sentry).to receive(:capture_message)

get auth_one_login_callback_path # set the session variables
expect {
get auth_one_login_callback_path # set the session variables
}.to change(SessionError, :count).by(1)

get auth_one_login_sign_out_complete_path

expect(Sentry).to have_received(:capture_message).with(
"Candidate #{candidate.id} has a different one login token than the " \
'user trying to login. Token used to auth 123',
"One login session error, check session_error record #{SessionError.last.id}",
level: :error,
)
expect(response).to redirect_to(internal_server_error_path)
Expand All @@ -203,12 +216,18 @@

describe 'GET /auth/one-login/failure' do
it 'redirects to auth_failure_path with one login error' do
allow(Sentry).to receive(:capture_message)
get auth_one_login_callback_path # set the session variables
get auth_failure_path(params: { message: 'error_message' })

expect(session[:one_login_error]).to eq(
'One login failure with error_message for one_login_id_token: id_token',
expect {
get auth_failure_path(params: { message: 'error_message' })
}.to change(SessionError, :count).by(1)

expect(Sentry).to have_received(:capture_message).with(
"One login failure with error_message, check session_error record #{SessionError.last.id}",
level: :error,
)
expect(session[:session_error_id]).to eq(SessionError.last.id)
expect(response).to redirect_to(auth_one_login_sign_out_path)
end
end
Expand Down

0 comments on commit b2e2674

Please sign in to comment.