Skip session validation during app load (Fixes #20756) #20757

sshambar · 2020-04-30T23:33:59Z

Adds parameter to User\Session:getUser() to skip validation, and
modifies OC_App::getEnabledApps() to use this parameter to skip
session validation when loading apps.

see #20756 for a detailed explanation

Signed-off-by: Scott Shambarger devel@shambarger.net

fixes #20756
fixes nextcloud/user_external#101
fixes nextcloud/user_external#70
fixes nextcloud/user_external#3

Adds parameter to User\Session:getUser() to skip validation, and modifies OC_App::getEnabledApps() to use this parameter to skip session validation when loading apps. Signed-off-by: Scott Shambarger <devel@shambarger.net>

kesselb · 2020-05-03T13:24:37Z

Thank you very much for looking into that issue 👍

Make sense to me. I'm not sure about the patch yet. A flag to skip the session validation at a certain stage seems a workaround. As you already pointed out that's an issue with the app loading order.

\OC::$server->getUserSession()->getUser() is necessary for getEnabledAppsForUser. The purpose is to load apps enabled for the current user. But it's not possible to limit authentication apps to certain groups hence the authentication apps is always loaded.

That means for loading authentication apps there is no dependency to \OC::$server->getUserSession()->getUser(). Probably some authentication apps itself have a dependency to getUserSession but that's another story.

I think we should introduce a dedicated method to load the authentication apps. Added a small POC but needs more work and testing.

POC

Index: ocs/v1.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- ocs/v1.php	(revision 29b4246840f298c03983457c99f4aa181f644b75)
+++ ocs/v1.php	(date 1588510049791)
@@ -51,7 +51,7 @@
  */
 try {
 	OC_App::loadApps(['session']);
-	OC_App::loadApps(['authentication']);
+	OC_App::loadAuthenticationApps();
 	// load all apps to get all api routes properly setup
 	OC_App::loadApps();
 
Index: lib/private/legacy/OC_App.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- lib/private/legacy/OC_App.php	(revision 29b4246840f298c03983457c99f4aa181f644b75)
+++ lib/private/legacy/OC_App.php	(date 1588510219400)
@@ -94,6 +94,32 @@
 		return in_array($app, self::$loadedApps, true);
 	}
 
+	/**
+	 * Loads all authentication apps
+	 *
+	 * @return bool
+	 */
+	public static function loadAuthenticationApps(): bool {
+		if ((bool)\OC::$server->getSystemConfig()->getValue('maintenance', false)) {
+			return false;
+		}
+
+		$apps = array_filter(\OC::$server->getAppManager()->getInstalledApps(), static function ($app) {
+			return self::isType($app, ['authentication']);
+		});
+
+		// prevent app.php from printing output
+		ob_start();
+		foreach ($apps as $app) {
+			if (!in_array($app, self::$loadedApps, true)) {
+				self::loadApp($app);
+			}
+		}
+		ob_end_clean();
+
+		return true;
+	}
+
 	/**
 	 * loads all apps
 	 *
Index: remote.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- remote.php	(revision 29b4246840f298c03983457c99f4aa181f644b75)
+++ remote.php	(date 1588509913770)
@@ -148,7 +148,7 @@
 
 	// Load all required applications
 	\OC::$REQUESTEDAPP = $app;
-	OC_App::loadApps(['authentication']);
+	OC_App::loadAuthenticationApps();
 	OC_App::loadApps(['filesystem', 'logging']);
 
 	switch ($app) {
Index: lib/base.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- lib/base.php	(revision 29b4246840f298c03983457c99f4aa181f644b75)
+++ lib/base.php	(date 1588509960366)
@@ -982,7 +982,7 @@
 		}
 
 		// Always load authentication apps
-		OC_App::loadApps(['authentication']);
+		OC_App::loadAuthenticationApps();
 
 		// Load minimum set of apps
 		if (!\OCP\Util::needUpgrade()
Index: lib/private/Updater.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- lib/private/Updater.php	(revision 29b4246840f298c03983457c99f4aa181f644b75)
+++ lib/private/Updater.php	(date 1588509974014)
@@ -262,7 +262,7 @@
 		$this->upgradeAppStoreApps($autoDisabledApps, true);
 
 		// install new shipped apps on upgrade
-		OC_App::loadApps(['authentication']);
+		OC_App::loadAuthenticationApps();
 		$errors = Installer::installShippedApps(true);
 		foreach ($errors as $appId => $exception) {
 			/** @var \Exception $exception */
Index: public.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- public.php	(revision 29b4246840f298c03983457c99f4aa181f644b75)
+++ public.php	(date 1588509946886)
@@ -66,7 +66,7 @@
 
 	// Load all required applications
 	\OC::$REQUESTEDAPP = $app;
-	OC_App::loadApps(['authentication']);
+	OC_App::loadAuthenticationApps();
 	OC_App::loadApps(['filesystem', 'logging']);
 
 	if (!\OC::$server->getAppManager()->isInstalled($app)) {

sshambar · 2020-05-03T16:53:57Z

Aha, so groups don't apply to authentication apps? That's valuable info... does it apply to prelogin apps too? (as that's the stage when users were logged out in this case :)

I had considered modifying loadApps() to call getEnabledApps() with $all = true for certain app-types, but I didn't know it would be correct logic... if prelogin and authentication app lists can ignore the user, then this would work and be an easy solution (a simple in_array check). (session apps aren't an issue as the user_id isn't available yet, but I think it'd be harmless to add them to the list)

If that's the case, I'd be happy to update the patch...

kesselb · 2020-05-03T17:14:37Z

Aha, so groups don't apply to authentication apps? That's valuable info... does it apply to prelogin apps too? (as that's the stage when users were logged out in this case :)

server/lib/private/App/AppManager.php

Lines 56 to 66 in caff102

    
           	/** 
        
           	 * Apps with these types can not be enabled for certain groups only 
        
           	 * @var string[] 
        
           	 */ 
        
           	protected $protectedAppTypes = [ 
        
           		'filesystem', 
        
           		'prelogin', 
        
           		'authentication', 
        
           		'logging', 
        
           		'prevent_group_restriction', 
        
           	];

Yep.

If that's the case, I'd be happy to update the patch...

Let's wait for @rullzer and @ChristophWurst ;) The current patch is a rather small change. It's probably easier to get this in now and still do a bigger refactoring later. I think there is already something planed to improve app loading for Nextcloud 20.

sshambar · 2020-05-03T17:24:20Z

The updated patch probably wouldn't be very large, a new AppManager method (hasNoProtectedAppTypes?), and use that to set the $all flag to getEnabledApps()

kesselb · 2020-05-03T17:26:37Z

prelogin apps

Out of curiosity: Are you still able to reproduce the issue if you remove <prelogin/> from info.xml? (the types are also cached in oc_appconfig table and needs to be updated there too). I've checked user_saml, user_sql and user_ldap and they are not using <prelogin/>. Probably the user_external app is just loaded to early? 🤔 🤷‍♂️

kesselb · 2020-05-03T17:29:48Z

The updated patch probably wouldn't be very large, a new AppManager method (hasNoProtectedAppTypes?), and use that to set the $all flag to getEnabledApps()

Sounds fair but the loadApps method is already complex 💩 😃

sshambar · 2020-05-03T17:34:58Z

No, the app type isn't the problem (although I agree it's probably unneeded, as it's also an authentication type). The bug is that getUser() is called even before the app-list is fetched in getEnabledApps... and since the DB backend is already loaded, it recognizes the user_id, fails the password check, and invalidates the session.

An alternative solution I considered (but seemed much more invasive), was to add the DB user backend in OC_User::setupBackends(), but that means it's added much later than currently, and might break some code that depends on it.

kesselb · 2020-05-03T18:15:47Z

The bug is that getUser() is called even before the app-list is fetched in getEnabledApps... and since the DB backend is already loaded, it recognizes the user_id, fails the password check, and invalidates the session.

I was just wondering why user_ldap or user_saml are not affected. <prelogin/> seems to be a difference. Also user_external uses a different (probably outdated way) to register user backends. For some reason user_ldap, user_saml and user_sql does this with their appinfo/app.php.

Such an interesting problem 🐛 Thanks again for digging 👍

sshambar · 2020-05-03T18:24:21Z

Actually, the other apps aren't affected as they use user_id's that are unique, and not likely to be in the database (eg, user_ldap may have a user_id like '3bb4681e-1ed6-103a-9648-7dc9709f72ae', which would be unlikely to overlap with a db user like 'admin').

user_external uses user_id's that match the supplied login, which is likely to overlap with database users... I suppose it could updated to use a "unique" user_id, but it'd create a tricky migration and would likely break a lot of installs.

Still, the underlying logic problem would still exist in the app loading code, where user sessions are validated long before the apps are setup to correctly handle it.

kesselb · 2020-05-03T19:12:27Z

If the user backend is not loaded yet $this->activeUser = $this->manager->get($uid); will be null and validation for the session also skipped.

the underlying logic problem would still exist in the app loading code

Yep (but only for apps using a "outdated" way to register their backend).

rullzer · 2020-05-03T19:18:37Z

user_external uses user_id's that match the supplied login, which is likely to overlap with database users... I suppose it could updated to use a "unique" user_id, but it'd create a tricky migration and would likely break a lot of installs.

Well that is then the issue. If you have users that are also in the main backend then it should log them out. Or you delete them form the main DB auth. But this not validating seems dangerous.

I really don't wanna mess with the session handling so close to a release.

sshambar · 2020-05-03T19:42:16Z

Well that is then the issue. If you have users that are also in the main backend then it should log them out. Or you delete them form the main DB auth. But this not validating seems dangerous.

Logging people out when they have a valid login just leads to people thinking nextcloud is broken... and a since there's no logging of why, much confusion (it's not easy to track this problem down, believe me :)

Validating still always happens, just not as early (before supporting apps are loaded)... getUser called everywhere except during the app loading will force a validation (which only happens every 5 mins anyway), and is called many times on any page load.

sshambar · 2020-05-03T19:44:02Z

Or you delete them form the main DB auth

Would you suggest deleting the admin user? Seems like a very dangerous thing to do...

sshambar · 2020-05-03T20:14:47Z

Actually, I just gave it a try on my test system.. deleting admin from the database works, and I can still login as admin via user_external. So perhaps that's the workaround for the user_external app until app loading can work without early session invalidation... of course, there's no "backup" login if user_external is every disabled... hmm...

I still like the idea of having getEnableApps() only use getUser() when there's a non-protected app type being loaded... that seems right logically.

rullzer · 2020-05-04T07:31:29Z

I would even argue that maybe admin is not the best to have as an username ;)

Don't get me wrong we should fix this probably at some point. But just right now I don't wanna touch this so close to 19.

t2d · 2020-05-04T08:58:05Z

Don't get me wrong we should fix this probably at some point. But just right now I don't wanna touch this so close to 19

I don't know how your release process works, so I 'm not qualified to comment on this. But please be aware that #20756 is extremely annoying to big user bases authenticated via IMAP.

sshambar · 2020-05-04T16:41:06Z

Don't get me wrong we should fix this probably at some point. But just right now I don't wanna touch this so close to 19.

Sounds good, if the release is imminent I can understand not wanting to risk any regressions :)

Unless you think it's a terrible idea, I'll update the patch to create a new "all protected app-types" test to determine if getEnableApps should get the $all=true parameter -- that should always be correct, fixes the problem, and also eliminates any change in user validation.

It might even be more efficient.

) Adds a new AppManager method which identifies when all app types are protected, loads all apps (without checking user filters). This has the intended benefit of avoiding calls to User\Session:getUser() for these app types, and likely invalidating the session before user backends are loaded. Signed-off-by: Scott Shambarger <devel@shambarger.net>

sshambar · 2020-05-04T21:17:31Z

OK, I've reverted the session invalidation deferral patch, and replaced it with code to request getEnableApps load all apps when only protected app types are being loaded.

This has the intended benefit of avoiding calls to User\Session:getUser() for these app types.

Patch is an alternative, and just as effective, fix to 20756. Please let me know if you'd like me to squish the original patch (I figured for now I'd leave it for reference).

kesselb · 2020-05-10T21:21:08Z

Don't check if the app is enabled for a user for protected apps 👍

But I still think the user_external app should also be fixed:

Don't listen for prelogin
Don't use user_backends directly. Instead use a different config key (e.g. external_user_backends) and let the app do the backend registration (like user_ldap). I know that's a BC break.

After that we should deprecate usage of user_backends. This will also fix the issue (because the backend is not loaded at the validation stage) and will probably land much sooner.

skjnldsv · 2024-02-27T13:13:08Z

@sshambar have you been able to check @kesselb's comment?
Otherwise, I'll close this PR for inactivity after a bit of time :(

sshambar · 2024-02-27T19:29:13Z

Don't check if the app is enabled for a user for protected apps 👍

But I still think the user_external app should also be fixed:

I agree that user_external should be fixed.

I suggested using unique userids in user_external to prevent db users from failing validation duing prelogin... but that fix would involve a significant migration step. Wouldn't overlapping userids still be an issue even if the external_user_backends change was made?

Of course, redesigning user_external registration is really a topic that should be discussed with the user_external maintainer (which I'm not :), I was just suggesting a fix to the nextcloud app-reg-flow that would affect any authentication app that had overlapping userids with db users.

skjnldsv · 2024-08-16T09:37:54Z

Hey @sshambar

It seems you're right, the approach was interesting, but ultimately we need a wider refactor of the user_external registration. Closing then, I hope we can tackle this in a different way in a near future 💪

Thanks for the PR and the discussions @sshambar
I wish you a great day :)
Cheers

Skip session validation during app load (Fixes nextcloud#20756)

581d300

Adds parameter to User\Session:getUser() to skip validation, and modifies OC_App::getEnabledApps() to use this parameter to skip session validation when loading apps. Signed-off-by: Scott Shambarger <devel@shambarger.net>

sshambar mentioned this pull request Apr 30, 2020

Frequent session timeouts when using External users (OC_User_IMAP) nextcloud/user_external#101

Open

violoncelloCH requested review from rullzer and ChristophWurst May 1, 2020 12:06

violoncelloCH added 3. to review Waiting for reviews bug feature: authentication labels May 1, 2020

sshambar added 2 commits May 4, 2020 14:00

Revert 581d300

1885d41

kesselb mentioned this pull request May 8, 2020

Add dedicated API for apps' bootstrapping process #20865

Merged

11 tasks

This comment has been minimized.

Sign in to view

blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023

This was referenced May 3, 2023

27.0.0 beta 1 #38038

Merged

27.0.0 beta 2 #38137

Closed

27.0.0 beta 2 #38139

Merged

blizzz mentioned this pull request May 17, 2023

27.0.0 RC1 #38341

Merged

blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023

skjnldsv mentioned this pull request Nov 1, 2023

chore: 28.0.0 beta 1 #41228

Merged

This was referenced Nov 6, 2023

28.0.0 beta 2 #41314

Merged

28.0.0 beta 3 #41379

Merged

This was referenced Nov 14, 2023

28.0.0 beta 4 #41477

Merged

28.0.0 RC1 #41621

Merged

blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023

skjnldsv added 2. developing Work in progress stale Ticket or PR with no recent activity and removed 3. to review Waiting for reviews labels Feb 27, 2024

skjnldsv marked this pull request as draft February 27, 2024 16:41

This was referenced Mar 12, 2024

29.0.0 beta 2 #44152

Merged

29.0.0 beta 3 #44186

Merged

29.0.0 beta 4 #44275

Merged

Altahrim mentioned this pull request Mar 20, 2024

29.0.0 beta 5 #44361

Merged

skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024

skjnldsv closed this Aug 14, 2024

skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip session validation during app load (Fixes #20756) #20757

Skip session validation during app load (Fixes #20756) #20757

sshambar commented Apr 30, 2020 •

edited by violoncelloCH

Loading

kesselb commented May 3, 2020

sshambar commented May 3, 2020

kesselb commented May 3, 2020

sshambar commented May 3, 2020

kesselb commented May 3, 2020

kesselb commented May 3, 2020

sshambar commented May 3, 2020

kesselb commented May 3, 2020

sshambar commented May 3, 2020

kesselb commented May 3, 2020 •

edited

Loading

rullzer commented May 3, 2020

sshambar commented May 3, 2020

sshambar commented May 3, 2020

sshambar commented May 3, 2020

rullzer commented May 4, 2020

t2d commented May 4, 2020

sshambar commented May 4, 2020

sshambar commented May 4, 2020

kesselb commented May 10, 2020

This comment has been minimized.

skjnldsv commented Feb 27, 2024

sshambar commented Feb 27, 2024

skjnldsv commented Aug 16, 2024

Skip session validation during app load (Fixes #20756) #20757

Skip session validation during app load (Fixes #20756) #20757

Conversation

sshambar commented Apr 30, 2020 • edited by violoncelloCH Loading

kesselb commented May 3, 2020

sshambar commented May 3, 2020

kesselb commented May 3, 2020

sshambar commented May 3, 2020

kesselb commented May 3, 2020

kesselb commented May 3, 2020

sshambar commented May 3, 2020

kesselb commented May 3, 2020

sshambar commented May 3, 2020

kesselb commented May 3, 2020 • edited Loading

rullzer commented May 3, 2020

sshambar commented May 3, 2020

sshambar commented May 3, 2020

sshambar commented May 3, 2020

rullzer commented May 4, 2020

t2d commented May 4, 2020

sshambar commented May 4, 2020

sshambar commented May 4, 2020

kesselb commented May 10, 2020

This comment has been minimized.

skjnldsv commented Feb 27, 2024

sshambar commented Feb 27, 2024

skjnldsv commented Aug 16, 2024

sshambar commented Apr 30, 2020 •

edited by violoncelloCH

Loading

kesselb commented May 3, 2020 •

edited

Loading