-
Notifications
You must be signed in to change notification settings - Fork 949
[Auth] Fix persistence issue when manually setting/using session storage #5239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Changeset File Check
|
Size Analysis ReportAffected Products
|
f52b079
to
ee1420a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - will defer to @yuchenshi / @avolkovi who have more context on the change! Thanks for the fix!
packages-exp/auth-exp/test/integration/webdriver/persistence.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments inline. I totally understand you're trying to improve the readability here -- and in that spirit I think we should split it into different helper functions.
Step 1: Choosing the preferred persistence (the first one that is available)
Step 2: Attempting to locate the user from all persistences (return user and source persistence)
Step 3 (can stay in this function): Migration logic (a. if user is in a persistence that doesn't support migration, use that instead and skip migration, or b. migrate if different)
With that being said, I don't feel like I have the authority for TypeScript readability to block this PR any longer. Approved with comments inline -- feel free to file a bug for later cleanup.
const migrationHierarchy = persistences.filter( | ||
p => p._shouldAllowMigration | ||
); | ||
chosenPersistence = migrationHierarchy[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if this is undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by just returning the manager if it is
packages-exp/auth-exp/src/core/persistence/persistence_user_manager.ts
Outdated
Show resolved
Hide resolved
if (persistence !== chosenPersistence) { | ||
userToMigrate = user; | ||
} | ||
chosenPersistence = persistence; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest not assigning to chosenPersistence
here since it is not the chosen persistence (but rather the persistence to migrate from). It is very confusing that chosenPersistence
is then overwritten in L162 below. I'd suggest inlining _shouldAllowMigration
check from below. Or just use a different variable name if you must.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to "selectedPersistence" as that accurately reflects what this code is doing
After some tinkering I think refactoring into helper functions would make this code even more complex. |
Fixes #5171
browserSession
persistence is now added to the escalation path ofgetAuth()
to match v8. We also now only "upgrade" the persistence if the persistence deems itself "upgradeable" (which only applies toindexedDB
andlocalStorage
)