Skip to content
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

Allow the user to select which account they want to use to log in. #47

Merged
merged 1 commit into from
Jan 30, 2020
Merged

Allow the user to select which account they want to use to log in. #47

merged 1 commit into from
Jan 30, 2020

Conversation

wparad
Copy link
Collaborator

@wparad wparad commented Jan 24, 2020

@wparad wparad requested review from thoean and akincel January 24, 2020 17:17
Copy link
Contributor

@thoean thoean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to have this new parameter in the Readme.md file.

@wparad
Copy link
Collaborator Author

wparad commented Jan 25, 2020

What's the benefit of double documenting every parameter, doesn't your IDE already share the parameter description from the method?

Okay, I guess it doesn't because we are using babel to compile the code. since async/await are already LTS, it might make sense to remove the unnecessary transpiling of the source, then the intellisense would work without duplicate documentation.

Or is there another reason to double document that I'm missing? I would expect the readme.md to include the most important parameters, rather than an example of all of them.

@thoean
Copy link
Contributor

thoean commented Jan 25, 2020

Intellisense != documentation.

But how we wrote the documentation, it's probably a close match. But ideally, the documentation would be a lot more verbose, go into details, and potentially examples.

I'd say, let's continue with the existing pattern of having it "duplicated" in this MR, and if we want a better strategy around documentation (vs. Intellisense or both), we can do that separately. It's a good point to address.

@wparad
Copy link
Collaborator Author

wparad commented Jan 27, 2020

After reviewing the functionality, I realized there was no need to provide this as a parameter, it should always be set, or at least if there is a case for it not be set it would be good to understand that requirement.

This parameter (incorrectly, thanks Auth0) does two things:

  • Ignores the current saved session
  • Asks the user to specify which logged in session they want to use from their social provider.

We never ignore the current saved session because instead of letting authorize internally handle checkSession we call it explicitly. This leaves only the second case, letting the user select their provider.

There should never be a case that the user isn't presented with the option to select an account. If we can log them in directly without needing the universal login, then its fine to reuse the account, but if they have never logged in before to this auth0 tenant, we should never automatically pick their account for them.

The default to authorize for prompt is none because you want to reuse the account and session if the user is already logged in. I have no idea why auth0 attached two pieces of functionality together with this parameter.

Is there a counter to that, or is this still approved?

@thoean
Copy link
Contributor

thoean commented Jan 28, 2020

Assuming checkSession doesn't work, I'm sometimes (a bit unsure what's the criteria here) presented with a blue button that simply allows to click it and logs me in with the last used Email.

Would making the prompt set to a value in all cases remove this option and always show a text field where I have to type my Email? Or is that independent of that?

Outside of that, I agree with your statement.

@wparad
Copy link
Collaborator Author

wparad commented Jan 28, 2020

As far as I can tell, (looking at the source code), prompt is only used by the auth0 server, and is converted from its value to something that the auth provider would understand. There is no code in the auth0Lock widget (and therefore the universal login) which has anything to do with this property. There is a separate property in the lock which does control the remember user: rememberlastlogin. This will pass the loginHint field to your provider.

While hypothetically, there is something that auth0 could be doing with this parameter beside the documented functionality (I.e. decided on the server to not return any data), I can't find any reference to actually passing this parameter to the Auth0 server before clicking through to the auth provider, which at that point is moot.

If I'm right, at that point, it would be up to the auth provider to decide what to with getting both loginHint and a prompt value, which is going to be auth provider specific.

I don't have access to the specific login provider you are using, but let's say you are using Microsoft Office 365 implicit grant flow

Prompt will be converted from none to login here. Which means, when you click on the last used email you'll be sent to microsoft's login and asked to put in your password.

image

You can see from the url that the value of prompt is login. If I set this to none, I just get redirected back to my application already with an access code (since there is a valid session).

Wait, if there was a valid session, I should have been able to bypass the whole login process anyway. Because if there wasn't then Microsoft should have forced me to use my credentials to log in anyway.

Although....

It's possible that Auth0 is keeping different sessions for different computers, or different clients, in that case what happens if you already had a valid microsoft session for Auth0, but Auth0 decided to compartmentalize all the applications for your tenant?

Well in that case there wouldn't be any way to differentiate a malicious application B and a friendly application C when using this setting. Both applications would end up using the same microsoft account. I.e. Assuming a user came from either B or C, you wouldn't know. And therefore wouldn't know if you should issue a request to select the appropriate account.

Interestingly enough, microsoft as well as google follow the Open IDC configuration and looking at prompt, officially there are 4 values.

Additionally, Auth0 is blindly passing this value along via the url, so I don't think there is any data manipulation or interference happening by setting this.

The question becomes, if a user is ever sent to an auth provider, what is the right action:

  • Always force the user to login (even if they have valid sessions)
  • Allow the user to select the correct account for them
  • Force the user to be tied to the first account the provider returns (in the case of a login hint, this is the one provided)
  • Require apps to select which option they would prefer.

I can see we are starting down the path of the last option.

However, in the case where there is a login hint, you could argue that we could skip asking for account selection. In the case where there is no login hint, you could argue that we can't skip it. So let's go for that. I can key this value from the explicit connection setting. We don't currently provide hint, but if we did we could use both fields to correctly determine what to do here.

@thoean thoean merged commit b9a0a49 into Cimpress-MCP:release/4.0 Jan 30, 2020
@wparad wparad deleted the allow-account-selection branch January 30, 2020 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants