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

Logout and PKCE #1389

Closed
pktharindu opened this issue Dec 2, 2020 · 18 comments
Closed

Logout and PKCE #1389

pktharindu opened this issue Dec 2, 2020 · 18 comments

Comments

@pktharindu
Copy link

pktharindu commented Dec 2, 2020

  • Passport Version: 10.1.0
  • Laravel Version: 8.16.1
  • PHP Version: 7.4.8
  • Database Driver & Version: MySQL 5.7

Description:

I'm using Authorization Code Grant with PKCE for a browser extension we are working on, and it works as expected. The only inconvenience that I have is when I log out (by revoking the token), and try to re-login, it skips the login form entirely and jumps straight into the callback page. In other words, users won't get to enter their credentials the second time around.

This happens because even though the access token is revoked, the server session is still active.

Although this may be the default behavior, I personally think this breaks users' expectations and possibly introduces a security concern (i.e. after I log out, someone else can access my account just by clicking "log in", without having to enter my credentials).

After a bit of Googling, I found that the prompt parameter can be used to change this behavior.

  • prompt=none: When prompt is set to none, Hosted Login first checks to see if the client has a valid session. If a valid session is found the user doesn't need to authenticate; instead, he or she is automatically logged in using the existing session. If a valid session can't be found a "No authenticated session found" error is generated and the user is not given the option of logging in.
  • prompt=login: The sign-in screen is always displayed first, even if a valid session is found. This ensures that users log in each time they access the site.
  • prompt=create: The traditional registration screen (used for creating new account) is always displayed first. Note, however, that the Sign In link isn’t found on the traditional registration screen. That means that setting the prompt to create represents a dead-end for existing users: they don’t need to create account, but they can’t log on using their existing account.

Source: https://janrain-education-center.knowledgeowl.com/home/authorization-code-pkce-for-mobile-apps. (There is a nice little video down below explaining how each of it works.)

But setting this parameter has no effect on Laravel Passport.

How can I achieve this prompt=login behavior?

BTW, my API logout function looks like this:

public function logout()
{
    Auth::user()->token()->revoke();
    $tokenId = Auth::user()->token()->id;

    $refreshTokenRepository = app('Laravel\Passport\RefreshTokenRepository');
    $refreshTokenRepository->revokeRefreshTokensByAccessTokenId($tokenId);

    return response(null, Response::HTTP_NO_CONTENT);
}
@jasperjorna
Copy link

I've ran into the same situation where users want to logout en switch to a different account. Laravel still holds the session and therefore skips the login screen. I'm using App Auth (https://github.com/FormidableLabs/react-native-app-auth), which has the option to set prompt as an additional parameter, but I don't think Passport is actually using it.

@davestewart
Copy link

Yeah, there seems to be quite a lot of information out there about the prompt parameter:

https://www.google.com/search?q=OAuth+PKCE+%22prompt%3Dlogin%22

@driesvints
Copy link
Member

I think we'd be open to adding this one way or another. Feel free to attempt a PR if anyone's up for one 👍

@davestewart
Copy link

Feel free to attempt a PR if anyone's up for one

Nice one @driesvints 🙂

Any suggestions on which classes / files to start from?

@driesvints
Copy link
Member

@davestewart I have to admit I'm not really sure myself. Now that I look at it closer can you not already achieve this with passing prompt in the query here? https://laravel.com/docs/8.x/passport#code-grant-pkce-redirecting-for-authorization

@davestewart
Copy link

Not sure actually; I think @pktharindu already tried this?

@pktharindu
Copy link
Author

Yes, I did, and it didn't have any effect.
Looking at the implementation, I can see League\OAuth2\Server\RequestTypes\AuthorizationRequest doesn't seem to accept the prompt parameter. So I guess it has to be PRed into league/oauth2-server instead.

@driesvints
Copy link
Member

Yeah that's what I believe as well. I think OAuth2 Server needs a patch for this.

@Sephster
Copy link
Contributor

Sephster commented Dec 8, 2020

Why isn't the session destroyed if you are logging out of the system? Apologies if I am missing something here.

@davestewart
Copy link

davestewart commented Dec 8, 2020

Why isn't the session destroyed if you are logging out of the system? Apologies if I am missing something here.

@Sephster

My understanding is that because of PKCE we need to log in via a form on the website, a session is started, the authentication code is returned, along with a cookie.

However, once we have exchanged the authentication code for an access token, and we log out (revoke the token) via the API – not the website – the session on the main website still exists.

Next, if we choose to log in again, because there is no prompt=login support, when the user returns to the site, we are transparently logged back in again – because of the cookie – and the PKCE flow starts again.

Right now, when the user clicks "log out" in the client, we are solving it by calling both:

  • the API logout endpoint (POST with Bearer token)
  • the main website logout endpoint (GET with cookie)

(Apparently there is no way to do it all on the one endpoint on the server)

I think I got that right @pktharindu !?

@jasperjorna
Copy link

@davestewart I think that's exactly right.

Is there already an issue at https://github.com/thephpleague/oauth2-server that we can track?

@davestewart
Copy link

davestewart commented Dec 10, 2020

It's a shame there's not some flag in the initial call which would trigger Laravel to forget the (supposedly temporary) session as the PKCE flow returns the access token, rather than having another flag for when the user may – or may not – log back in again.

Something like:

GET /oauth/authorize?redirect_url=xxx&session=temporary1&...

Is there a parallel in Laravel Land? Perhaps flash messages?

It's been so long since I did any Laravel!

EDIT:

The workaround for now would be to call /logout immediately after completing the PKCE flow, but the client wouldn't know if the user was already logged in or such like, so you could accidentally nuke an existing session.

@Sephster
Copy link
Contributor

The key bit here is that login sessions and token possession are or should be independent of one another.

I don't think there is an issue flagged in oauth2 server. I will take a look at this before the week is out and maybe we can transfer this issue to that repo if appropriate

@pktharindu
Copy link
Author

@Sephster that's my understanding as well. Ideally, prompt=login should always show the login form despite having/not having a valid session.

In case anybody is looking for a workaround, ours currently looks something like below:

class LogoutController extends Controller
{
    public function __construct()
    {
        $this->middleware('auth:api')->only('logout');
        $this->middleware('auth')->only('logoutSession');
    }

    public function logout()
    {
        Auth::user()->token()->revoke();
        $tokenId = Auth::user()->token()->id;

        $refreshTokenRepository = app('Laravel\Passport\RefreshTokenRepository');
        $refreshTokenRepository->revokeRefreshTokensByAccessTokenId($tokenId);

        return response(null, Response::HTTP_NO_CONTENT);
    }

    public function logoutSession(Request $request)
    {
        Auth::logout();

        $request->session()->invalidate();

        $request->session()->regenerateToken();

        if ($request->has('redirect_uri')) {
            return redirect($request->redirect_uri);
        }

        return redirect('/');
    }
}

And in the client, you can do something like:

axios.post('/api/v1/logout')
  .then(() => window.location.href = '/logout?' + new URLSearchParams({redirect_uri: 'http://localhost:3000'}).toString())
  .catch(function (error) {
    console.log(error);
  });

As @davestewart said, this also invalidates the session. So, if you have an admin panel or whatever, you'll have to re-login to that. But IMHO, it beats not having this behavior at all. 😅

@davestewart
Copy link

Another option we discussed was running the API auth on a different domain, that way any admin sessions would be independent. I can't remember now if there were any incompatibilities with that approach now, or whether it just seemed like too much hassle, @pktharindu ?

@Sephster
Copy link
Contributor

I've had a chance to look into this now. The prompt parameter is part of Open ID Connect and not part of standard OAuth 2 so I don't think that is the right solution for this issue.

Looking at the authorize end point it should only automatically authorize the client if a valid token is present or if the client is first party:

if (($token && $token->scopes === collect($scopes)->pluck('id')->all()) ||
$client->skipsAuthorization()) {
return $this->approveRequest($authRequest, $user);
}

I can't see anything relating to sessions although I might be missing something here. Can anyone see the specific bit of code tha is ignoring the authorize step if we already have a valid login?

@pktharindu
Copy link
Author

@Sephster, the authorization step isn't really the problem. Let me explain the context here.

We have a web application that includes a dashboard where the users can manage their personal/billing details and an API powering our first-party chrome extension. Ideally, the users should be able to login to the dashboard, and the extension independently.

But since the PKCE flow is not fully independent from the server session, logging out of the extension becomes problematic as invalidating the access token does nothing to the server session. So, in case you want to switch to a different account, you have to log out of both the extension as well as the web app, which seems like an extra step that doesn't really make sense from the users' point of view. If you only log out of the extension, somebody else can still access your account without having to enter your credentials.

@Sephster
Copy link
Contributor

Because your browser extension is first party, I believe this is expected behaviour. If you want to prevent this, you should set skipsAuthorization to return false.

See pr #1022 for where this feature was first introduced.

One final thing I was wondering is, does your extension initiate the flow without any user input? This doesn't seem correct if this is the case.

Hope this helps

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

No branches or pull requests

5 participants