FIX: Make email_valid handling consistent (PR #11556)

[davidtaylorhq]

Previously we were checking truthiness in some places, and == true in others. That can lead to some inconsistent UX where the interface says the email is valid, but account creation fails.

This commit casts the value to a boolean when it is first set, with a safety check in place for badly parsed ‘false’ strings.

If this safety check is triggered, it means the specific auth provider should be parsing the string and converting to a boolean.

I am not aware of any providers which have this issue, but let’s keep the check to make this change fail-safe.

GitHub

[CvX]

lib/auth/result.rb

@@ -52,6 +52,13 @@ def email
     @email&.downcase
   end
 
+  def email_valid=(val)
+    if val == 'false'
+      raise ArgumentError, "email_valid is a 'false' string. Convert to boolean before creating Auth::Result"
+    end
+    @email_valid = !!val

Is "false" the only invalid input this can receive? If not, perhaps allowing a certain set of values and raising on anything else would be better? 🤷

[davidtaylorhq]

I made this PR because someone had configured the discourse-oauth2-basic plugin to use use the actual email address as the email_verified value.

Ideally, only true and false would be valid values. My worry is that enforcing that will break some auth plugins… maybe we should just rip off the band-aid and identify all the issues.

[CvX]

I’d either go all in and let any invalid auth plugins burn or accept all input and try to auto-correct (i.e. instead of raising on "false" treat it as false)

One is more “correct” but provides you with with potential work for the years to come

[davidtaylorhq]

Let’s just go all-in. Auth, and email validation, are security-critical things so I think it’s better for things to fail than have weird edge cases. Will wait until after the holidays to merge though - we don’t want to accidentally make extra work for ourselves over the next few days.

[davidtaylorhq]

To be merged after the holidays, just in case it causes any auth plugin issues. Marking draft to prevent accidental merging