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

OAuth2 token revokation does not expire sessions #103

Closed
jesmrec opened this issue Dec 1, 2017 · 12 comments · Fixed by owncloud/core#30365 or #112
Closed

OAuth2 token revokation does not expire sessions #103

jesmrec opened this issue Dec 1, 2017 · 12 comments · Fixed by owncloud/core#30365 or #112

Comments

@jesmrec
Copy link

jesmrec commented Dec 1, 2017

Steps to reproduce

  1. Create a new OAuth2 session:

screen shot 2017-12-01 at 13 16 14

  1. Remove the authorized OAuth2 token in webUI Security section

Expected behaviour

Sessions for this client/user agent are removed/invalidated as well

Current behaviour

Sessions keep alive, all request are reponsed with 200

screen shot 2017-12-01 at 13 17 02

Set up:

  • ownCloud 10.0.4 RC1 (daily) Build:2017-12-01T04:16:36+00:00 d4eab96823baec0061bc221e35b49dce968cbed9

  • OAuth 0.2.1

@jesmrec jesmrec added the bug label Dec 1, 2017
@jesmrec jesmrec changed the title OAuth2 token revoke does not expire sessions OAuth2 token revokation does not expire sessions Dec 1, 2017
@davitol davitol added this to the triage milestone Dec 4, 2017
@PVince81
Copy link
Contributor

PVince81 commented Dec 4, 2017

@DeepDiver1975 @pmaier1

@Dagefoerde
Copy link

Why do token-authenticated requests even create a session? I'd expect token-authenticated requests to be (rather) stateless, as the token is transmitted with every request. Therefore I think that such requests should not result in a session in the PHP sense at all, since that could be used to circumvent the token.

@DeepDiver1975
Copy link
Member

@Dagefoerde you are absolutely right - this is some technical dept about the session handling ....

@DeepDiver1975
Copy link
Member

ok - there is an issue in UserSession::verifyAuthHeaders - not all scenarios are covered .....

@SamuAlfageme
Copy link
Contributor

Yup, I'll suggest moving this to core since "client-associated-session revocation" does work out of the box. For individual session revocation (not yet supported) refer to #97

Core is both generating a valid cookie (like @Dagefoerde pointed out in #103 (comment)) and also accepting it (i.e. not ignoring) as authentication mechanism. A client sending both, an expired/revoked auth. token and this cookie therefore continues working normally. The same would apply for other stateless/token-based methods (e.g. app. passwords)...

@PVince81
Copy link
Contributor

So next step is owncloud/core#29779 ?

@jesmrec
Copy link
Author

jesmrec commented Dec 19, 2017

should be. At least, OAuth2 does not have to handle cookie sessions. The validity of the token should be enough to determine if the client is allowed to perform actions.

@michaelstingl
Copy link

@PVince81 quick-fix: just drop the sessions?

@PVince81
Copy link
Contributor

@DeepDiver1975 can we do a quick fix and drop the sessions only for oauth, not in a broader scope ? (the broader scope needs more discussion)

@PVince81
Copy link
Contributor

The code must likely be added here: https://github.com/owncloud/oauth2/blob/master/lib/Controller/SettingsController.php#L177

I had a look at the DB structure and it seems we don't have a mapping between OAuth token and sessions.
However it is likely that core's DefaultTokenProvider also stores an entry in "oc_authtoken" that uses the OAuth token as password. So this entry could be invalidated explicitly in the above code.

@SamuAlfageme
Copy link
Contributor

SamuAlfageme commented Feb 5, 2018

I'd set the expectations for this as "remove and invalidate the orphaned sessions created when using OAuth" as Auth8N method, as:

I'd expect token-authenticated requests to be (rather) stateless, as the token is transmitted with every request.

...is yet not possible considering the cookies are required for some core features to work IIRC (correct me if I'm wrong @DeepDiver1975)

Sessions keep alive, all request are replied with 200: Success

This is the part not reproducible as for today (10.0.6) - in contrast with the similar: owncloud/core#28553. We should recap everything sessions-related to work towards the same goal.

DeepDiver1975 added a commit to owncloud/core that referenced this issue Feb 7, 2018
…over all modules and allow each of them

to verify the headers and in case something is terribly wrong the user will be logged out and the session will be destroyed.

The BasicAuthModule now handles app passwords as well - this was missing before and caused users being locked out of ldap.

Solves OAuth token expiry issue - fixes owncloud/oauth2#103
DeepDiver1975 added a commit to owncloud/core that referenced this issue Feb 8, 2018
…over all modules and allow each of them

to verify the headers and in case something is terribly wrong the user will be logged out and the session will be destroyed.

The BasicAuthModule now handles app passwords as well - this was missing before and caused users being locked out of ldap.

Solves OAuth token expiry issue - fixes owncloud/oauth2#103
DeepDiver1975 added a commit to owncloud/core that referenced this issue Feb 12, 2018
…over all modules and allow each of them

to verify the headers and in case something is terribly wrong the user will be logged out and the session will be destroyed.

The BasicAuthModule now handles app passwords as well - this was missing before and caused users being locked out of ldap.

Solves OAuth token expiry issue - fixes owncloud/oauth2#103
DeepDiver1975 added a commit to owncloud/core that referenced this issue Feb 12, 2018
…over all modules and allow each of them

to verify the headers and in case something is terribly wrong the user will be logged out and the session will be destroyed.

The BasicAuthModule now handles app passwords as well - this was missing before and caused users being locked out of ldap.

Solves OAuth token expiry issue - fixes owncloud/oauth2#103
DeepDiver1975 added a commit to owncloud/core that referenced this issue Feb 12, 2018
…over all modules and allow each of them

to verify the headers and in case something is terribly wrong the user will be logged out and the session will be destroyed.

The BasicAuthModule now handles app passwords as well - this was missing before and caused users being locked out of ldap.

Solves OAuth token expiry issue - fixes owncloud/oauth2#103
DeepDiver1975 added a commit to owncloud/core that referenced this issue Feb 13, 2018
…over all modules and allow each of them

to verify the headers and in case something is terribly wrong the user will be logged out and the session will be destroyed.

The BasicAuthModule now handles app passwords as well - this was missing before and caused users being locked out of ldap.

Solves OAuth token expiry issue - fixes owncloud/oauth2#103
DeepDiver1975 added a commit to owncloud/core that referenced this issue Feb 13, 2018
…over all modules and allow each of them

to verify the headers and in case something is terribly wrong the user will be logged out and the session will be destroyed.

The BasicAuthModule now handles app passwords as well - this was missing before and caused users being locked out of ldap.

Solves OAuth token expiry issue - fixes owncloud/oauth2#103
DeepDiver1975 added a commit to owncloud/core that referenced this issue Feb 13, 2018
…ed to loop over all modules and allow each of them

to verify the headers and in case something is terribly wrong the user will be logged out and the session will be destroyed.

The BasicAuthModule now handles app passwords as well - this was missing before and caused users being locked out of ldap.

Solves OAuth token expiry issue - fixes owncloud/oauth2#103
@SamuAlfageme SamuAlfageme added invalid and removed bug labels Feb 13, 2018
@SamuAlfageme
Copy link
Contributor

Relabeling because of owncloud/core#30365 (comment) -> moved to mobile team backlog.

DeepDiver1975 added a commit to owncloud/core that referenced this issue Feb 13, 2018
…mechanisms we need to loop over all modules and allow each of them

to verify the headers and in case something is terribly wrong the user will be logged out and the session will be destroyed.

The BasicAuthModule now handles app passwords as well - this was missing before and caused users being locked out of ldap.

Solves OAuth token expiry issue - fixes owncloud/oauth2#103
@felixboehm felixboehm removed this from the triage milestone Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants