Bugfix/owncloud/oauth2#103 #30365

DeepDiver1975 · 2018-02-05T11:32:43Z

Description

Related Issue

fixes owncloud/oauth2#103

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

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.

codecov · 2018-02-06T14:12:15Z

Codecov Report

Merging #30365 into master will increase coverage by 0.03%.
The diff coverage is 84.37%.

@@             Coverage Diff              @@
##             master   #30365      +/-   ##
============================================
+ Coverage     60.86%   60.89%   +0.03%     
- Complexity    18584    18597      +13     
============================================
  Files          1093     1093              
  Lines         61371    61405      +34     
============================================
+ Hits          37351    37393      +42     
+ Misses        24020    24012       -8

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Session/Internal.php	`0% <0%> (ø)`	`19 <0> (ø)`	⬇️
lib/private/User/Session.php	`62.21% <75%> (+3.45%)`	`144 <5> (+4)`	⬆️
lib/private/User/BasicAuthModule.php	`95.45% <91.66%> (+1.7%)`	`11 <0> (+2)`	⬆️
lib/private/User/TokenAuthModule.php	`92.1% <92.59%> (+2.1%)`	`15 <9> (+7)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5be225...80fe030. Read the comment docs.

PVince81 · 2018-02-06T15:52:47Z

lib/private/User/BasicAuthModule.php

-		$user = $this->manager->checkPassword($request->server['PHP_AUTH_USER'], $request->server['PHP_AUTH_PW']);
-		if ($user instanceof IUser) {
-			return $user;
+		// reuse logClientIn because this method


because this method... yes ?

PVince81 · 2018-02-06T15:54:10Z

lib/private/User/BasicAuthModule.php

@@ -66,6 +63,10 @@ public function getUserPassword(IRequest $request) {
 			return '';
 		}

+		if ($this->session->getSession()->exists('app_password')) {
+			// TODO: use proper password
+			throw new \Exception('Not implemented yet');


is this ever triggered ?

Not at the moment - this is a safety exception which will punch us in the face as soon as we use it accidentally.

Actually the whole method should not yet be called ... we could just throw the exception right away

PVince81 · 2018-02-06T15:55:30Z

lib/private/User/Session.php

+				$user = $module->auth($request);
+				if ($user !== null) {
+					if ($this->isLoggedIn() && $this->getUser()->getUID() !== $user->getUID()) {
+						throw new Exception();


would be nice to have a message, especially if we don't have a proper type.

no more auto-logout of bad sessions ?

these exceptions are basically just to jump out of the loop and do the logout in the catch block - I'm not to happy with this .....

PVince81 · 2018-02-06T15:58:51Z

As someone unfamiliar with this code, it would be nice to have a more detailed description that tells why you went to make the changes you made.

The commit says something about reordering auth modules but the PR does more than this: it seems to replace API calls with other ones and throw exceptions instead of returning. Are these latter changes part of the bugfix or are they more about cleaning up ?

PVince81

ok then 👍

PVince81 · 2018-02-07T12:00:17Z

looks good, yes

PVince81 · 2018-02-08T08:55:17Z

@SamuAlfageme can you retest auth issue with this ?

PVince81 · 2018-02-08T15:29:23Z

CI fails ? :-/

DeepDiver1975 · 2018-02-08T15:30:24Z

CI fails ? :-/

drone has network issues

DeepDiver1975 · 2018-02-08T15:30:41Z

jenkins has failing integration tests

PVince81 · 2018-02-08T15:39:31Z

the integration tests seem completely unrelated. trashbin ?! 🙈

PVince81 · 2018-02-08T15:40:28Z

oh wait... trashbin result says Failed asserting that 401 matches expected '200'..

Either we broke something or the test was passing by chance before

DeepDiver1975 · 2018-02-08T15:40:42Z

I better have a look ....

DeepDiver1975 · 2018-02-08T15:51:28Z

10:27:08     And logging in using web as "user1"                                                                   # FeatureContext::loggingInUsingWebAs()
10:27:08 [Thu Feb  8 10:27:08 2018] ::1:53702 [200]: /index.php/apps/files/
10:27:08 [Thu Feb  8 10:27:08 2018] ::1:53706 [401]: /index.php/apps/files_trashbin/ajax/list.php?dir=%2F
10:27:08     When as "user1" the file with original path "/renamed_shared/shared_file.txt" is restored             # FeatureContext::elementInTrashIsRestored()
10:27:08       Failed asserting that 401 matches expected '200'.
10:27:09     Then as "user1" the file with original path "/renamed_shared/shared_file.txt" does not exist in trash # FeatureContext::elementIsNotInTrashCheckingOriginalPath()
10:27:09     And user "user1" should see following elements                                                        # FeatureContext::checkElementList()
10:27:09       | /renamed_shared/                |
10:27:09       | /renamed_shared/shared_file.txt |

DeepDiver1975 · 2018-02-08T15:54:34Z

The requests as executed by the integration tests are not reusing the session cookies - which might have a side effect ...

PVince81 · 2018-02-12T11:10:10Z

lib/private/User/BasicAuthModule.php

 		}
-		return $this->manager->checkPassword($users[0]->getUID(), $request->server['PHP_AUTH_PW']);
+		throw new \Exception('Invalid credentials');


return false ? Throwing could cause other side effects...

in TokenAuthModule we return null, let's stay consistent

null is returned if there is no authentication header to be evaluated aka handled by this module.
exception is throws if credentials are wrong
user object is returned if creds are valid

But you are right - this logic seems to be missing in the token auch module ....

PVince81 · 2018-02-12T11:11:38Z

lib/private/User/TokenAuthModule.php

+	 * @return null|IToken
+	 */
+	private function getTokenForAppPassword(IRequest $request, &$token) {
+		if (empty($request->server['PHP_AUTH_USER']) || empty($request->server['PHP_AUTH_PW'])) {


not afraid of "0" users ?

@PVince81 as requested: fuck php

PVince81

see comments

…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 · 2018-02-13T09:29:00Z

@DeepDiver1975 initial tests suggest the problem no longer reproduces with this + owncloud/oauth2#112; great news! 🎉

Reduced drastically AccessToken's resetExpires() to bump expires every 20s
Authorized the Desktop client
Close the client during access_token validity period and wait 20s
Replay any successful request the client made previously
Observe 401 "Unauthorized" reply on the replayed request

Would a traversal unit test like this one make sense to be included in the OAuth2 test pool?

Will focus now on trying to reproduce owncloud/oauth2#103 on the mobile apps; with and without these patches.

SamuAlfageme · 2018-02-13T12:00:31Z

@DeepDiver1975 can you check if the scope of this patch can be extended to the old WebDAV endpoint?

After some more tests, I believe the original cause of owncloud/oauth2#103 is the logic for the old endpoint. Take these 2 PROPFINDs using OAuth2 as the auth8n mechanism:

First 2 requests (url ending on :4443) belong to a 10.0.6 "unpatched" server (1st. is iOS: old endpoint, 2nd. Desktop, new)
The following 2 are 10.0.6 + Bugfix/owncloud/oauth2#103 #30365 + Throw an exception if the Bearer header holds an invalid token oauth2#112

Replaying the iOS request any number of times results on 2XX, while desktop gets the correct 401 (after the token is expired; as described in my previous #30365 (comment))

SamuAlfageme · 2018-02-13T12:32:28Z

Hm. Forcing the use of the old endpoint on the Desktop client (setting env. OWNCLOUD_CHUNKING_NG=0) still results on 401 & token refresh.

Appart from the endpoints there are a few apparent (but not relevant) differences between iOS and Desktop PROPFINDS 😕

Desktop:

curl \
    -H 'Depth:0' \
    -H 'Authorization:Bearer VBvulBvRbhOpaA5BLkaLl7H7AdeatO5kwGyDNqfwYf2XRnsJTz0sEagLwgSuT3U3' \
    -H 'User-Agent:Mozilla/5.0 (Macintosh) mirall/2.4.0 (build 8836)' \
    -H 'Accept:*/*' \
    -H 'Content-Type:text/xml; charset=utf-8' \
    -H 'X-Request-ID:83fd38fa-395b-429e-a1be-80aad5fc9c32' \
    -H 'Cookie:oc_sessionPassphrase=fnebJi6XYVoOIsvNDYO9KMec7ZI8qYbD3m0D0kYi5UJhoXh%2FviUkj7hqrvuPkXpaFgzSW0UqemP4JIOjFUMjmR2tWh%2Bt1DjRkr8uXnypprsGZvRoy7EGWdSwGemssovV; oc3kizzp7f7d=plgcob981he5sjek79r7avjqp1' \
    -H 'Content-Length:147' \
    -H 'Connection:Keep-Alive' \
    -H 'Accept-Encoding:gzip, deflate' \
    -H 'Accept-Language:en-US,*' \
    -H 'Host:alfageme.solidgear.prv:4443' \
    -X PROPFIND 'https://alfageme.solidgear.prv:4443/remote.php/webdav/' \
    --data-binary '<?xml version="1.0" ?>
<d:propfind xmlns:d="DAV:">
  <d:prop>
    <d:quota-available-bytes />
    <d:quota-used-bytes />
  </d:prop>
</d:propfind>
'

iOS:

curl \
    -H 'Host:alfageme.solidgear.prv:4443' \
    -H 'Content-Type:application/xml' \
    -H 'Depth:1' \
    -H 'Accept:*/*' \
    -H 'Accept-Encoding:gzip, deflate' \
    -H 'Connection:keep-alive' \
    -H 'Cookie:oc3kizzp7f7d=6ds6rnc8mcl5jg31dkmdnpqli6; oc_sessionPassphrase=9sWpKyFrO8B59uXHo%2F4BRmHB3X2q6qJDtpS17D2Zx7BcFPp0s4yPdWiZBPqmzkmWvo4yoBW9XJ7%2BwzgtfCVWfmy%2F0TzJwWUVbROzy0LDGHFPM9543cKTkWnf6%2FqoFcB1' \
    -H 'User-Agent:Mozilla/5.0 (iOS) ownCloud-iOS/3.7.0' \
    -H 'Accept-Language:en-US;q=1, es-US;q=0.9, zh-Hant-US;q=0.8, en-GB;q=0.7, en;q=0.6' \
    -H 'Authorization:Bearer fVbLupKEkYi4Yr6tzALsAZlMZIjMDEkpjZXr5kcwYR6HMKvjDy0RQPoXS4dJ7QSl' \
    -H 'Content-Length:383' \
    -X PROPFIND 'https://alfageme.solidgear.prv:4443/remote.php/webdav/' \
    --data-binary '<?xml version="1.0" encoding="UTF-8"?><D:propfind xmlns:D="DAV:"><D:prop><D:resourcetype/><D:getlastmodified/><size xmlns="http://owncloud.org/ns"/><D:creationdate/><id xmlns="http://owncloud.org/ns"/><D:getcontentlength/><D:displayname/><D:quota-available-bytes/><D:getetag/><permissions xmlns="http://owncloud.org/ns"/><D:quota-used-bytes/><D:getcontenttype/></D:prop></D:propfind>'

Diffs:

Order of the Authorization and Cookie headers: shouldn't matter as WebDAV inherits HTTP's "The order in which header fields with differing field names are received is not significant"
Order of the Cookies itself: oc_sessionPassphrase first/last
Depth: 1/0
Content of the request itself

Will play with these differences and report back when I find the one that messes with the auth. middleware.

DeepDiver1975 · 2018-02-13T12:46:09Z

I honestly question if oauth works at all on the old endpoint .......

PVince81 · 2018-02-13T14:00:53Z

If we didn't break more stuff that wasn't broken before I'd suggest moving forward with the PR and get it released as it adresses the more critical issue. Then investigate this detail separately ?

@SamuAlfageme @DeepDiver1975

SamuAlfageme · 2018-02-13T17:10:05Z

As discussed with @DeepDiver1975 and @owncloud/ios-developers, in the end, owncloud/oauth2#103 was all about the iOS app circumventing the webview's sandbox (I haven't had the chance to check with Android's implementation, tho). This means the client was reusing cookies obtained during the authentication steps delegated to the browser (which goes against the end goal of the OAuth2 mechanism: isolation from this); those were bounded to the browser's activity; just like if they were obtained on safari or any other mobile browser, and valid as long as them (not dependent on the access_token at all).

After all the tests performed, all the basic access/refresh/revoke scenarios look sane enough. I'll go for the merge here. 👍

lock · 2019-08-01T03:02:34Z

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.

DeepDiver1975 added the 2 - Developing label Feb 5, 2018

DeepDiver1975 added this to the development milestone Feb 5, 2018

DeepDiver1975 self-assigned this Feb 5, 2018

DeepDiver1975 requested a review from SamuAlfageme February 5, 2018 11:32

DeepDiver1975 force-pushed the bugfix/owncloud/oauth2#103 branch from cadc772 to e838056 Compare February 6, 2018 14:12

This was referenced Feb 6, 2018

decouple token check from user sync #30386

Closed

Throw an exception if the Bearer header holds an invalid token owncloud/oauth2#112

Merged

PVince81 reviewed Feb 6, 2018

View reviewed changes

DeepDiver1975 force-pushed the bugfix/owncloud/oauth2#103 branch from e838056 to 84d3242 Compare February 7, 2018 11:56

PVince81 approved these changes Feb 7, 2018

View reviewed changes

This was referenced Feb 7, 2018

First we need to check the token auth module - otherwise we DOS the u… #30364

Merged

Authentication test plan owncloud/QA#529

Open

DeepDiver1975 force-pushed the bugfix/owncloud/oauth2#103 branch from 84d3242 to c06cbbc Compare February 8, 2018 08:32

PVince81 mentioned this pull request Feb 8, 2018

Ensure that the basic auth module is the last module in the list #30398

Merged

9 tasks

SamuAlfageme mentioned this pull request Feb 8, 2018

No need to handle authentication that early aka in here at all #30421

Merged

14 tasks

PVince81 added the p1-urgent Critical issue, need to consider hotfix with just that issue label Feb 8, 2018

DeepDiver1975 force-pushed the bugfix/owncloud/oauth2#103 branch 2 times, most recently from d4a9a90 to c90f461 Compare February 12, 2018 10:49

PVince81 reviewed Feb 12, 2018

View reviewed changes

DeepDiver1975 force-pushed the bugfix/owncloud/oauth2#103 branch 2 times, most recently from bbf4193 to ee3c057 Compare February 13, 2018 07:13

DeepDiver1975 added 2 commits February 13, 2018 08:50

destroy the session before starting a new one - prevents php warnings

80fe030

DeepDiver1975 force-pushed the bugfix/owncloud/oauth2#103 branch from ee3c057 to 80fe030 Compare February 13, 2018 07:50

PVince81 modified the milestones: development, QA Feb 13, 2018

DeepDiver1975 added 3 - To Review and removed 2 - Developing labels Feb 13, 2018

PVince81 assigned SamuAlfageme Feb 13, 2018

DeepDiver1975 merged commit 03ed016 into master Feb 13, 2018

DeepDiver1975 deleted the bugfix/owncloud/oauth2#103 branch February 13, 2018 17:11

DeepDiver1975 mentioned this pull request Feb 13, 2018

[stable10] Bugfix/owncloud/oauth2#103 #30479

Merged

This was referenced Feb 13, 2018

[OAuth2] Improve trust/security when login in embedded web view owncloud/android#2036

Closed

OAuth2 token revokation does not expire sessions owncloud/oauth2#103

Closed

DeepDiver1975 mentioned this pull request Feb 13, 2018

[hotfix-10.0.7] When verifying the request authentication … #30481

Merged

SamuAlfageme mentioned this pull request Feb 14, 2018

[OAuth2] Determine points where token refresh might not happen owncloud/client#6357

Closed

1 task

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

Bugfix/owncloud/oauth2#103 #30365

Bugfix/owncloud/oauth2#103 #30365

DeepDiver1975 commented Feb 5, 2018 •

edited

Loading

codecov bot commented Feb 6, 2018 •

edited

Loading

PVince81 Feb 6, 2018

PVince81 Feb 6, 2018

DeepDiver1975 Feb 7, 2018

PVince81 Feb 6, 2018

DeepDiver1975 Feb 7, 2018

PVince81 commented Feb 6, 2018

PVince81 left a comment

PVince81 commented Feb 7, 2018

PVince81 commented Feb 8, 2018

PVince81 commented Feb 8, 2018

DeepDiver1975 commented Feb 8, 2018

DeepDiver1975 commented Feb 8, 2018

PVince81 commented Feb 8, 2018

PVince81 commented Feb 8, 2018

DeepDiver1975 commented Feb 8, 2018

DeepDiver1975 commented Feb 8, 2018

DeepDiver1975 commented Feb 8, 2018

PVince81 Feb 12, 2018

PVince81 Feb 12, 2018

DeepDiver1975 Feb 12, 2018

PVince81 Feb 12, 2018

DeepDiver1975 Feb 12, 2018

PVince81 Feb 12, 2018

DeepDiver1975 Feb 12, 2018

PVince81 left a comment

SamuAlfageme commented Feb 13, 2018

SamuAlfageme commented Feb 13, 2018 •

edited

Loading

SamuAlfageme commented Feb 13, 2018 •

edited

Loading

DeepDiver1975 commented Feb 13, 2018

PVince81 commented Feb 13, 2018

SamuAlfageme commented Feb 13, 2018

lock bot commented Aug 1, 2019

Bugfix/owncloud/oauth2#103 #30365

Bugfix/owncloud/oauth2#103 #30365

Conversation

DeepDiver1975 commented Feb 5, 2018 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

codecov bot commented Feb 6, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Feb 6, 2018

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 commented Feb 7, 2018

PVince81 commented Feb 8, 2018

PVince81 commented Feb 8, 2018

DeepDiver1975 commented Feb 8, 2018

DeepDiver1975 commented Feb 8, 2018

PVince81 commented Feb 8, 2018

PVince81 commented Feb 8, 2018

DeepDiver1975 commented Feb 8, 2018

DeepDiver1975 commented Feb 8, 2018

DeepDiver1975 commented Feb 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

SamuAlfageme commented Feb 13, 2018

SamuAlfageme commented Feb 13, 2018 • edited Loading

SamuAlfageme commented Feb 13, 2018 • edited Loading

Desktop:

iOS:

Diffs:

DeepDiver1975 commented Feb 13, 2018

PVince81 commented Feb 13, 2018

SamuAlfageme commented Feb 13, 2018

lock bot commented Aug 1, 2019

DeepDiver1975 commented Feb 5, 2018 •

edited

Loading

codecov bot commented Feb 6, 2018 •

edited

Loading

SamuAlfageme commented Feb 13, 2018 •

edited

Loading

SamuAlfageme commented Feb 13, 2018 •

edited

Loading