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

Remember users are already logged in on non-origin requests, without giving them authorization. #157

Open
rien opened this issue Oct 11, 2021 · 7 comments
Labels

Comments

@rien
Copy link
Collaborator

rien commented Oct 11, 2021

In PR #155 we've added a page where users always have to confirm they want to start the authorization flow for a client. This is needed because our session cookie is SameSite=Strict so this cookie won't be sent when a user is redirected to zauth, unless the user does an interaction like clicking the "yes" button (issue #146).

I think a solution here would be to add a SameSite=Lax cookie with information about who is logged in, but which does not give authorization rights. When the authorization flow is started, the absence of this cookie would send the user directly to the login page. If such a cookie is present, we can present the new authorization page where the user has to confirm they want to login to the client with their user (similar as to how the Microsoft login works):

image

Special care should be taken to not give the Lax cookie rights to other authorizations, it should be purely to detect who could be logged in. One way to do this would be to not store the session id, but only the user's id or even the user's name.

@rien rien added good first issue Good for newcomers backend and removed good first issue Good for newcomers labels Oct 11, 2021
@chvp
Copy link
Member

chvp commented Oct 12, 2021

I think this solution still leaves a weird flow. If a user is already logged in, and wants to authorize a client that needs a grant, they will be hit by two very similar screens one after another.

@chvp
Copy link
Member

chvp commented Oct 12, 2021

That scenario wouldn't be as bad if zauth remembered if a user already granted a client once, but looking at the code it doesn't seem like it does so.

@rien
Copy link
Collaborator Author

rien commented Oct 12, 2021

If a grant would required and the user is already logged in, we can just show the grant screen instead of the current authorization confirmation. I don't think this would lower our security, but it does make the flow more complex.

That said, we should indeed remember previous grants, but that's for another issue.

@chvp
Copy link
Member

chvp commented Oct 12, 2021

But that brings us back to the original issue, right? Where we don't have the session cookie until a manual action by the user.

@rien
Copy link
Collaborator Author

rien commented Oct 12, 2021

Not if we have a SameSite=Lax cookie with the user name.

Since the authorization request contains the client id, we know whether a grant is required or not. Thus, if a lax cookie is present, we can immediately show the grant page without even having to know if the user is actually logged in or not.

@chvp
Copy link
Member

chvp commented Oct 12, 2021

Hmm, my brain is unable to figure out right now what should happen in the case that the Lax cookie is wrong and the grant is granted, but that can probably be figured out when actually implementing this.

@rien
Copy link
Collaborator Author

rien commented Oct 12, 2021

When a grant is submitted (through a POST request), the controller will/should always check the session cookie. That way, an incorrect Lax cookie would just result in the user being shown an "unauthorized" screen.

Incorrect Lax cookies should also not occur in practice: these cookies should be encrypted (private) as well, so one should not be able to forge such a cookie.

We want to prevent that it is possible for a user to perform an OAuth2 flow initialized by a malicious client without consenting or noticing. The user should always be able to check whether they really want to authorize an application. In the case a grant is needed, the user is able to do this.

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

No branches or pull requests

2 participants