-
Notifications
You must be signed in to change notification settings - Fork 171
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
Support multiple anonymous sessions #5693
Conversation
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.
This would be a breaking change and not what #4607 is asking for. What I'm suggesting there is that we add a parameter to the anonymous credentials that would indicate whether to reuse or not reuse existing anonymous users. The default value should be true
to make sure that existing applications that authenticate anonymous users continue behaving the same way. If the user opts out of reusing an existing user, then we should authenticate a new user and return that.
OK, my change was too extreme, I redesign the code as per your suggestion. |
It might also make sense to instead have reused and unique anonymous users instead be different values in the credentials provider enum rather than having a bool field which is only applicable to one of the credential types. |
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.
Looks fine to me, though it'd be a good idea to add some tests to verify that it works as intended.
Done, please approve, and I will merge the PR once CI has run successfully. |
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.
It seems fine to me, but I strongly suggest you get a review from someone on the Core team.
test/object-store/sync/app.cpp
Outdated
auto user2 = log_in(app, AppCredentials::anonymous(false)); | ||
CHECK(user1 != user2); | ||
|
||
App::clear_cached_apps(); |
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.
TestSyncManager takes care of this.
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.
OK, let me remove the line.
What, How & Why?
Allow multiple anonymous sessions.
Fixes: #4607