Skip to content
This repository has been archived by the owner on Dec 22, 2023. It is now read-only.

Allow logging in existing user in promotion flow #1470

Open
2 tasks
kiootic opened this issue Jun 1, 2020 · 11 comments · Fixed by #1472
Open
2 tasks

Allow logging in existing user in promotion flow #1470

kiootic opened this issue Jun 1, 2020 · 11 comments · Fixed by #1472
Assignees

Comments

@kiootic
Copy link
Contributor

kiootic commented Jun 1, 2020

User may select an existing identity in promotion flow. Currently, it would fail with an identity exists error. Use case instead requires user to perform authentication and log in using the existing identity. The anonymous user is discarded in this case.

  • Configuration flag (TBC): on_identity_conflict.promotion
    • error
    • login
  • Hook events (TBC): before_user_promote, after_user_promote
    • anonymous_user
    • user
    • usually anonymous_user and user have same ID
    • when on_identity_conflict.promotion is login, anonymous_user and user may have different ID.

ref #1469

@kiootic kiootic self-assigned this Jun 1, 2020
@louischan-oursky
Copy link
Contributor

I have question regarding the SDK. After the promotion, the user is authenticated as a normal user. When the user logs out and authenticateAnonymously again, is the anonymous user a brand new one?

@kiootic
Copy link
Contributor Author

kiootic commented Jun 1, 2020

Yes, after finishing the promotion flow, the key ID is deleted and a new key is generated.

@chpapa
Copy link
Contributor

chpapa commented Jun 1, 2020

ah, it makes a lot of sense.

Will we have a default login with on_identity_conflict.promotion parameters?

I think in our doc or config comment, we need to remind users that we won't MERGE the anonymous user for them, the application need to handles the merging behavior if they want to after login.

@louischan-oursky
Copy link
Contributor

If the default is login, we assume the application has specific logic to merge the two users. I think error is a better default value because it does not introduce potential data integrity problem to the application. When the developer encounters the error, the developer consults the documentation and realize their application needs to handle such case. login being the default may be surprising to developers who are not aware of this situation.

Also we should turn off anonymous user by default as not every application needs it.

@chpapa
Copy link
Contributor

chpapa commented Jun 3, 2020

@louischan-oursky the dilemma here is, is there any users who would prefer to have an error instead of able to login directly, if he "upgrade" the anonymous users to an existing logged in users? .....

@kiootic
Copy link
Contributor Author

kiootic commented Jun 3, 2020

Login directly as existing user would 'lose' the anonymous user, along with its associated data. If developer forgot to configure it and deployed to production, it can cause destructive data damage. Therefore, error is a safer default.

@chpapa
Copy link
Contributor

chpapa commented Jun 3, 2020

@kiootic I understand what @louischan-oursky said, actually I've already spell out we should add a warning in the documentation about "merging users" before all these already.

I'm just pointing out no normal people would expect, say after clicking "Continue as guest", if you choose "Login", it said "Users existed" when he tried to login an existing user account. This would definitely look like a bug.

So the safer default is also a non-sense default from user's POV.

@chpapa
Copy link
Contributor

chpapa commented Jun 3, 2020

Some ideas of how we might provide a better experience:

Idea One: Explicit error message

If I'm right that no normal users would expect the error flag, what about we make it easy to customize the error message?

And in the default error message, we can make it clear that it is not configured, something like: "Guest users can't promote to an existing user, please read the configuration of on_identity_conflict.promotion" -- so although it looks non-sense to normal users, developers will know what it is about immediately.

Idea Two: Explicit auth UI

At the promotion screen, we default show a sign up UI (like now), with another link to "Login with account"

And instead, we have a configuration to disable and merge the signup / login UI at promotion screen

I feel like idea two is better (more smooth, so the default options make a clear distinguishment, while the configuration provide a smoother experience if the developers know what they're doing.

Any other idea?

@chpapa chpapa reopened this Jun 3, 2020
@chpapa
Copy link
Contributor

chpapa commented Jun 3, 2020

Re-open issue to consider if there is better way to solve the problem

@louischan-oursky
Copy link
Contributor

From the end user's perspective, just like what Ben said, the user normally does not expect the error case. Auth Gear ideally should behave in the following ways

  1. If the identity the End-User provides does not exist, then just promote the anonymous user.
  2. Otherwise, Auth Gear explicitly shows a page to tell the End-user that the identity exists and ask if the End-User wants to just login or merge the two users.

From the developer's perspective, they are very likely unaware of the merge case. So the default configuration should not cause destructive data damage. The main problem becomes how to make the developer aware of it. If the default is error and the developer does test their application before going live on production, the end user should have good UX.

Actually I want to merge the signup, login and promotion page into one. The End-User has fewer things to choose. Instead the UI tells the End-User what is happening and guide them to finish the flow. So I prefer Idea 1.

@chpapa
Copy link
Contributor

chpapa commented Jun 3, 2020

Discussed with @louischan-oursky @kiootic off line, agreed Idea 1 is better, maybe we can use an explicit error message like "You can't continue as guest by loggin in an existing user account".

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants