Skip to content
Permalink
Browse files Browse the repository at this point in the history
SECURITY: Add confirmation screen when logging in via user-api OTP
  • Loading branch information
davidtaylorhq committed Jun 17, 2019
1 parent 52387be commit e6e47f2
Show file tree
Hide file tree
Showing 5 changed files with 54 additions and 5 deletions.
15 changes: 11 additions & 4 deletions app/controllers/session_controller.rb
Expand Up @@ -365,12 +365,19 @@ def email_login
end

def one_time_password
otp_username = $redis.get "otp_#{params[:token]}"
@otp_username = otp_username = $redis.get "otp_#{params[:token]}"

if otp_username && user = User.find_by_username(otp_username)
log_on_user(user)
$redis.del "otp_#{params[:token]}"
return redirect_to path("/")
if current_user&.username == otp_username
$redis.del "otp_#{params[:token]}"
return redirect_to path("/")
elsif request.post?
log_on_user(user)
$redis.del "otp_#{params[:token]}"
return redirect_to path("/")
else
# Display the form
end
else
@error = I18n.t('user_api_key.invalid_token')
end
Expand Down
6 changes: 6 additions & 0 deletions app/views/session/one_time_password.html.erb
Expand Up @@ -2,4 +2,10 @@
<div class='alert alert-error'>
<%= @error %>
</div>
<%else%>
<%= form_tag do%>
<h2><%= t("user_api_key.otp_confirmation.confirm_title", site_name: SiteSetting.title) %></h2>
<p><%= t("user_api_key.otp_confirmation.logging_in_as", username: @otp_username) %></p>
<%= submit_tag(t("user_api_key.otp_confirmation.confirm_button"), class: "btn btn-primary") %>
<%end%>
<%end%>
4 changes: 4 additions & 0 deletions config/locales/server.en.yml
Expand Up @@ -939,6 +939,10 @@ en:
description: '"%{application_name}" is requesting the following access to your account:'
instructions: 'We just generated a new user API key for you to use with "%{application_name}", please paste the following key into your application:'
otp_description: 'Would you like to allow "%{application_name}" to access this site?'
otp_confirmation:
confirm_title: Continue to %{site_name}
logging_in_as: Logging in as %{username}
confirm_button: Finish Login
no_trust_level: "Sorry, you do not have the required trust level to access the user API"
generic_error: "Sorry, we are unable to issue user API keys, this feature may be disabled by the site admin"
scopes:
Expand Down
1 change: 1 addition & 0 deletions config/routes.rb
Expand Up @@ -340,6 +340,7 @@
get "session/email-login/:token" => "session#email_login_info"
post "session/email-login/:token" => "session#email_login"
get "session/otp/:token" => "session#one_time_password", constraints: { token: /[0-9a-f]+/ }
post "session/otp/:token" => "session#one_time_password", constraints: { token: /[0-9a-f]+/ }
get "composer_messages" => "composer_messages#index"
post "composer/parse_html" => "composer#parse_html"

Expand Down
33 changes: 32 additions & 1 deletion spec/requests/session_controller_spec.rb
Expand Up @@ -1369,9 +1369,40 @@ def post_login
get "/session/otp/asd1231dasd123"

expect(response.status).to eq(404)

post "/session/otp/asd1231dasd123"

expect(response.status).to eq(404)
end

context 'when token is valid' do
it "should display the form for GET" do
token = SecureRandom.hex
$redis.setex "otp_#{token}", 10.minutes, user.username

get "/session/otp/#{token}"

expect(response.status).to eq(200)
expect(response.body).to include(
I18n.t("user_api_key.otp_confirmation.logging_in_as", username: user.username)
)
expect($redis.get("otp_#{token}")).to eq(user.username)

expect(session[:current_user_id]).to eq(nil)
end

it "should redirect on GET if already logged in" do
sign_in(user)
token = SecureRandom.hex
$redis.setex "otp_#{token}", 10.minutes, user.username

get "/session/otp/#{token}"
expect(response.status).to eq(302)

expect($redis.get("otp_#{token}")).to eq(nil)
expect(session[:current_user_id]).to eq(user.id)
end

it 'should authenticate user and delete token' do
user = Fabricate(:user)

Expand All @@ -1381,7 +1412,7 @@ def post_login
token = SecureRandom.hex
$redis.setex "otp_#{token}", 10.minutes, user.username

get "/session/otp/#{token}"
post "/session/otp/#{token}"

expect(response.status).to eq(302)
expect(response).to redirect_to("/")
Expand Down

0 comments on commit e6e47f2

Please sign in to comment.