Skip to content
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.

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

Gracefully handle Google OAuth failures from Maker App #38245

Merged
merged 9 commits into from
Jan 23, 2021
15 changes: 11 additions & 4 deletions dashboard/app/controllers/maker_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -122,17 +122,24 @@ def login_code
# renders a page for users to copy and paste a login key
def display_code
# Generate encrypted code to display to user
user_auth = current_user.authentication_options.find_by_credential_type(AuthenticationOption::GOOGLE)
@secret_code = Encryption.encrypt_string_utf8(
Time.now.strftime('%Y%m%dT%H%M%S%z') + user_auth['authentication_id'] + user_auth['credential_type']
)
user_auth = current_user&.authentication_options&.find_by_credential_type(AuthenticationOption::GOOGLE)
if user_auth
JillianK marked this conversation as resolved.
Show resolved Hide resolved
@secret_code = Encryption.encrypt_string_utf8(
Time.now.strftime('%Y%m%dT%H%M%S%z') + user_auth.authentication_id + user_auth.credential_type
)
end
end

# GET /maker/confirm_login
# @param [String] params[:user_return_to] Where to redirect the user after they've logged in
# Renders a page to confirm uses want to login via Google OAuth
JillianK marked this conversation as resolved.
Show resolved Hide resolved
# This route is need to convert the GET request from the Maker App into
# a POST that can be used to login via Google OAuth
def confirm_login
session[:user_return_to] = params[:user_return_to]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we are handling the redirect here, I think we can remove this if statement (

) from the sessions controller. If I recall correctly, it was added so that we could piggy-back on the session management from that route. So now that we handle it here, I think that path is unused and can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can do that until after we publish the new maker app right? Since the current version of maker app still goes through there?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup! Thanks for catching that - I didn't think that through. Is there a way we can track that? It's a small change, but anytime we can minimize the number of branches of code we support, I'm excited.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, jillian and i were discussing this before break -- we could log to either Honeybadger (easily viewable but can cause alarm if it's triggered a lot) or Firehose (we aren't notified and have to remember to check Redshift) when this path is hit. then, if we don't log any usage for ~1 month (or whatever timeframe), we can safely remove that code

if current_user
JillianK marked this conversation as resolved.
Show resolved Hide resolved
redirect_to session[:user_return_to]
end
end

# POST /maker/complete
Expand Down
2 changes: 2 additions & 0 deletions dashboard/app/controllers/omniauth_callbacks_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ def google_oauth2

if user
sign_in_google_oauth2 user
elsif session[:user_return_to] == maker_display_google_oauth_code_path
redirect_to session[:user_return_to]
else
sign_up_google_oauth2
end
Expand Down
22 changes: 12 additions & 10 deletions dashboard/app/views/maker/display_code.html.haml
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
%h1= I18n.t('maker.google_oauth.login_with_google')
- if @secret_code
%p= I18n.t('maker.google_oauth.copy_code')

%p= I18n.t('maker.google_oauth.copy_code')
= text_field_tag :maker_code, @secret_code, readonly: true
= button_tag I18n.t(:copy), id: "btn-copy"

= text_field_tag :maker_code, @secret_code, readonly: true
= button_tag I18n.t(:copy), id: "btn-copy"

:javascript
$(document).ready(function() {
$('#btn-copy').click(function() {
$('#maker_code').select();
document.execCommand("copy");
:javascript
$(document).ready(function() {
$('#btn-copy').click(function() {
$('#maker_code').select();
document.execCommand("copy");
});
});
});
- else
%p= I18n.t('maker.google_oauth.error_no_linked_google')
JillianK marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions dashboard/config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1155,6 +1155,7 @@ en:
copy_code: 'Please copy and paste this code into the Maker App to login to Code.org.'
error_token_expired: 'Token has expired. Please log in again for a new token.'
error_wrong_provider: 'Wrong provider. Please make sure you are logging in with Google.'
error_no_linked_google: 'Please go back to the Maker App and use the "Sign in" button in the top right to log in or click the back button to log in with a different Google account'
error_invalid_user: 'Invalid user id. Please try logging in again.'
error_no_code: 'Please enter your login code.'
confirm_login: 'Select "Confirm" to log in with your Google account.'
Expand Down
48 changes: 48 additions & 0 deletions dashboard/test/controllers/maker_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,54 @@ class MakerControllerTest < ActionController::TestCase
assert_equal 1, CircuitPlaygroundDiscountApplication.where(user_id: @teacher.id).length
end

test "confirm_login redirects to maker_display_google_oauth_code_path when a google user is signed in" do
@student = create :student, :google_sso_provider
CDO.stubs(:properties_encryption_key).returns(SecureRandom.base64(Encryption::KEY_LENGTH / 8))
sign_in @student

get :confirm_login, params: {user_return_to: '/maker/display_google_oauth_code'}
assert_redirected_to '/maker/display_google_oauth_code'
end

test "confirm_login redirects to maker_display_google_oauth_code_path when a non google user is signed in" do
sign_in @student

get :confirm_login, params: {user_return_to: '/maker/display_google_oauth_code'}

assert_redirected_to '/maker/display_google_oauth_code'
end

test "confirm_login does not redirect when no user is signed in" do
get :confirm_login, params: {user_return_to: '/maker/display_google_oauth_code'}
assert_response :success
assert_template :confirm_login
end

test "display_code shows a code when a google user is signed in" do
@student = create :student, :google_sso_provider
CDO.stubs(:properties_encryption_key).returns(SecureRandom.base64(Encryption::KEY_LENGTH / 8))
sign_in @student
get :display_code
assert_response :success
assert_select '#btn-copy'
JillianK marked this conversation as resolved.
Show resolved Hide resolved
assert_select '#maker_code' do
assert_select ":match('value', ?)", /.+/
end
end

test "display_code does not show a code when a non google user is signed in" do
sign_in @student
get :display_code
assert_response :success
assert_select '#btn-copy', false
end

test "display_code does not show a code when no user is signed in" do
get :display_code
assert_response :success
assert_select '#btn-copy', false
end

private

def ensure_script(script_name, version_year, is_stable=true)
Expand Down
43 changes: 43 additions & 0 deletions dashboard/test/controllers/omniauth_callbacks_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,49 @@ class OmniauthCallbacksControllerTest < ActionController::TestCase
assert_equal auth[:credentials][:refresh_token], partial_user.oauth_refresh_token
end

test 'google_oauth2: redirects to maker display code if present as user_return_to on successful google login' do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I am having trouble parsing the title of this test. The comments in the tests are really helpful in understanding, but it would be great to see if we can make the title a little clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to clarify the title, let me know if that helped :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - thank you!

# Given I have a Google-Code.org account
user = create :student, :google_sso_provider

# When I hit the google oauth callback
auth = generate_auth_user_hash \
provider: AuthenticationOption::GOOGLE,
uid: user.primary_contact_info.authentication_id
@request.env['omniauth.auth'] = auth
@request.env['omniauth.params'] = {}
session[:user_return_to] = "/maker/display_google_oauth_code"
CDO.stubs(:properties_encryption_key).returns("thisisafakekeyyyyyyyyyyyyyyyyyyyyy")
assert_does_not_create(User) do
get :google_oauth2
end

# Then I am signed in
user.reload
assert_equal user.id, signed_in_user_id
# Then I go to the display google oauth page to get my code
assert_redirected_to 'http://test.host/maker/display_google_oauth_code'
end

test 'google_oauth2: redirects to maker/display_google_oauth_code if present as user_return_to on unsuccessful google login' do
# Given I do not have a Code.org account
uid = "nonexistent-google-oauth2"

# When I hit the google oauth callback
auth = generate_auth_user_hash \
provider: AuthenticationOption::GOOGLE,
uid: uid,
user_type: '' # Google doesn't provider user_type
@request.env['omniauth.auth'] = auth
@request.env['omniauth.params'] = {}
session[:user_return_to] = "/maker/display_google_oauth_code"
assert_does_not_create(User) do
get :google_oauth2
end

# Then I go to the display google oauth page to see an error message
assert_redirected_to 'http://test.host/maker/display_google_oauth_code'
end

test 'login: google_oauth2 silently takes over unmigrated student with matching email' do
email = 'test@foo.xyz'
uid = '654321'
Expand Down