Gracefully handle Google OAuth failures from Maker App

[JillianK]

A few changes here:

1. When a user was already logged in, they were seeing an error instead of the "magic code" to paste into Maker App. This takes them directly to the code page if they're already logged in. This is actually handled in the related PR on the Maker App
2. If a user tries to log in without a google authentication option, show them a more useful message (final wording still in progress)

Here's a video for when a user is not logged in and uses a valid google auth option: https://drive.google.com/file/d/1_fvrYv2oMNnOcluvxi2LFgRiSGbIGlJW/view?usp=sharing
Here's a video for when a user is not logged in and uses a non google auth option: https://drive.google.com/file/d/1SAux-C79c6dOOyJu_IxR2nL3v60vgZhQ/view?usp=sharing
If the user is already logged in, it will skip directly to that last page (with either the code or the error message depending on the existence of a google authentication option)

Links

• jira
• related PR on the Maker App to go directly to confirm_login

Testing story

Reviewer Checklist:

• Tests provide adequate coverage
• Privacy and Security impacts have been assessed
• Code is well-commented
• New features are translatable or updates will not break translations
• Relevant documentation has been added or updated
• User impact is well-understood and desirable
• Pull Request is labeled appropriately
• Follow-up work items (including potential tech debt) are tracked and linked

[maddiedierker]

we should verify with marina that the "logged out with no google authentication option" behavior is what we want here. i know this is the default route for this type of user/situation, but it feels a little roundabout to show them a message that says they can update their account with a google option, then immediately give them an error after they sign in

[maddiedierker]

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

[JillianK]

we should verify with marina that the "logged out with no google authentication option" behavior is what we want here. i know this is the default route for this type of user/situation, but it feels a little roundabout to show them a message that says they can update their account with a google option, then immediately give them an error after they sign in

I didn't think it was an option but I think I've found a way to go around it. I added it in this commit in omniauth_callbacks_controller and I'd love to know if there's a better way to be doing it.

[maddiedierker]

wonderful work, jillian, and thank you for making this experience much more robust!

[epeach]

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.

[JillianK]

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

[epeach]

Yes - thank you!

[epeach]

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

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

Line 10 in c47eb84

if params[:maker]

) 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.

[JillianK]

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?

[epeach]

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.

[maddiedierker]

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

[epeach]

LGTM! Yay!