Refactor login to converge in the same function #40520

jvillafanez · 2022-11-30T09:26:52Z

Description

Refactor login process so it ends going through the new loginInOwnCloud method.

The new method will be perform the following:

It will trigger the login events (pre and post login)
Perform common checks to all auth mechanisms, such as checking if the logging user is disabled
Set the user, and prepare it
Update login timestamp

Specific checks based on the auth mechanism (checking if the password is correct, or checking the expiration of the token) will be performed outside.

Additional changes brought in the PR:

As said, the login will be performed by the new loginInOwnCloud method. This has caused some changes for some authentication methods about when the login events are triggered. This means that for some auth methods, the events will be triggered later and after some operations / checks have been performed. This might cause issues and will require verification.
For the loginWithToken, the token will be validated before trying to login with it. Previously, we logged in and then validated the token, so this might cause changes in the behavior
The createSessionToken will be called after a successful login. There were cases were this was called before login and others after login, so it's unclear when the session token should be created. This might need to be adjusted.
preRememberedLogin and postRememberedLogin events have been removed. These were additional events triggered by the "remember me" login which was broken until recently, so it's very unlikely they were used. Currently, the "remember me" login won't trigger any login event, but it might need to trigger the regular ones (prelogin and postlogin). They're currently being skipped.

Known issues:

openidconnect's LoginFlowController will call the loginUser method without a module class. This will report the login type as empty instead of the expected openidconnect class name. There is nothing we can do without changing the app. This is harmless for this PR, but it will affect further functionality.
Since user_shibboleth depends on an apache module and ownCloud will get the info from apache, the "apache" login type will be reported for the user_shibboleth app.

Related Issue

Preparations for https://github.com/owncloud/enterprise/issues/5295

Motivation and Context

This is a preparation to bring new functionality. We need a centralized point where all login paths can converge.

How Has This Been Tested?

test environment:
test case 1:
test case 2:
...

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Database schema changes (next release will require increase of minor version instead of patch)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests only (no source changes)

Checklist:

update-docs · 2022-11-30T09:26:56Z

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

jvillafanez · 2022-11-30T09:44:05Z

Need to check the following, having oauth2 and openidconnect apps active. It seems openidconnect triggers even though it's an oauth2 token.

{"reqId":"bbc1f402-53e7-469c-8f18-4a80ffa455a4","level":3,"time":"2022-11-30T09:41:46+00:00","remoteAddr":"10.0.2.27","user":"admin","app":"OpenID","method":"GET","url":"\/ocs\/v1.php\/cloud\/capabilities?format=json","message":"OpenIdConnectAuthModule::authToken Bearer SKWHXCJ4fxeByCoglUK0AlQ2tkeGZoHGmEO57XRgS3cbo7MlEroVgXdvxxvs9Irf"}
{"reqId":"bbc1f402-53e7-469c-8f18-4a80ffa455a4","level":3,"time":"2022-11-30T09:41:46+00:00","remoteAddr":"10.0.2.27","user":"admin","app":"PHP","method":"GET","url":"\/ocs\/v1.php\/cloud\/capabilities?format=json","message":"Undefined offset: 1 at \/var\/www\/owncloud\/apps\/openidconnect\/vendor\/jumbojett\/openid-connect-php\/src\/OpenIDConnectClient.php#1319"}
{"reqId":"bbc1f402-53e7-469c-8f18-4a80ffa455a4","level":3,"time":"2022-11-30T09:41:46+00:00","remoteAddr":"10.0.2.27","user":"admin","app":"no app in context","method":"GET","url":"\/ocs\/v1.php\/cloud\/capabilities?format=json","message":"Token (as per introspection) is inactive: {\"active\":false}"}
{"reqId":"bbc1f402-53e7-469c-8f18-4a80ffa455a4","level":3,"time":"2022-11-30T09:41:46+00:00","remoteAddr":"10.0.2.27","user":"admin","app":"OpenID","method":"GET","url":"\/ocs\/v1.php\/cloud\/capabilities?format=json","message":"Exception: {\"Exception\":\"Jumbojett\\\\OpenIDConnectClientException\",\"Message\":\"Token (as per introspection) is inactive\",\"Code\":0,\"Trace\":\"#0 \\\/var\\\/www\\\/owncloud\\\/apps\\\/openidconnect\\\/lib\\\/OpenIdConnectAuthModule.php(160): OCA\\\\OpenIdConnect\\\\Client->verifyToken()\\n#1 \\\/var\\\/www\\\/owncloud\\\/apps\\\/openidconnect\\\/lib\\\/OpenIdConnectAuthModule.php(116): OCA\\\\OpenIdConnect\\\\OpenIdConnectAuthModule->verifyToken()\\n#2 \\\/var\\\/www\\\/owncloud\\\/apps\\\/openidconnect\\\/lib\\\/OpenIdConnectAuthModule.php(95): OCA\\\\OpenIdConnect\\\\OpenIdConnectAuthModule->authToken()\\n#3 \\\/var\\\/www\\\/owncloud\\\/lib\\\/private\\\/User\\\/Session.php(1235): OCA\\\\OpenIdConnect\\\\OpenIdConnectAuthModule->auth()\\n#4 \\\/var\\\/www\\\/owncloud\\\/ocs\\\/v1.php(80): OC\\\\User\\\\Session->verifyAuthHeaders()\\n#5 {main}\",\"File\":\"\\\/var\\\/www\\\/owncloud\\\/apps\\\/openidconnect\\\/lib\\\/Client.php\",\"Line\":211}"}
{"reqId":"75972b02-db89-47c9-a075-f08cb32b030b","level":3,"time":"2022-11-30T09:41:46+00:00","remoteAddr":"10.0.2.27","user":"admin","app":"OpenID","method":"GET","url":"\/ocs\/v1.php\/cloud\/user?format=json","message":"OpenIdConnectAuthModule::authToken Bearer SKWHXCJ4fxeByCoglUK0AlQ2tkeGZoHGmEO57XRgS3cbo7MlEroVgXdvxxvs9Irf"}
{"reqId":"75972b02-db89-47c9-a075-f08cb32b030b","level":3,"time":"2022-11-30T09:41:46+00:00","remoteAddr":"10.0.2.27","user":"admin","app":"PHP","method":"GET","url":"\/ocs\/v1.php\/cloud\/user?format=json","message":"Undefined offset: 1 at \/var\/www\/owncloud\/apps\/openidconnect\/vendor\/jumbojett\/openid-connect-php\/src\/OpenIDConnectClient.php#1319"}
{"reqId":"75972b02-db89-47c9-a075-f08cb32b030b","level":3,"time":"2022-11-30T09:41:46+00:00","remoteAddr":"10.0.2.27","user":"admin","app":"no app in context","method":"GET","url":"\/ocs\/v1.php\/cloud\/user?format=json","message":"Token (as per introspection) is inactive: {\"active\":false}"}
{"reqId":"75972b02-db89-47c9-a075-f08cb32b030b","level":3,"time":"2022-11-30T09:41:46+00:00","remoteAddr":"10.0.2.27","user":"admin","app":"OpenID","method":"GET","url":"\/ocs\/v1.php\/cloud\/user?format=json","message":"Exception: {\"Exception\":\"Jumbojett\\\\OpenIDConnectClientException\",\"Message\":\"Token (as per introspection) is inactive\",\"Code\":0,\"Trace\":\"#0 \\\/var\\\/www\\\/owncloud\\\/apps\\\/openidconnect\\\/lib\\\/OpenIdConnectAuthModule.php(160): OCA\\\\OpenIdConnect\\\\Client->verifyToken()\\n#1 \\\/var\\\/www\\\/owncloud\\\/apps\\\/openidconnect\\\/lib\\\/OpenIdConnectAuthModule.php(116): OCA\\\\OpenIdConnect\\\\OpenIdConnectAuthModule->verifyToken()\\n#2 \\\/var\\\/www\\\/owncloud\\\/apps\\\/openidconnect\\\/lib\\\/OpenIdConnectAuthModule.php(95): OCA\\\\OpenIdConnect\\\\OpenIdConnectAuthModule->authToken()\\n#3 \\\/var\\\/www\\\/owncloud\\\/lib\\\/private\\\/User\\\/Session.php(1235): OCA\\\\OpenIdConnect\\\\OpenIdConnectAuthModule->auth()\\n#4 \\\/var\\\/www\\\/owncloud\\\/ocs\\\/v1.php(80): OC\\\\User\\\\Session->verifyAuthHeaders()\\n#5 {main}\",\"File\":\"\\\/var\\\/www\\\/owncloud\\\/apps\\\/openidconnect\\\/lib\\\/Client.php\",\"Line\":211}"}

DeepDiver1975 · 2022-11-30T09:50:58Z

Need to check the following, having oauth2 and openidconnect apps active. It seems openidconnect triggers even though it's an oauth2 token.

Both are Bearer tokens - both modules are triggered. Generally speaking this is/was fine as these two modules learned to live together ....

jvillafanez · 2022-11-30T10:29:02Z

Are both modules expected to be enabled at the same time? If so, we should fix those errors because they happen periodically with the desktop client. Otherwise, we could treat it as incompatible apps so only one of them should be enabled.

jvillafanez · 2022-11-30T14:01:43Z

Things tested so far:

regular login page -> reports "password" login
basic auth (for webdav, for example) -> "password" login
app password -> reports "token" login (also in the login page)
user_shibboleth -> reports "apache" login
oauth2 -> reports "OCA\OAuth2\AuthModule" login
openidconnect from the login page -> reports empty login (known issue)
openidconnect (using an access token) -> reports "OCA\OpenIdConnect\OpenIdConnectAuthModule" login

Not tested:

kerberos -> expects "OCA\Kerberos\AuthModule"
user_external -> expects "password" (no specific auth module found, likely using the regular user + password code path, but checking against different services)

jvillafanez · 2022-12-01T09:28:13Z

Some additional changes in the code:

the validateSession method will explicitly check if the user is disabled. This was happening inside the validateToken method, but we're already checking it in the new loginInOwnCloud method. Any login attempt should go through the loginInOwnCloud method, so we'll check if the user is disabled or not. However, the validateSession won't go through the login process, so we need an explicit check there.
Additional changes to the token login:
- Token will be invalidated if the login failed, or if a LoginException happens (not sure if these cases were covered previously)
- Checking if the user is disabled has been removed from the validateToken method.
The loginUser requires a valid user, if no user is provided, a failed login will be triggered. (This is a fix in this PR. Code in master had this check in place)
New info log added to notify the user is disabled. This has been added because there is an info log to show the uid and the login type used. If the user is disabled, that log will show and it might be misleading; the admin could think the disabled user has logged in successfully. This new log will clarify that such user is disabled.

Other than that, the unit tests showed some code duplication in the token login flow (expected only one call in some methods, but currently calling them twice). No plans to fix it for the moment in order to limit the changes, but we should look it up at some point.

jvillafanez · 2022-12-01T12:23:49Z

I think this is ready to review. Taking into account this is mostly a refactor, I don't think we need a changelog entry.
There is a feature incoming relying on these changes, but it might be better if we merge this refactor first so we can check if there is something missing. These changes shouldn't give problems, but better to ensure it..

DeepDiver1975 · 2022-12-14T09:37:22Z

lib/private/User/Session.php

@@ -530,38 +534,12 @@ public function tryBasicAuthLogin(IRequest $request) {
 	 * compatibility.
 	 */
 	private function loginWithPassword($login, $password) {
-		$beforeEvent = new GenericEvent(null, ['loginType' => 'password', 'login' => $login, 'uid' => $login, '_uid' => 'deprecated: please use \'login\', the real uid is not yet known', 'password' => $password]);


This changes the order of events. Could have some side effects ...

... applies to all refactored methods

We can try to use an "afterPreLogin" callback in the loginInOwnCloud method in order to move the checks. It will add additional complexity though, and it looks bad to have to use callbacks in the same class.
I don't think we have better options. The code paths are quite different and I had trouble trying to funnel the code into one method.

Right now, the loginInOwnCloud requires a user object. In this loginWithPassword method, the user object comes from the checkPassword method, not from the regular userManager->get(...). The userManager might not be able to find the user by login and rely on the checkPassword to get the user object (likely the case of LDAP).
This means that we'll have to use another callback (the second one) so the loginInOwnCloud method gets the right user object.

Even if we don't use callbacks and use some kind of flag, the loginInOwnCloud method will need to use more than one way to get the user object. We'll also need to inject code before getting the user object (for apache login) and after that (for cookie login).

So we need a flag to decide if we use the userManager's get method or the checkPassword, and we also need 2 callbacks to inject code before and after getting the user. It seems to much spaghetti

jvillafanez · 2022-12-14T14:30:16Z

I think we'll have to take the risk of changing the behavior....

Currently the loginInOwnCloud method does:

trigger beforeLogin event
if the user is enabled
    set session user
    trigger afterLogin event
    prepare user login (initialize FS)

In order to keep the previous behavior the method should do

trigger beforeLogin event
run custom code (for apache login)
get user instance either via userManager's `get` or `checkPassword` (need a flag)
run custom code (for cookie login)
if the user is enabled
    set session user
    trigger afterLogin event
    prepare user login (initialize FS)

So we need a flag because the loginInOwnCloud method will need to use checkPassword when it comes from the loginWithPassword (likely due to LDAP), and we need 2 callbacks to inject code before and after getting the user.
Having to use a different method to get the user looks bad, but having to use callbacks to inject code in a private method seems to much spaghetti. Anyway, I don't think there are other options in order to keep the previous behavior.

Regarding the change in the behavior, I think it sums up to when the events are triggered.

Previously, a pre-login event could be triggered followed by a failed-login event. The post-login event wouldn't trigger if a failed-login event happened. A wrong password attempt would trigger that event sequence.

With the current code, the login events are triggered quite late. It would be rare that something goes wrong after the pre-login event.
For the particular case of a wrong password, the current code would trigger only the failed-login event, but it doesn't trigger the pre-login event. The same behavior happens in many other situations.

In case of disabled users, some previous code triggered a failed-login event while other not. For now, the PR doesn't trigger the event, but we might need to change this.

I think this could be a good chance to homogenize the behavior with the events:

If a failed-login event is triggered, there shouldn't be a pre-login event. This seems reasonable to me.
If a pre-login event happens, all the required checks (at least the ones from core) should have passed
If the user is disabled, a failed-login event should happen, but not a pre-login event (this is something we should adjust in the PR)
A post-login event should happen after the user is set in the session and the FS is initialized (it seems reasonable that the FS initialization is part of the login process)

DeepDiver1975 · 2022-12-14T14:44:18Z

The only code using the preLogin event is in one code location and it blocks login of one specfic user.
Running this code in a later point in time has no further impact.

Let's move on 👍

IljaN · 2022-12-21T14:12:01Z

CI Fails

DeepDiver1975 · 2022-12-21T14:47:54Z

CI Fails

windows smb -> known
webdav -> timeout -> just trigger it again

IljaN · 2022-12-21T15:00:19Z

Already re-triggered once, will retry again

phil-davis · 2023-01-02T10:38:21Z

I rebased to trigger fresh CI - we can see if anything unexpected fails.

ValidateSession will explicitly check if the user is disabled. Previously, the check was done in the validateToken method, but general check for the disabled user has been moved to the loginInOwncloud method, which requires login that won't happen in the validateSession method. Token will be invalidated if a LoginException happens. New info log added to notify the disabled user in the logs

phil-davis · 2023-01-05T05:51:21Z

I rebased again - CI should be green now.

sonarcloud · 2023-01-05T06:18:11Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

93.9% Coverage
0.0% Duplication

phil-davis · 2023-01-05T06:51:35Z

CI is green. I will merge this. Overnight we will get CI results of all nightly oC10 apps. That will help to know if there are any app-related side-effects.

jvillafanez self-assigned this Nov 30, 2022

jvillafanez added the 2 - Developing label Nov 30, 2022

jvillafanez force-pushed the login_refactor branch from 55cac82 to 989690d Compare November 30, 2022 14:10

jvillafanez added 3 - To Review and removed 2 - Developing labels Dec 1, 2022

jvillafanez marked this pull request as ready for review December 1, 2022 11:25

DeepDiver1975 requested changes Dec 14, 2022

View reviewed changes

DeepDiver1975 approved these changes Dec 14, 2022

View reviewed changes

jvillafanez mentioned this pull request Dec 14, 2022

Send to auth module so the login type is recognized in core owncloud/openidconnect#272

Merged

10 tasks

phil-davis force-pushed the login_refactor branch from b9733ee to ad22b85 Compare January 2, 2023 10:37

jvillafanez added 2 commits January 5, 2023 11:35

Refactor login to converge in the same function

eaf5a9f

phil-davis force-pushed the login_refactor branch from ad22b85 to 4e42b45 Compare January 5, 2023 05:50

phil-davis merged commit f6c15ed into master Jan 5, 2023

delete-merged-branch bot deleted the login_refactor branch January 5, 2023 06:51

saw-jan mentioned this pull request Jan 6, 2023

Login test failed nightly owncloud/brute_force_protection#190

Closed

This was referenced Jan 6, 2023

phpstan failed nightly owncloud/openidconnect#279

Closed

Bump version to 10.12.0 prealpha #40565

Merged

saw-jan mentioned this pull request Jan 6, 2023

Login is not blocked after multiple retries owncloud/brute_force_protection#191

Closed

amrita-shrestha mentioned this pull request Jan 6, 2023

Owncloud log behavior changed #40566

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor login to converge in the same function #40520

Refactor login to converge in the same function #40520

jvillafanez commented Nov 30, 2022 •

edited

Loading

update-docs bot commented Nov 30, 2022

jvillafanez commented Nov 30, 2022

DeepDiver1975 commented Nov 30, 2022

jvillafanez commented Nov 30, 2022

jvillafanez commented Nov 30, 2022

jvillafanez commented Dec 1, 2022

jvillafanez commented Dec 1, 2022

DeepDiver1975 Dec 14, 2022

DeepDiver1975 Dec 14, 2022

jvillafanez Dec 14, 2022

jvillafanez Dec 14, 2022

jvillafanez commented Dec 14, 2022

DeepDiver1975 commented Dec 14, 2022

IljaN commented Dec 21, 2022

DeepDiver1975 commented Dec 21, 2022

IljaN commented Dec 21, 2022

phil-davis commented Jan 2, 2023

phil-davis commented Jan 5, 2023

sonarcloud bot commented Jan 5, 2023

phil-davis commented Jan 5, 2023

Refactor login to converge in the same function #40520

Refactor login to converge in the same function #40520

Conversation

jvillafanez commented Nov 30, 2022 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

update-docs bot commented Nov 30, 2022

jvillafanez commented Nov 30, 2022

DeepDiver1975 commented Nov 30, 2022

jvillafanez commented Nov 30, 2022

jvillafanez commented Nov 30, 2022

jvillafanez commented Dec 1, 2022

jvillafanez commented Dec 1, 2022

DeepDiver1975 Dec 14, 2022

Choose a reason for hiding this comment

DeepDiver1975 Dec 14, 2022

Choose a reason for hiding this comment

jvillafanez Dec 14, 2022

Choose a reason for hiding this comment

jvillafanez Dec 14, 2022

Choose a reason for hiding this comment

jvillafanez commented Dec 14, 2022

DeepDiver1975 commented Dec 14, 2022

IljaN commented Dec 21, 2022

DeepDiver1975 commented Dec 21, 2022

IljaN commented Dec 21, 2022

phil-davis commented Jan 2, 2023

phil-davis commented Jan 5, 2023

sonarcloud bot commented Jan 5, 2023

phil-davis commented Jan 5, 2023

jvillafanez commented Nov 30, 2022 •

edited

Loading