🐞🔨Authentication Prevent race condition when double-tapping sign-in
#1892
Conversation
zspencer
changed the title
Authentication Prevent a condition when double-tapping sign-inAuthentication Prevent race condition when double-tapping sign-in
app/models/authenticated_session.rb
Outdated
| @@ -45,6 +45,16 @@ def save | |||
| end | |||
|
|
|||
| false | |||
| rescue ActiveRecord::RecordInvalid | |||
| self.authentication_method = nil | |||
| if one_time_password.nil? | |||
There was a problem hiding this comment.
I suppose you could extract lines 38-44 into a method called, say verify_or_resend_otp, and then this save method would become something like:
def save
return false if !valid? || !actionable?
verify_or_resend_otp
rescue ActiveRecord::RecordInvalid
# Retry in case of race condition
self.authentication_method = nil
verify_or_resend_otp
endThere was a problem hiding this comment.
That's a better name than what I came up with on my own; so I'll do that.
app/models/authenticated_session.rb
Outdated
| @@ -45,6 +45,16 @@ def save | |||
| end | |||
|
|
|||
| false | |||
There was a problem hiding this comment.
I know this is not from this PR, but I think this false never did anything -- the if/else above will always return before this line is executed.
There was a problem hiding this comment.
Oh that's a good point!
|
And for completeness: a different approach, if we think this is due to simultaneous submissions, we could consider using some kind of locking mechanism. Seems overkill for now, though. |
This is due to simultaneous first-time submissions. I.e. if you attempt to sign in as [email protected] for the first time and double-click the button; the error will occur. It does not appear to occur with second-times; as the authentication method is already in the database so double-clicking will just double-email.
I could have sworn there was something like that built into rails, but maybe it was part of ujs and isn't in turbo yet... |
Me too, and I googled a bunch and couldn't find anything. Something about "disable with"??? But couldn't find it. 🤷🏼♀️ |
- #1809 OK, so my working assumption is that the bug is caused by two requests being processed at the same time for a new email address, and the first one creates the `AuthenticationMethod` and the second one *tries* to, and fails; resulting in the validation error. This requeries the database and retries the rest of the work. Would love to have names for methods to pull this out into, rather than copy-pasting the method body but oh well.
zspencer
force-pushed
the
authentication/fix-the-other-weird-sign-in-bug
branch
from
f931f7d to
8586db7
Compare
…#1892) * 🐞🔨`Authentication` Prevent a condition when double-tapping sign-in - #1809 OK, so my working assumption is that the bug is caused by two requests being processed at the same time for a new email address, and the first one creates the `AuthenticationMethod` and the second one *tries* to, and fails; resulting in the validation error. This requeries the database and retries the rest of the work.
OK, so my working assumption is that the bug is caused by two requests being processed at the same time for a new email address, and the first one creates the
AuthenticationMethodand the second one tries to, and fails; resulting in the validation error.This requeries the database and retries the rest of the work.
Would love to have names for methods to pull this out into, rather than copy-pasting the method body but oh well.