[stable10] Missing oc_authtoken (session) for native clients #28879

IljaN · 2017-09-01T08:33:15Z

Description

oc_authtoken was not created for clients which use basic auth. (Probably everything, except browser and some dav-clients)

Related Issue

fixes #27845
fixes #28553
fixes #28881

Motivation and Context

This bug was introduced with commit f1dc2d9 - "Validate session only if we are not in a basic auth request".

Looking at the commit message the intent is not quite clear to me because the dektop client and all the mobile clients use basic auth at the moment. As the issue is relatively old I assume that some technical circumstances changed or that the validateSesion method is badly named because basic auth is technically not a persistent session trough requests. With this assumption the intent of the commit makes sense.

How Has This Been Tested?

Manually

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

IljaN · 2017-09-01T08:47:11Z

@phisch / @DeepDiver1975: Is it possible to move that session check to somwhere else? Was suprised when I saw session handling code in a method called "loadApps" ;-)))

SamuAlfageme · 2017-09-01T10:58:24Z

@IljaN cool, now I'm getting all the sessions logged back in the webUI 🎉

Revoking though, (using the 'Delete' button next to them) is still not working (i.e. clients using basic auth are still able to sync and keep their session - it spawns in the table on next client request). So #27845 might be deeper. Also I've noticed after removing a browser session (from a different one) and hitting:

curl 'https://<server>/index.php/settings/personal/authtokens' \
    -H 'OCS-APIREQUEST: true' \
    -H 'Cookie: $SESSION'

Server replies with 503. The rest of the endpoints is fine. No exception is raised in the logs.

SamuAlfageme · 2017-09-01T11:40:15Z

Described that one issue in #28881 as this one was only for the regression display.

PVince81 · 2017-09-01T13:32:44Z

From my understanding from last time I looked at similar code, it seemed that there was a difference between basic auth and DAV authentication (still not sure where this is happening).

If we consider that both are two different things, then the old commit from @DeepDiver1975 was correct as it was only validating the session if we're NOT dealing with DAV authentication, so $davUser is null, in which case we're dealing with basic auth, not DAV auth.

The original issue though was that sessions did not appear in the list. Is there some piece of code inside of validateSession which we could extract and use here to make sure the session entry will be updated without validating ?

Now the other question would be whether it's really that bad to validate here additionally ?

Another concern: since you reversed the condition, it is likely that now basic auth is not getting validated any more. Maybe the if part should be removed completely ?

IljaN · 2017-09-01T14:01:33Z

In my tests $davUser was always set using the sync client so go figure... Web interface does not go trough this codepath at all.

I also had the idea to pull the token update out but the code is buried in a private tokenProvider object inside session...

Yeah it would probably be the safest bet to always validate, basically reverting @DeepDiver1975 `s change.

IljaN · 2017-09-01T14:04:03Z

Oh look what i found:

core/lib/private/User/Session.php

Lines 438 to 448 in f8e2940

    
           					/** 
        
           					 * Add DAV authenticated. This should in an ideal world not be 
        
           					 * necessary but the iOS App reads cookies from anywhere instead 
        
           					 * only the DAV endpoint. 
        
           					 * This makes sure that the cookies will be valid for the whole scope 
        
           					 * @see https://github.com/owncloud/core/issues/22893 
        
           					 */ 
        
           					$this->session->set( 
        
           						Auth::DAV_AUTHENTICATED, $this->getUser()->getUID() 
        
           					); 
        
           					return true;

PVince81 · 2017-09-01T14:04:08Z

It is possible that "DAV auth" means "basic auth done on a Webdav endpoint"

PVince81 · 2017-09-04T17:19:07Z

settings/Controller/AuthSettingsController.php

 		$user = $this->userManager->get($this->uid);
+		$currentToken = $this->tokenProvider->getToken($this->session->getId());
+
+		if ($currentToken->getId() == $id) {


=== ? See https://doc.owncloud.org/server/10.0/developer_manual/general/codingguidelines.html#operators

PVince81 · 2017-09-04T17:21:06Z

If validating in all cases isn't causing any other side effects, then I'm fine with this.

Needs to be retested at best will most authentication schemes:

basic auth on Webdav
basic auth on OCS
web UI session
shibboleth ?
auth with application token instead of password
oauth2

IljaN · 2017-09-05T09:25:08Z

Will figure out how to test most of these, not sure:
...how to setup/test shibboleth and oauth2
... what exactly OCS is.

@phisch, can you help?

PVince81 · 2017-09-05T09:26:11Z

@IljaN get help from QA or @SamuAlfageme for testing these cases

SamuAlfageme · 2017-09-05T11:08:56Z

Some results after some quick checks:

✅ basic auth on Webdav
✅ basic auth on OCS
✅ web UI session
❌ shibboleth
✅ auth with application token instead of password
✅ oauth2

About Shib, I'm not sure if the sessions ever appeared in this section (or they have a separate pane somewhere; kind of like it happens with OAuth)

Here ya go with a normal SAML request if that can help out determining if something is missing:

SAML-Authenticated PROPFIND

curl \
-H 'Cookie:oc_sessionPassphrase=7hZcZNUIb3AaiNocC%2BIXh5aAYM36%2F3R1WG6i6OltcbqdYsJhHTorkR8%2BgQcmTZL7ax7CuavRwJL4GpNb2uBGPjEN2LsM2aMZ58pVCHlMzu5bMMkjzuPAvyNhwr6dgajQ; ocgwv1nxeve8=iq8npgptet00ocj15qtc2vr4g6; _shibsession_64656661756c7468747470733a2f2f646f636b65722e6f632e736f6c6964676561722e65733a31303237312f6f632d73686962=_58f8ec23b20e1ac03e7d389bad9d7197' \
-H 'Content-Length:114' \
 -X PROPFIND 'https://<server>/<shib-path>/remote.php/dav/files/alterego@testshib.org/' --data-binary '<?xml version="1.0" ?>
<d:propfind xmlns:d="DAV:">
  <d:prop>
    <d:getlastmodified />
  </d:prop>
</d:propfind>
'

@IljaN maybe we can also get the _shibsession_*=_*'s logged in the session table somehow. I don't really know the specifics about it maybe @butonic or @DeepDiver1975 can help here.

@PVince81 about

basic auth on OCS

I'm getting the "last activity" time updated from ocs calls, so I'd say that one works as expected as well. (they don't and I guess shouldn't originate new entries)

PVince81 · 2017-09-05T11:17:04Z

@SamuAlfageme for shib, try with shib and the latest OC version before the regression to find out whether it ever worked before.

PVince81 · 2017-09-06T19:38:38Z

lots of failing integration tests, like "auth.access files app with basic auth"

IljaN · 2017-09-07T16:40:38Z

All tests should pass now 🤞
Altough the code feels like a hack...

IljaN · 2017-09-08T13:33:28Z

@SamuAlfageme All tests are green now, please retest.

IljaN · 2017-11-08T12:30:04Z

@davitol I can confirm that cadavre session is not displayed... investigating

IljaN · 2017-11-08T16:23:29Z

@davitol We figured out that cadaver does not support cookies thus doing basic auth on every request. This is why nothing is displayed in the sessions view because there is technically no session established. This would explain why Cyberduck is displayed (it probably supports cookies)

After discussing with @DeepDiver1975 the expected behavior is that nothing is displayed for clients which do not support cookies.

If we would "fix" that, we would have a entry for every request made by cadaver.

As for the iOS issue I need to find someone with a iPhone to be able to debug this...

PS: I will try to replicate the issue with Android

davitol · 2017-11-08T16:40:02Z

PS: I will try to replicate the issue with Android

@IljaN With Android it went fine for me

jesmrec · 2017-11-09T12:06:00Z

@IljaN the point is the way the iOS client is handling with the cookies. Some request are including authorization basic header but no cookies, therefore the server answers with a new cookie, and i guess that in this case a new row is included there. We will take care

PVince81 · 2017-11-09T15:06:57Z

@jesmrec so can we merge this or will this require modifications of the PR code ?

jesmrec · 2017-11-09T20:00:12Z

@PVince81 you can merge

PVince81 · 2017-11-10T07:04:57Z

@jesmrec thanks.

Now for a last CI battle then we can merge.

Current token must be not null Generate auth token for http_basic_auth logins Cleanup

PVince81 · 2017-11-10T11:15:00Z

Rebased for CI, hopefully for higher timeout values

PVince81 · 2017-11-10T13:48:45Z

codecov errors about the legacy app code, passing on this one 🙈

PVince81 · 2017-11-10T13:49:00Z

@IljaN please forward port to master

lock · 2019-08-01T23:02:54Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

IljaN requested review from DeepDiver1975, PVince81 and phisch September 1, 2017 08:33

IljaN assigned phisch Sep 1, 2017

SamuAlfageme mentioned this pull request Sep 1, 2017

Session can remove itself and does not really invalidate the session for all endpoints #28881

Closed

IljaN self-assigned this Sep 1, 2017

SamuAlfageme mentioned this pull request Sep 1, 2017

"Disconnect" application password should kill its open sessions #28553

Closed

PVince81 added the 3 - To Review label Sep 1, 2017

PVince81 added this to the development milestone Sep 1, 2017

PVince81 added the backport-request label Sep 1, 2017

IljaN force-pushed the missing_non_browser_authtoken branch from 14af487 to a0330c7 Compare September 4, 2017 10:27

IljaN requested a review from SamuAlfageme September 4, 2017 12:51

PVince81 reviewed Sep 4, 2017

View reviewed changes

IljaN force-pushed the missing_non_browser_authtoken branch from 8f39c1d to ed85adc Compare September 6, 2017 08:36

IljaN force-pushed the missing_non_browser_authtoken branch from 99d1df6 to 6ea527b Compare September 7, 2017 16:34

IljaN force-pushed the missing_non_browser_authtoken branch from 1967464 to ac7057d Compare November 8, 2017 15:48

PVince81 added 4 - To release and removed 3 - To Review labels Nov 10, 2017

IljaN added 2 commits November 10, 2017 12:14

Fixing missing oc_authtoken for native clients #27845

03b0bdb

Current token must be not null Generate auth token for http_basic_auth logins Cleanup

Better code formating

de3597a

PVince81 force-pushed the missing_non_browser_authtoken branch from ac7057d to de3597a Compare November 10, 2017 11:14

PVince81 merged commit dc57c65 into stable10 Nov 10, 2017

PVince81 deleted the missing_non_browser_authtoken branch November 10, 2017 13:48

IljaN mentioned this pull request Nov 10, 2017

From [stable10] missing non browser auth token #29533

Merged

PVince81 mentioned this pull request Nov 13, 2017

Sessions of Desktop, iOS and Android clients should be shown #27845

Closed

SergioBertolinSG mentioned this pull request Nov 28, 2017

Testing 10.0.4RC1 / RC2 Changelog #29697

Closed

87 tasks

ownclouders added the status/STALE label Dec 20, 2017

PVince81 removed the backport-request label Jan 8, 2018

ownclouders removed the status/STALE label Jan 8, 2018

tomneedham mentioned this pull request May 3, 2018

Remove session token creation for dav requests during loadApps #31343

Merged

9 tasks

lock bot locked as resolved and limited conversation to collaborators Aug 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stable10] Missing oc_authtoken (session) for native clients #28879

[stable10] Missing oc_authtoken (session) for native clients #28879

IljaN commented Sep 1, 2017 •

edited

Loading

IljaN commented Sep 1, 2017

SamuAlfageme commented Sep 1, 2017

SamuAlfageme commented Sep 1, 2017

PVince81 commented Sep 1, 2017

IljaN commented Sep 1, 2017

IljaN commented Sep 1, 2017 •

edited

Loading

PVince81 commented Sep 1, 2017

PVince81 Sep 4, 2017

PVince81 commented Sep 4, 2017

IljaN commented Sep 5, 2017 •

edited

Loading

PVince81 commented Sep 5, 2017

SamuAlfageme commented Sep 5, 2017 •

edited

Loading

PVince81 commented Sep 5, 2017

PVince81 commented Sep 6, 2017 •

edited

Loading

IljaN commented Sep 7, 2017 •

edited

Loading

IljaN commented Sep 8, 2017

IljaN commented Nov 8, 2017

IljaN commented Nov 8, 2017 •

edited

Loading

davitol commented Nov 8, 2017

jesmrec commented Nov 9, 2017

PVince81 commented Nov 9, 2017

jesmrec commented Nov 9, 2017

PVince81 commented Nov 10, 2017

PVince81 commented Nov 10, 2017

PVince81 commented Nov 10, 2017

PVince81 commented Nov 10, 2017

lock bot commented Aug 1, 2019

[stable10] Missing oc_authtoken (session) for native clients #28879

[stable10] Missing oc_authtoken (session) for native clients #28879

Conversation

IljaN commented Sep 1, 2017 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

IljaN commented Sep 1, 2017

SamuAlfageme commented Sep 1, 2017

SamuAlfageme commented Sep 1, 2017

PVince81 commented Sep 1, 2017

IljaN commented Sep 1, 2017

IljaN commented Sep 1, 2017 • edited Loading

PVince81 commented Sep 1, 2017

PVince81 Sep 4, 2017

Choose a reason for hiding this comment

PVince81 commented Sep 4, 2017

IljaN commented Sep 5, 2017 • edited Loading

PVince81 commented Sep 5, 2017

SamuAlfageme commented Sep 5, 2017 • edited Loading

PVince81 commented Sep 5, 2017

PVince81 commented Sep 6, 2017 • edited Loading

IljaN commented Sep 7, 2017 • edited Loading

IljaN commented Sep 8, 2017

IljaN commented Nov 8, 2017

IljaN commented Nov 8, 2017 • edited Loading

davitol commented Nov 8, 2017

jesmrec commented Nov 9, 2017

PVince81 commented Nov 9, 2017

jesmrec commented Nov 9, 2017

PVince81 commented Nov 10, 2017

PVince81 commented Nov 10, 2017

PVince81 commented Nov 10, 2017

PVince81 commented Nov 10, 2017

lock bot commented Aug 1, 2019

IljaN commented Sep 1, 2017 •

edited

Loading

IljaN commented Sep 1, 2017 •

edited

Loading

IljaN commented Sep 5, 2017 •

edited

Loading

SamuAlfageme commented Sep 5, 2017 •

edited

Loading

PVince81 commented Sep 6, 2017 •

edited

Loading

IljaN commented Sep 7, 2017 •

edited

Loading

IljaN commented Nov 8, 2017 •

edited

Loading