Add clear method on login storage to prevent storing invalid tokens #44
Conversation
| return | ||
| } | ||
|
|
||
| try await storage.storeLogin(login) |
There was a problem hiding this comment.
We could alternatively do a storage.storeLogin(nil) as the "clearLogin" but I'm not sure if that's a wise idea
| } | ||
| catch let authenticatorError as AuthenticatorError { | ||
| await self.config.authenticationStatusHandler?(.failure(authenticatorError)) | ||
| try await clearLogin() |
There was a problem hiding this comment.
I'm not sure if I really needed to move this or not. I suspect maybe not.
There was a problem hiding this comment.
I've moved this back, but I'm not certain that's correct, as the final call in makeLoginTask could still throw, even though we bailed refresh because the refresh token wasn't valid.
I think we actually want to clear the login if the refresh isn't valid, and then also not have the last line of makeLoginTask because that would arbitrarily launch of authorization code grant flow, which would be unexpected for the user — throwing in this case is almost certainly the desired behavior.
"We had a session, we tried to refresh it, we failed, so we cleared the login" is conceptually better from a UX perspective than "we had a session, we tried to refresh it, we failed, so we showed an OAuth authorization flow when the user was not expecting it"
|
|
||
| public struct LoginStorage: Sendable { | ||
| public typealias RetrieveLogin = @Sendable () async throws -> Login? | ||
| public typealias StoreLogin = @Sendable (Login) async throws -> Void |
There was a problem hiding this comment.
So we could alternative change this to:
public typealias StoreLogin = @Sendable (Login? = nil) async throws -> Void
|
Yeah, this was definitely a missing piece. I'm only 75% sure I agree that a failed refresh should not result in re-auth flow. But, I think this whole thing is an improvement anyways, and that can always be revisited if it turns out to be undesirable for some reason. |
ThisIsMissEm
force-pushed
the
feat/clear-method-on-login-storage
branch
from
9dede37 to
529b8f1
Compare
|
@mattmassicotte I've rebased this and it should be good to go. |
|
Not sure what the test failures are from.. |
2 participants
This removes the "invalid login" that was previously being used, so is a breaking change, since it requires a new method be implemented in
loginStorage(clearLogin). Perhaps we take this moment to also rename those methods toclearSessionet' all.Adding a
clearLoginmethod seemed like the better option than makingstoreLoginaccept a nil value as theloginargument.This is unfortunately based on #42