Performing the OAuth authorisation flow leads to logout in the browser

[Dagefoerde]

Steps to reproduce

1. Log in to Nextcloud
2. Use another application that requests authorisation to use Nextcloud via the OAuth login flow (using the same browser as in 1.!)
3. Observe that the original session from step 1 is not usable anymore; i.e., the OAuth client is authorised correctly but the browser needs to log in again.

Expected behaviour

The requesting client is authorised correctly AND the original session is still usable. The user should not need to log in again.

Actual behaviour

Browser is logged out: Either original session expired or the session ID is removed from/replaced in the browser so that original session cannot be accessed anymore.

Server configuration

Operating system: Ubuntu 16.04.5 LTS (GNU/Linux 4.4.0-131-generic x86_64)

Web server: Apache/2.4.18

Database: PostgreSQL 9.5

PHP version: 7.0.30-0ubuntu0.16.04.1

Nextcloud version: 13.0.5.2 (1a082ab)

Updated from an older Nextcloud/ownCloud or fresh install: Upgraded from 13.0.4

Where did you install Nextcloud from: nextcloud.com

Are you using encryption: no

Are you using an external user-backend, if yes which one: no

Client configuration

Browser: Version 68.0.3440

Operating system: Ubuntu 18.04.1 LTS

[Dagefoerde]

Also, (slightly related, at least for my use case...) if you could reopen #7344 (comment) that would be great. I added steps at the end of the discussion. Thanks.

[stjosh]

I can confirm @Dagefoerde's observation - just stumbled on this and it's puzzling me quite a bit. I'm glad I'm not the only one irritated by this behavior. 😆

Digging into the Code, I found that this line in core/Controller/ClientFlowLoginController.php is causing this behavior:

$this->tokenProvider->invalidateToken($sessionId);

It was introduced in #7698 to fix #7697 ... @rullzer @MorrisJobke : Maybe you could comment on this? I see the point mentioned in #7697 that ending up with two "fresh" sessions is undesirable, especially when going through the flow in order to get fresh credentials for a Client App. However, I think killing a pre-existing Browser session by the OAuth 2.0 flow is probably an unintended side-effect of #7698 ? At least to me (and @Dagefoerde ), this is very much unexpected.

Would it be a possibility to move the line mentioned into the else block just above, i.e., only deleting the previous session when redirecting to the nc:// URL (i.e., when dealing with an App or so) but not when dealing with the OAuth 2.0 Flow which usually happens in a normal Browser? (At least, that's how I interpret the distinction between the if and the else block there...)

[rullzer]

@stjosh yes it was added there because we did not consider the OAuth flow at that point.
I'll see if I can make a check not to invalidate the session if you are logged in to start with.

[rullzer]

@stjosh actually what you propose makes perfect sense and should be trivial to backport as well. Let me prepare a PR.

[stjosh]

Very cool, thanks a lot @rullzer