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.

Sign up for GitHub

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

Jump to bottom

Gracefully handle Google OAuth failures from Maker App #38245

Merged
merged 9 commits into from
Jan 23, 2021
Merged

Gracefully handle Google OAuth failures from Maker App #38245

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
19f2bd3
update maker sign in URL and go directly to display maker app code if…
JillianK Dec 15, 2020
1cfeb5f
Trigger Drone build
JillianK Dec 15, 2020
abcfdaa
resolve PR comments
JillianK Dec 17, 2020
30e87d0
write tests for maker app google oauth flow
JillianK Dec 18, 2020
6e87b4e
Merge branch 'staging' of github.com:code-dot-org/code-dot-org into j…
JillianK Dec 18, 2020
e2e0f8b
fix tests to stub encryption key
JillianK Dec 18, 2020
3a37b35
fix maker oauth tests
JillianK Dec 18, 2020
78d99b2
clarify test name
JillianK Jan 4, 2021
eb2d245
add comment about being deprecated
JillianK Jan 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 9 additions & 3 deletions dashboard/app/controllers/maker_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,16 +123,22 @@ def login_code
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']
)
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
# 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]
JillianK marked this conversation as resolved.
Show resolved Hide resolved
if current_user
JillianK marked this conversation as resolved.
Show resolved Hide resolved
redirect_to maker_display_google_oauth_code_path
Copy link
Contributor

Choose a reason for hiding this comment

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

since we're making a change to the route in the maker app anyways, i think we should clean up the method names for the routes for this feature -- there are some incorrect/mismatched names and conventions that make it sound like these routes work for any login, when they only currently work for google.

in our routes file:

code-dot-org/dashboard/config/routes.rb

Lines 53 to 55 in 043945b

get 'maker/google_oauth_login_code', to: 'maker#login_code'
get 'maker/display_google_oauth_code', to: 'maker#display_code'
get 'maker/google_oauth_confirm_login', to: 'maker#confirm_login'

the actual URLs are correct, but the method names (e.g., maker#login_code) don't match, so we should change the method names to make it clear that these routes are currently meant for google only.

in the controller file:

code-dot-org/dashboard/app/controllers/maker_controller.rb

Lines 116 to 136 in 043945b

# GET /maker/login_code
# renders a page for users to enter a login key
def login_code
end
# GET /maker/display_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']
)
end
# GET /maker/confirm_login
# Renders a page to confirm uses want to login via Google OAuth
# 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
end

the comment above each route here is incorrect. for the first one, maker#login_code, we can see from the routes file that the URL is GET /maker/google_oauth_login_code. these are definitely just mistakes we missed during code review.

i think any/all of these changes should be made in a follow-up PR, but i wanted to call them out here

end
end

# POST /maker/complete
Expand Down
22 changes: 12 additions & 10 deletions dashboard/app/views/maker/display_code.html.haml
View file
Open in desktop
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
View file
Open in desktop
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: 'Wrong provider. Please make sure you are logging in with Google or you have linked your Google account to your Code.org account'
JillianK marked this conversation as resolved.
Show resolved Hide resolved
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
1 change: 1 addition & 0 deletions i18n/locales/source/dashboard/base.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1153,6 +1153,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: 'Wrong provider. Please make sure you are logging in with Google or you have linked your Google account to your Code.org account'
error_invalid_user: 'Invalid user id. Please try logging in again.'
error_no_code: 'Please enter your login code.'
parent_email_banner:
Expand Down