Coverage of weird edge cases

[islemaster]

Trying to figure out how I can tear out some weird edge cases in our sign-up code. Here I've proven a little of it is unreachable and torn it out, and worked out the bizarre circumstances that allow some other code to run. Descriptions inline.

[islemaster]

This code is unreachable because:

It falls beneath the if clause above:

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

Line 184 in 90b09b5

if allows_silent_takeover user, auth_hash

Which is defined thus:

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

Lines 438 to 443 in 90b09b5

	def allows_silent_takeover(oauth_user, auth_hash)
	allow_takeover = auth_hash.provider.present?
	allow_takeover &= AuthenticationOption::SILENT_TAKEOVER_CREDENTIAL_TYPES.include?(auth_hash.provider.to_s)
	lookup_user = User.find_by_email_or_hashed_email(oauth_user.email)
	allow_takeover && lookup_user && !oauth_user.persisted?
	end

Let's break these cases into named clauses to help work out the logic:

• A: AuthenticationOption::SILENT_TAKEOVER_CREDENTIAL_TYPES.include?(auth_hash.provider.to_s)
• B: User.find_by_email_or_hashed_email(oauth_user.email)
• C: !oauth_user.persisted?

So we can express our branching logic this way:

if A && B && C
  silent takeover and sign in
elsif B
  case we want to eliminate
else
  register new user

Now let's simplify some things:

• Because this is in the Google-specific callback, A is always true and can simply be eliminated from this expression.
• B and C do not vary independently: User.from_omniauth will persist a google user if and only if that user's email is not already taken. (This is not true for all providers - more on that later). Therefore, we can say if B is true, then C must also be true. (If B is false then C may be true or false). Therefore B && C depends entirely on the value of B and C can be eliminated from this expression.

That reduces our logic to:

if B
  silent takeover and sign in
elsif B
  case we want to eliminate
else
  register new user

We can safely eliminate the second case.

[islemaster]

That second case can't be eliminated for Clever because User.from_omniauth may persist a user whose email is already taken, so I wanted to work out test cases that actually exercise this path.

It turns out this only happens if

1. You are signing in with an untrusted email provider, like Clever.
2. You are a teacher so we actually keep and care about your email address.
3. A prior account already exists in our system using your email address.
4. A second prior account also exists that was created with an untrusted email provider (like Clever) resulting in an email like 'me@example.com.oauthemailalreadytaken'.
5. Neither of the above accounts match your OAuth credentials.

For example, this might be an issue if many Clever teacher accounts have the same email address (apparently possible in their system).

For extra weirdness, the resulting redirect and the message we eventually display to the teacher varies depending on whether the second account (created in step 4, above) is a Clever account or some other untrusted email account - maybe a mistake?

I'd eventually like creation to fail at the second account (step 4) above and direct the teacher to perform a password reset on their existing account, and link Clever via the accounts page. Once that is true we will have a strong guarantee that User.from_omniauth will not persist users with an email conflict, and we'll be able to tear out these tests and the corresponding code they cover. For now though, I'll settle for test coverage of these weird cases.

[joshlory]

I wonder if a contributing factor here is how much code lives in OmniauthCallbacksController vs. being pushed down to a model. Would it be more understandable/testable if more logic lived in model code?

[maddiedierker]

this is a great refactor! thank you for the additional test coverage as well 🎉

[islemaster]

Re @joshlory's comment: I think that's a little bit of the problem, but maybe less than some other cleanliness issues here (and we're all loath to move code into User). We've got a bunch of state-dependent functions with side effects in this code - @user gets set in a few places, and User.from_omniauth rolls create, retrieve and update behaviors all into one call.