Add support for login policies

[jvillafanez]

A policy has been implemented so admins can allow or reject groups of users to access via specific login mechanisms

Description

Add support for login policies. An initial login policy has been implemented so admins can allow or reject groups to access ownCloud via specific login mechanism.
For example, only the groups "guests" and "admin" could access via username + password, while the rest of users must access through other mechanisms such as openidconnect.

Login policies will emit a "failed login" event if the user isn't allowed to login.

Some additional changes have been made in the token access (app password) so it isn't invalidated under some circumstances (login policies were causing the invalidation of the token if the policy rejected the user.)

Related Issue

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

Motivation and Context

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:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:
• Changelog item, see TEMPLATE

TODO:

• Unit tests.
• Changes in config.sample.php

Usage:

The GroupLoginPolicy is already pre-registered, so there is nothing to do in order to register this policy in the LoginPolicyManager

The LoginPolicyManager will use the loginPolicy.order key in the config.php file to activate and use the policies in the list in the specified order.

'loginPolicy.order' => ['OC\Authentication\LoginPolicies\GroupLoginPolicy'],

If no login policy is activated in the loginPolicy.order list, ownCloud will work normally. Only registered policies can be activated.

For now, only the OC\Authentication\LoginPolicies\GroupLoginPolicy is available.

GroupLoginPolicy configuration.

The configuration of the GroupLoginPolicy is done through the config.php file.

loginChecker.groupLoginChecker.forbidMap => [
  'password' => [
    'allowOnly' => ['group1, group2'],
    'reject' => ['group3'],
  ],
  'token' => [
    'reject' => ['admin'],
  ],
  '' => [
    'allowOnly' => ['group2'],
  ],
]

Each key of the forbidMap is a known login type (to be listed every known login type), and the value is a map containing a list of groups that are allow to use that login type (rejecting everyone else), and a list of groups that are rejected from using that login type.

In the above example, users belonging to the admin group won't be able to access via token (app password), while the rest of the users can.
Only users from group1 and group2 are allowed to access through username + password, and users from group3 will be rejected. In case a users is member of an allowOnly group and a reject group, rejection will take priority. This means that even if user1 is member of group1 he won't be able to access if he's also member of group3.

In some weird cases the login type is unknown (there was an issue with openidconnect which should be solved in the master code of the branch). In this case, the login type might be the empty string ''.

[butonic]

The Idea with #31546 was to use an IAuthModule or a similar PAM concept for thngs like this. But if you think it does not fit ... go ahead ...

[jvillafanez]

At least for this particular use case, I don't think we can use #31546 . We need to know how the user is being logged in or how he has logged in, and we don't have that information there. This would require changes in the interface.

In addition, I'm not exactly sure about the architecture. It seems the AuthModule depends on the AccountModule. This would mean that each AuthModule would depend and use the AccountModuleManager, which I'm not sure if it happens already (for openidconnect, kerberos and others).
There are also cases where the login doesn't go through an auth module (regular username + password, for example). It would be weird if you can't access through webdav but you can through web UI using the same username and password.

[DeepDiver1975]

usage of ?? can reduce line number .....

[jvillafanez]

Done.

For the "reject" and "allowOnly", I think it's clearer this way because we won't check anything if the keys aren't present.

[DeepDiver1975]

I guess the idea is also to allow apps to add login policies via calling the manager in app.php.
Was that tested as well? THX

[jvillafanez]

I guess the idea is also to allow apps to add login policies via calling the manager in app.php.
Was that tested as well? THX

That's the idea. I haven't tested it yet though. I'll probably check it by adding the expected code in a random app instead of creating one from scratch.

[mmattel]

@jvillafanez pls do not forget to add a config.sample.php entry like done in GroupLoginPolicy.php.
When merged, docs need to do a config-to-docs run to get the changes from config sample added.

[jvillafanez]

For testing, I've used the brute_force_protection app as base. Added the "lib/ProtPolicy.php" and the patch:

<?php

namespace OCA\BruteForceProtection;

use OCP\IUser;
use OCP\IRequest;
use OCP\Authentication\LoginPolicies\ILoginPolicy;
use OC\User\LoginException;

class ProtPolicy implements ILoginPolicy {
	private $request;

	public function __construct(IRequest $request) {
		$this->request = $request;
	}

	public function checkPolicy(string $loginType, IUser $user): bool {
		if ($user->getUID() === 'admin' && $this->request->getServerProtocol() === 'http') {
			throw new LoginException('admin access requires https');
		}
		return true;
	}
}

patch:

diff --git a/appinfo/app.php b/appinfo/app.php
index 0212f5f..1bc1170 100644
--- a/appinfo/app.php
+++ b/appinfo/app.php
@@ -27,3 +27,4 @@ use OCA\BruteForceProtection\Hooks;
 
 $app = new Application();
 $app->getContainer()->query(Hooks::class)->register();
+$app->registerLoginPolicy();
diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php
index 07987c9..4217aef 100644
--- a/lib/AppInfo/Application.php
+++ b/lib/AppInfo/Application.php
@@ -24,9 +24,16 @@
 namespace OCA\BruteForceProtection\AppInfo;
 
 use \OCP\AppFramework\App;
+use OCA\BruteForceProtection\ProtPolicy;
 
 class Application extends App {
        public function __construct(array $urlParams=[]) {
                parent::__construct('brute_force_protection', $urlParams);
        }
+
+       public function registerLoginPolicy() {
+               $server = $this->getContainer()->getServer();
+               $loginPolicyManager = $server->getLoginPolicyManager();
+               $loginPolicyManager->registerPolicy($this->getContainer()->query(ProtPolicy::class));
+       }
 }

Note that the brute_force_protection app is of the type "prelogin", so the app is expected to load quite early.
We require the app to be loaded before the user has logged in, so I'm not fully sure if the app requires any special type. I guess the "authentication" type could also make sense.

The basic idea is that the LoginPolicy is registered in the LoginPolicyManager when the app is loaded. This should be quite light because it's just creating the instance of the policy and register it. Other than that, it depends on what the actual policy does.
The configuration of the policy depends entirely on the app itself. In this dummy case, it doesn't require configuration, but more complex apps might provide a web UI to configure the policy, or do it through the config.php app.
The only thing to take into account is that, the policy needs to be registered and activated. To activate the policy, it needs to be in the loginPolicy.order list (config.php file). For this dummy test, it should be:

  'loginPolicy.order' => [
    'OC\Authentication\LoginPolicies\GroupLoginPolicy',
    'OCA\BruteForceProtection\ProtPolicy',
  ],

I think this is enough as a PoC that 3rd party apps can also provide custom login policies with this mechanism. I don't think we want to create a complete app just for testing this feature, at least without specific use cases.

[ownclouders]

💥 Acceptance tests pipeline cliExternalStorage-git-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/37643/116

[jvillafanez]

This is ready to review. I've checked the original issue and it works fine with this PR (tested with openidconnect and guests app to ensure it works).
We might need to adjust the wording of the error messages.

@mmattel I guess this will need to end up in the docs eventually. There should be enough information in this PR, but feel free to ask any question.

[DeepDiver1975]

The only thing to take into account is that, the policy needs to be registered and activated. To activate the policy, it needs to be in the loginPolicy.order list (config.php file). For this dummy test, it should be:

  'loginPolicy.order' => [
    'OC\Authentication\LoginPolicies\GroupLoginPolicy',
    'OCA\BruteForceProtection\ProtPolicy',
  ],

I think this is enough as a PoC that 3rd party apps can also provide custom login policies with this mechanism. I don't think we want to create a complete app just for testing this feature, at least without specific use cases.

That's a bit odd from my pov that this has to be setup in two places.....

[jvillafanez]

The registration part is for the devs. If we want to give them a chance to implement custom policies, someone will need to create the objects, and for core to create them will require a specific signature in the constructors, which will limit the options. By using a registration process, it's expected that the app will provide the objects already created, so it will just need to register them.
This registration process is expected to be done by the apps in the app.php file, so once the app is enabled the policy will always be registered. Note that for the policies provided by core, they're expected to be registered as soon as the policy manager instance is created.

Now, the admin needs a way to control the registered policies. Since core policies will always be registered, the admin needs a way to activate or deactivate them. In addition, he also has control over the policy order, so maybe he wants a custom policy to be checked before any of the core ones.

Maybe the confusion comes from using the class names there... we could require a getPolicyName() function in the interface, but we'd need to check for possible name collisions which would be a pain to deal with. Using the full class name should be easy enough to ensure there are no name collisions, and there is no additional requirement.

Note that an app could provide and register multiple login policies. There could be an app that groups a bunch of policies and registers 10 policies; in this case it's very unlikely that the admin will use all of them, but maybe 2 or 3.

[pako81]

@jvillafanez merge it?

[jvillafanez]

no problems from my side

[phil-davis]

I fixed the last-mentioned grammar thing in config.sample.php
And rebased, because the branch was now a long way behind master.
IMO this is good to merge when CI finishes.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
22 Code Smells

93.7% Coverage
0.0% Duplication