Introduce account modules #31547

butonic · 2018-05-28T12:11:38Z

This introduces Account Modules using password expiry as an example. Requires owncloud/password_policy#15

codecov · 2018-05-31T11:24:51Z

Codecov Report

Merging #31547 into master will increase coverage by <.01%.
The diff coverage is 60.68%.

@@             Coverage Diff              @@
##             master   #31547      +/-   ##
============================================
+ Coverage      63.4%    63.4%   +<.01%     
- Complexity    18461    18493      +32     
============================================
  Files          1162     1165       +3     
  Lines         69267    69367     +100     
  Branches       1264     1264              
============================================
+ Hits          43920    43984      +64     
- Misses        24978    25014      +36     
  Partials        369      369

Flag	Coverage Δ	Complexity Δ
#javascript	`52.58% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`64.65% <60.68%> (-0.01%)`	`18493 <28> (+32)`

Impacted Files	Coverage Δ	Complexity Δ
apps/dav/appinfo/v1/carddav.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
apps/dav/appinfo/v1/caldav.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
lib/private/legacy/util.php	`69.91% <0%> (-0.59%)`	`246 <0> (+1)`
apps/dav/appinfo/v1/webdav.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
lib/private/legacy/api.php	`41.15% <0%> (-1.05%)`	`85 <0> (+2)`
lib/private/legacy/json.php	`8.69% <0%> (-0.83%)`	`29 <1> (+2)`
apps/dav/lib/Server.php	`51.42% <100%> (+0.34%)`	`24 <0> (ø)`	⬇️
...e/AppFramework/DependencyInjection/DIContainer.php	`73.23% <100%> (+0.54%)`	`81 <0> (ø)`	⬇️
core/Middleware/AccountModuleMiddleware.php	`100% <100%> (ø)`	`10 <10> (?)`
lib/private/Server.php	`85.02% <100%> (+0.12%)`	`253 <2> (+2)`	⬆️
... and 7 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 70fbb2e...c29a99a. Read the comment docs.

butonic · 2018-06-12T07:58:38Z

~~register login hook to check account modules on webdav and ocs login (move code from password policy to core)~~
- no, the app providing the account module has to do this itself, because not all account modules may need to check something based on the postLogin event. That is only the case for the force password expiry. An OptInAccountModule eg does not need the password info of the post login event. It is invoked by the AccountModuleMiddleware. Same for a Force2FAAccountModule.
add dav plugin reproducing the middleware logic
- grepped where needsSecondFactor is used ... omg there is not only legacy api.php but also json.php and util.php 😱
- added account module check there as well 🙄
add middleware logic to ocs https://github.com/owncloud/core/blob/master/lib/private/legacy/api.php#L333
how do desktop and mobile clients handle forced password reset?
should we add a notification that they show the user
-> password policy, optional, subsequent PR
requires configuring forcing token based auth for clients
how should clients be affected if an auth module check fails?
- login should be denied? then all clients will pop up saying login denied.
  - We must show a message what the user should do now.
  - do the clients automatically sync again if login was denied
  - should we return service unavailable to clients?
check if we can use a redirect exception instead of IAccountModuleSteps
store order in db (appconfig witl json string array), when app is enabled add it to the end ... UI in different PR

PVince81 · 2018-06-15T14:15:42Z

@SamuAlfageme see questions about clients ^

DeepDiver1975 · 2018-06-18T06:59:01Z

apps/dav/lib/Connector/Sabre/Auth.php

+				try {
+					$this->accountModuleManager->check($user);
+				} catch (AccountCheckException $ex) {
+					throw new \Sabre\DAV\Exception\NotAuthenticated($ex->getMessage(), $ex->getCode(), $ex);


this will logout all clients of the user - maybe not what we want - 503 might be a better idea

DeepDiver1975 · 2018-06-18T06:59:49Z

apps/dav/lib/Connector/Sabre/Auth.php

@@ -236,9 +252,17 @@ private function auth(RequestInterface $request, ResponseInterface $response) {

 		$data = parent::check($request, $response);
 		if ($data[0] === true) {
+			$user = $this->userSession->getUser();


code duplication ...... just saying

extracted checkAccountModule($user)

DeepDiver1975 · 2018-06-18T07:00:22Z

apps/files/js/file-upload.js

@@ -297,6 +297,7 @@ OC.FileUpload.prototype = {
 			headers['OC-Total-Length'] = size;

 		}
+		headers['OC-LazyOps'] = true;


whot? wrong PR 😬

DeepDiver1975 · 2018-06-18T07:01:09Z

lib/private/AppFramework/DependencyInjection/DIContainer.php

@@ -381,12 +383,30 @@ public function __construct($appName, $urlParams = []) {
 			return new TwoFactorMiddleware($twoFactorManager, $userSession, $urlGenerator, $reflector, $request);
 		});

+		$this->registerService('AccountModuleManager', function ($c) use ($app) {


should not be necessary - DI magic will create it

DeepDiver1975 · 2018-06-18T07:01:27Z

lib/private/AppFramework/DependencyInjection/DIContainer.php

+			);
+		});
+
+		$this->registerService('AccountMiddleware', function (SimpleContainer $c) use ($app) {


same as above - DI magic ..

DeepDiver1975 · 2018-06-18T07:04:30Z

lib/private/legacy/api.php

+			try {
+				\OC::$server->getAccountModuleManager()->check($userSession->getUser());
+			} catch (AccountCheckException $ex) {
+				// Deny login if any IAuthModule check fails


logged by https://github.com/owncloud/core/pull/31547/files/1079de094343bf6769e158b741f63e0a92b28a2e#diff-81fc557ed3d19729d72efce4e07a71dcR129

DeepDiver1975 · 2018-06-18T07:04:52Z

lib/private/legacy/json.php

@@ -62,24 +63,40 @@ public static function checkAppEnabled($app) {
 		}
 	}

+	/**
+	 * FIXME this needs so much refactoring ...


no fixing - this is legacy - kill it

Too many places where it is still used -> not in this PR. That is why I added a FIXME

I know it's being used - my comment was about the need to kill this all instead of trying to fix this

DeepDiver1975 · 2018-06-18T07:05:54Z

lib/public/Authentication/IAccountModule.php

@@ -0,0 +1,28 @@
+<?php
+


copyright missing

DeepDiver1975 · 2018-06-18T07:06:15Z

lib/public/Authentication/IAccountModule.php

+ * Interface IAccountModule
+ *
+ * @package OCP\Authentication
+ * @since 10.0.9


10.0.10 - 10.0.9 is out

DeepDiver1975 · 2018-06-18T07:06:36Z

lib/public/Authentication/IAccountModuleController.php

@@ -0,0 +1,21 @@
+<?php
+/**
+ * Created by PhpStorm.


SamuAlfageme · 2018-06-18T10:41:26Z

@PVince81 @butonic re. client questions - need some basic instructions for trying this PoC out, I'm not quite familiar with password_policy

all addressed, please re review

DeepDiver1975 · 2018-06-25T08:09:20Z

apps/dav/lib/Connector/Sabre/Auth.php

+	 * @param $user
+	 * @throws ServiceUnavailable
+	 */
+	private function checkAccountModule($user): void {


return type will block backport - we can keep it this way but need to modify this on the backport then

DeepDiver1975 · 2018-06-25T08:28:16Z

core/Middleware/AccountModuleMiddleware.php

+	 * @param Controller $controller
+	 * @param string $methodName
+	 * @throws AccountCheckException
+	 * @throws \OCP\AppFramework\QueryException


not thrown ?

DeepDiver1975 · 2018-06-25T08:28:22Z

core/Middleware/AccountModuleMiddleware.php

+	 * @param string $methodName
+	 * @throws AccountCheckException
+	 * @throws \OCP\AppFramework\QueryException
+	 * @throws \OC\NeedsUpdateException


not thrown ?

DeepDiver1975 · 2018-06-25T08:29:30Z

lib/private/AppFramework/DependencyInjection/DIContainer.php

@@ -281,6 +283,7 @@ public function __construct($appName, $urlParams = []) {
 			return $this->getServer();
 		});
 		$this->registerAlias('OCP\\IServerContainer', 'ServerContainer');
+		$this->registerAlias(IServiceLoader::class, 'ServerContainer');


I moved this to the core server impl

DeepDiver1975 · 2018-06-25T08:33:11Z

lib/private/legacy/json.php

@@ -62,24 +63,40 @@ public static function checkAppEnabled($app) {
 		}
 	}

+	/**
+	 * FIXME this needs so much refactoring ...
+	 * @param $message


Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

PVince81 · 2018-06-25T12:09:48Z

@butonic please backport

DeepDiver1975 · 2018-06-25T12:12:49Z

@butonic please backport

#31883

lock · 2019-07-30T14:03:03Z

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.

butonic added enhancement 2 - Developing comp:authentication labels May 28, 2018

butonic self-assigned this May 28, 2018

butonic requested review from PVince81, tomneedham and DeepDiver1975 May 28, 2018 12:11

This was referenced May 28, 2018

password expiry using an IAccountModule owncloud/password_policy#15

Merged

[Concept] Introduce (Pluggable) Authentication Modules #31546

Closed

butonic force-pushed the feature/password-expiry branch from 5d10d2c to 40c6e44 Compare May 31, 2018 11:24

butonic force-pushed the feature/password-expiry branch 7 times, most recently from f4c3b39 to f752182 Compare June 4, 2018 14:16

ownclouders added the status/STALE label Jun 12, 2018

ownclouders removed the status/STALE label Jun 12, 2018

butonic force-pushed the feature/password-expiry branch 9 times, most recently from 62982d0 to 5c0c189 Compare June 14, 2018 13:13

butonic force-pushed the feature/password-expiry branch from 5c0c189 to 1079de0 Compare June 14, 2018 20:55

DeepDiver1975 previously requested changes Jun 18, 2018

View reviewed changes

butonic force-pushed the feature/password-expiry branch from 1079de0 to 2ea171e Compare June 22, 2018 13:51

butonic changed the title ~~[PoC] Introduce account modules~~ Introduce account modules Jun 22, 2018

butonic force-pushed the feature/password-expiry branch 2 times, most recently from 726de89 to 7683e0a Compare June 22, 2018 20:57

DeepDiver1975 reviewed Jun 25, 2018

View reviewed changes

DeepDiver1975 force-pushed the feature/password-expiry branch 2 times, most recently from fe6962f to 010fb79 Compare June 25, 2018 09:25

add IAccountModules and AccountModuleMiddleware

c29a99a

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

DeepDiver1975 force-pushed the feature/password-expiry branch from 46abb21 to c29a99a Compare June 25, 2018 11:08

DeepDiver1975 approved these changes Jun 25, 2018

View reviewed changes

DeepDiver1975 merged commit 35eb867 into master Jun 25, 2018

DeepDiver1975 deleted the feature/password-expiry branch June 25, 2018 12:05

PVince81 added this to the development milestone Jun 25, 2018

DeepDiver1975 mentioned this pull request Jun 25, 2018

[stable10] add IAccountModules and AccountModuleMiddleware #31883

Merged

DeepDiver1975 added 4 - To release and removed 2 - Developing labels Jun 25, 2018

ownclouders mentioned this pull request Jul 16, 2018

Account module infinite redirect with two factor challenge page #32059

Closed

lock bot locked as resolved and limited conversation to collaborators Jul 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce account modules #31547

Introduce account modules #31547

butonic commented May 28, 2018 •

edited

codecov bot commented May 31, 2018 •

edited

butonic commented Jun 12, 2018 •

edited

PVince81 commented Jun 15, 2018

DeepDiver1975 Jun 18, 2018

DeepDiver1975 Jun 18, 2018

butonic Jun 22, 2018

DeepDiver1975 Jun 18, 2018

DeepDiver1975 Jun 18, 2018

DeepDiver1975 Jun 18, 2018

DeepDiver1975 Jun 18, 2018

butonic Jun 22, 2018

DeepDiver1975 Jun 18, 2018

butonic Jun 22, 2018

DeepDiver1975 Jun 25, 2018

DeepDiver1975 Jun 18, 2018

DeepDiver1975 Jun 18, 2018

butonic Jun 25, 2018

DeepDiver1975 Jun 18, 2018

SamuAlfageme commented Jun 18, 2018

DeepDiver1975 Jun 25, 2018

DeepDiver1975 Jun 25, 2018

DeepDiver1975 Jun 25, 2018

DeepDiver1975 Jun 25, 2018

DeepDiver1975 Jun 25, 2018

PVince81 commented Jun 25, 2018

DeepDiver1975 commented Jun 25, 2018

lock bot commented Jul 30, 2019

Introduce account modules #31547

Introduce account modules #31547

Conversation

butonic commented May 28, 2018 • edited

codecov bot commented May 31, 2018 • edited

Codecov Report

butonic commented Jun 12, 2018 • edited

PVince81 commented Jun 15, 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

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

Choose a reason for hiding this comment

SamuAlfageme commented Jun 18, 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

PVince81 commented Jun 25, 2018

DeepDiver1975 commented Jun 25, 2018

lock bot commented Jul 30, 2019

butonic commented May 28, 2018 •

edited

codecov bot commented May 31, 2018 •

edited

butonic commented Jun 12, 2018 •

edited