Introduce UserSession::verifyAuthHeaders which validates the session … #28733

DeepDiver1975 · 2017-08-17T16:14:26Z

…against given auth headers like basic auth or oauth

Description

Related Issue

#28707

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.

SamuAlfageme · 2017-09-13T13:56:21Z

@DeepDiver1975 is this still a WIP?

DeepDiver1975 · 2017-09-13T14:12:40Z

@DeepDiver1975 is this still a WIP?

yes still WIP - will be back on this after the conference ...

michaelstingl · 2017-10-25T14:25:03Z

@PVince81 ping

PVince81 · 2017-10-25T14:46:47Z

@DeepDiver1975 any chance to finish this for 10.0.4 ? It is a blocker for all clients. Without this, they all need to build in ugly workarounds.

PVince81 · 2017-10-26T12:02:22Z

@DeepDiver1975 please post a list of remaining tasks/issues so someone else can take over

DeepDiver1975 · 2017-10-27T06:03:51Z

@DeepDiver1975 please post a list of remaining tasks/issues so someone else can take over

test if this is already enough to fullfill the requirement
make sure nothing else was broken

PVince81 · 2017-10-30T11:42:37Z

test if this is already enough to fullfill the requirement

@michaelstingl @SamuAlfageme

michaelstingl · 2017-10-30T11:45:30Z

@jesmrec @nasli @davigonz Please check with mobile platforms ^

DeepDiver1975 · 2017-10-30T16:50:58Z

god damn - my latest changes of today are resulting in an infinite loop ..... 💥

nasli · 2017-10-31T11:54:05Z

👍 @DeepDiver1975 ping us again when is ready to test

DeepDiver1975 · 2017-11-02T11:23:15Z

👍 @DeepDiver1975 ping us again when is ready to test

@nasli ping 😉

butonic · 2017-11-02T11:25:19Z

lib/private/App/AppManager.php

@@ -124,10 +124,10 @@ public function getInstalledApps() {
 	/**
 	 * List all apps enabled for a user
 	 *
-	 * @param \OCP\IUser $user
+	 * @param \OCP\IUser|null $user


hm then checkAppForUser in line 165 also needs te allow null in the phpdocs

nasli · 2017-11-03T08:01:07Z

Great!! Thanks @DeepDiver1975, spoiler! Jesus has made first tests and seems to work fine, we let you know

jesmrec · 2017-11-03T09:09:08Z

ping us when it is (finally) ready.

PVince81 · 2017-11-06T09:31:28Z

so it seems the code is ready apart from the PHPDoc comment ?

this checkbox item still requires attention:

make sure nothing else was broken

What needs to be retested ? Everything auth related, all auth types, shibboleth, etc ?

PVince81 · 2017-11-07T10:52:24Z

@davitol is going to try and break this with different auth types.

@DeepDiver1975 assuming this is done apart from the PHPDoc issue

davitol · 2017-11-07T15:01:58Z

Tests results:

❌  basic auth on Webdav: It is not listed. It is needed a forwardport of https://github.com/owncloud/core/pull/28879

✅ web UI session

✅ auth with application token instead of password

✅ oauth2

DeepDiver1975 · 2017-11-09T11:37:50Z

@PVince81 this is for 10.0.4 - right? I need to adjust the since annotation then

PVince81 · 2017-11-09T11:43:21Z

@DeepDiver1975 yes please, thanks

codecov · 2017-11-09T13:25:08Z

Codecov Report

Merging #28733 into master will increase coverage by 0.01%.
The diff coverage is 57.4%.

@@             Coverage Diff              @@
##             master   #28733      +/-   ##
============================================
+ Coverage     61.37%   61.39%   +0.01%     
- Complexity    17436    17472      +36     
============================================
  Files          1042     1044       +2     
  Lines         57630    57695      +65     
============================================
+ Hits          35368    35419      +51     
- Misses        22262    22276      +14

Impacted Files	Coverage Δ	Complexity Δ
core/Application.php	`27% <0%> (-0.73%)`	`2 <0> (ø)`
lib/private/legacy/api.php	`41.7% <0%> (-0.18%)`	`81 <0> (+1)`
lib/base.php	`2.68% <0%> (-0.01%)`	`156 <0> (+2)`
ocs/v1.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
lib/private/App/AppManager.php	`84.31% <100%> (ø)`	`54 <1> (ø)`	⬇️
...e/AppFramework/DependencyInjection/DIContainer.php	`65.13% <100%> (+0.45%)`	`15 <0> (ø)`	⬇️
apps/dav/lib/Connector/Sabre/Auth.php	`94.04% <100%> (+0.07%)`	`35 <0> (+1)`	⬆️
lib/public/User.php	`30% <100%> (ø)`	`10 <0> (ø)`	⬇️
core/Controller/AvatarController.php	`90.57% <100%> (ø)`	`31 <0> (ø)`	⬇️
lib/private/User/Session.php	`62.13% <11.53%> (-1.02%)`	`124 <10> (+5)`
... and 6 more

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 91ffee2...719b5f6. Read the comment docs.

PVince81 · 2017-11-09T13:42:28Z

lib/public/App/IServiceLoader.php

+use OCP\IUser;
+
+/**
+ * Interface IServiceLoader


PHPDoc... what's a service loader, what's a service, etc.

PVince81 · 2017-11-09T13:42:50Z

lib/public/App/IServiceLoader.php

+	 * @return \Generator
+	 * @since 10.0.4
+	 */
+	public function load(array $xmlPath, IUser $user = null);


PHPDoc, what are we loading here ? People looking at the docs of \OC::$server::load will wonder

PVince81

Please add missing PHPDoc then merge and backport 👍

…nfo xmls

…against given auth headers like basic auth or oauth

phil-davis · 2017-11-10T01:52:39Z

Backport stable10 #29525

PVince81 · 2018-02-02T12:45:35Z

Regression: #30157

lock · 2019-08-01T08:01:57Z

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 Aug 17, 2017

DeepDiver1975 added this to the development milestone Aug 17, 2017

felixboehm mentioned this pull request Aug 29, 2017

0.2.0 Testplan owncloud/oauth2#63

Closed

8 tasks

PVince81 mentioned this pull request Aug 30, 2017

Inconsistency between OAuth2 access token and session cookie expirations #28707

Closed

SamuAlfageme mentioned this pull request Aug 31, 2017

Improve non-registered client workflows owncloud/oauth2#77

Open

phisch self-requested a review September 6, 2017 14:31

PVince81 modified the milestones: 10.0.3, development Sep 18, 2017

DeepDiver1975 mentioned this pull request Oct 5, 2017

Lack of 401 when token is expired in iOS client owncloud/oauth2#89

Closed

DeepDiver1975 force-pushed the verify-session branch 2 times, most recently from 022145e to 2f66e0e Compare October 30, 2017 09:14

DeepDiver1975 force-pushed the verify-session branch from 2f66e0e to b6e6eab Compare November 2, 2017 09:59

butonic reviewed Nov 2, 2017

View reviewed changes

PVince81 mentioned this pull request Nov 7, 2017

Release checklist 10.0.4 RC1 #29468

Closed

22 tasks

owncloud deleted a comment from codecov bot Nov 8, 2017

DeepDiver1975 force-pushed the verify-session branch from bc662ae to b886660 Compare November 9, 2017 11:12

DeepDiver1975 changed the title ~~[WIP] Introduce UserSession::verifyAuthHeaders which validates the session …~~ Introduce UserSession::verifyAuthHeaders which validates the session … Nov 9, 2017

DeepDiver1975 added 3 - To Review and removed 2 - Developing labels Nov 9, 2017

DeepDiver1975 force-pushed the verify-session branch from b886660 to c024a41 Compare November 9, 2017 11:58

owncloud deleted a comment from codecov bot Nov 9, 2017

PVince81 reviewed Nov 9, 2017

View reviewed changes

PVince81 approved these changes Nov 9, 2017

View reviewed changes

DeepDiver1975 added 3 commits November 9, 2017 15:10

Introduce IServiceLoader - helps to load services as defined in app i…

2e1a269

…nfo xmls

Introduce UserSession::verifyAuthHeaders which validates the session …

96d4643

…against given auth headers like basic auth or oauth

Fixing avatar null user folder ....

719b5f6

DeepDiver1975 force-pushed the verify-session branch from c024a41 to 719b5f6 Compare November 9, 2017 14:11

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

DeepDiver1975 merged commit 06e0710 into master Nov 9, 2017

DeepDiver1975 deleted the verify-session branch November 9, 2017 16:20

DeepDiver1975 mentioned this pull request Nov 9, 2017

[stable10] Introduce UserSession::verifyAuthHeaders which validates t… #29525

Merged

DeepDiver1975 removed the backport-request label Nov 9, 2017

PVince81 mentioned this pull request Nov 13, 2017

Cannot impersonate with stable10 and verifyAuthHeaders change owncloud/impersonate#74

Closed

ownclouders added the status/STALE label Dec 20, 2017

ownclouders removed the status/STALE label Feb 2, 2018

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

Introduce UserSession::verifyAuthHeaders which validates the session … #28733

Introduce UserSession::verifyAuthHeaders which validates the session … #28733

DeepDiver1975 commented Aug 17, 2017 •

edited

Loading

SamuAlfageme commented Sep 13, 2017

DeepDiver1975 commented Sep 13, 2017

michaelstingl commented Oct 25, 2017

PVince81 commented Oct 25, 2017

PVince81 commented Oct 26, 2017

DeepDiver1975 commented Oct 27, 2017 •

edited by davitol

Loading

PVince81 commented Oct 30, 2017

michaelstingl commented Oct 30, 2017

DeepDiver1975 commented Oct 30, 2017

nasli commented Oct 31, 2017

DeepDiver1975 commented Nov 2, 2017

butonic Nov 2, 2017

DeepDiver1975 Nov 2, 2017

nasli commented Nov 3, 2017

jesmrec commented Nov 3, 2017

PVince81 commented Nov 6, 2017

PVince81 commented Nov 7, 2017

davitol commented Nov 7, 2017 •

edited

Loading

DeepDiver1975 commented Nov 9, 2017

PVince81 commented Nov 9, 2017

codecov bot commented Nov 9, 2017 •

edited

Loading

PVince81 Nov 9, 2017

PVince81 Nov 9, 2017

PVince81 left a comment

phil-davis commented Nov 10, 2017

PVince81 commented Feb 2, 2018

lock bot commented Aug 1, 2019

Introduce UserSession::verifyAuthHeaders which validates the session … #28733

Introduce UserSession::verifyAuthHeaders which validates the session … #28733

Conversation

DeepDiver1975 commented Aug 17, 2017 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

SamuAlfageme commented Sep 13, 2017

DeepDiver1975 commented Sep 13, 2017

michaelstingl commented Oct 25, 2017

PVince81 commented Oct 25, 2017

PVince81 commented Oct 26, 2017

DeepDiver1975 commented Oct 27, 2017 • edited by davitol Loading

PVince81 commented Oct 30, 2017

michaelstingl commented Oct 30, 2017

DeepDiver1975 commented Oct 30, 2017

nasli commented Oct 31, 2017

DeepDiver1975 commented Nov 2, 2017

butonic Nov 2, 2017

Choose a reason for hiding this comment

DeepDiver1975 Nov 2, 2017

Choose a reason for hiding this comment

nasli commented Nov 3, 2017

jesmrec commented Nov 3, 2017

PVince81 commented Nov 6, 2017

PVince81 commented Nov 7, 2017

davitol commented Nov 7, 2017 • edited Loading

DeepDiver1975 commented Nov 9, 2017

PVince81 commented Nov 9, 2017

codecov bot commented Nov 9, 2017 • edited Loading

Codecov Report

PVince81 Nov 9, 2017

Choose a reason for hiding this comment

PVince81 Nov 9, 2017

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

phil-davis commented Nov 10, 2017

PVince81 commented Feb 2, 2018

lock bot commented Aug 1, 2019

DeepDiver1975 commented Aug 17, 2017 •

edited

Loading

DeepDiver1975 commented Oct 27, 2017 •

edited by davitol

Loading

davitol commented Nov 7, 2017 •

edited

Loading

codecov bot commented Nov 9, 2017 •

edited

Loading