fix auth races for react native #29
Conversation
erquhart
force-pushed
the
fix-auth-races
branch
from
c75820d to
9d467b1
Compare
| // is waiting for the server to confirm a token, ignore. | ||
| // TODO: This shouldn't rely on a specific error text, make less brittle. | ||
| // May require backend changes. | ||
| if ( |
There was a problem hiding this comment.
There's a server-side change where the AuthError should have a field authUpdateAttempted: boolean. It's true when we're attempting to update to a new auth token, and false otherwise, so I believe you can switch this condition to checking for authUpdateAttempted is false
| @@ -206,12 +222,13 @@ export class AuthenticationManager { | |||
| // in that we pause the WebSocket so that mutations | |||
| // don't retry with bad auth. | |||
| private async tryToReauthenticate(serverMessage: AuthError) { | |||
| // We only retry once, to avoid infinite retries | |||
There was a problem hiding this comment.
Can we add this comment back? I understand that there are retries now to deal with interruptions during the re-authentication, but I think the point that we should really only be calling tryToReauthenticate a fixed number of times seems nice to document
| @@ -176,13 +179,26 @@ export class AuthenticationManager { | |||
| if (this.authState.state === "waitingForServerConfirmationOfFreshToken") { | |||
| this._logVerbose("server confirmed new auth token is valid"); | |||
| this.scheduleTokenRefetch(this.authState.token); | |||
| this.tokenConfirmationRetries = TOKEN_CONFIRMATION_RETRIES; | |||
There was a problem hiding this comment.
nit: It was unclear to me whether this was supposed to be tries remaining or tries attempted. maybe this.tokenConfirmationAttempts and set it at 0 initially?
| ); | ||
| this.resumeSocket(); | ||
| this.restartSocket(); |
There was a problem hiding this comment.
Should we also be resetting this.tokenConfirmationRetries?
| ]; | ||
| const tokenFetcher = vi.fn(async (_opts) => tokens.shift()!); | ||
| const onChange = vi.fn(); | ||
| client.setAuth(tokenFetcher, onChange); |
There was a problem hiding this comment.
To make this more realistic, we should be subscribing to a query, right? Otherwise there's no reason we'd get the AuthError back if there are no functions to execute.
If that's hard to actually do in this test, we can just add a comment that in the real world we'd need a query subscription here
| client.setAuth(tokenFetcher, onChange); | ||
|
|
||
| expect((await receive()).type).toEqual("Connect"); | ||
| expect((await receive()).type).toEqual("Authenticate"); |
There was a problem hiding this comment.
This is Authenticate with identity version 1, right?
There was a problem hiding this comment.
(I'll start my own branch adding some more comments since that seems more efficient than telling you to add them)
|
Ok it took me forever, but I built off of this here (sorry for weird branching). Would love a look at those changes + some manual testing, and then we can either port them over here or merge the other one |
|
Cool, I'll go ahead and close this, we could always reopen if need be. |
Most of this PR is from get-convex/convex-js#29 from @erquhart There's one more commit I added to (1) use `authUpdateAttempted` on `AuthError` instead of needing to string match now that the backend feature is in -- we should double check that we're seeing this field come through on the WS with any repros we have and (2) add even more comments to the tests. (pushed up here because GH permissions are hard) Co-authored-by: Sarah Shader <sarahshader@gmail.com> Co-authored-by: Shawn Erquhart <shawn@erquh.art> GitOrigin-RevId: 446f5ade8dee52d8b8c045e80105c26200458a8c
Most of this PR is from #29 from @erquhart There's one more commit I added to (1) use `authUpdateAttempted` on `AuthError` instead of needing to string match now that the backend feature is in -- we should double check that we're seeing this field come through on the WS with any repros we have and (2) add even more comments to the tests. (pushed up here because GH permissions are hard) Co-authored-by: Sarah Shader <sarahshader@gmail.com> Co-authored-by: Shawn Erquhart <shawn@erquh.art> GitOrigin-RevId: 446f5ade8dee52d8b8c045e80105c26200458a8c
|
merged in here |
This PR addresses three race conditions, which can technically appear on the web, but are very likely to appear for native. They are technically isolated, but it helps to evaluate as a whole, so I've opened a single PR for fixing all of them.
All of these were observed and fixed using Convex Auth, but only one was suitable for fixing directly in that library: get-convex/convex-auth#163
The race conditions present in one of two ways:
Race conditions
False unauth state: Authenticate ws message followed by unrelated auth error
When sending a fresh token for server validation, any subsequent auth error triggers unauth state in the client. It is possible for a query or mutation to trigger an auth error response while the client is awaiting token validation, leading to an erroneous unauth state in the client. This PR checks the auth error message in this case to ensure the error is in response to an Authenticate request, and ignores the error if not.
False unauth state: Reauth attempt disrupted by app moved to background
When a native app is moved to background during reauth, the response may not be processed at all, or could be queued and executed after the token has expired. This PR allows up to two retries when token validation returns an error.
Lost ws connection: Reauth stops connection and doesn't restart
Reauth stops the connection and restarts it at the end of the function, but the function can return before the restart if another token fetch has initiated while the reauth was running. If a regular refetch, such as a scheduled refetch, runs while reauth is running, reauth will return early and refetch does not restart the connection. This PR attempts to restart the connection at the end of both refetch and reauth to ensure the connection recovers.