If basic_auth_guest_only is active, allow groups to bypass the check

[jvillafanez]

Description

Follow up of https://github.com/owncloud/openidconnect/pull/253 , sysadmin can allow some groups to access via basic auth.

Assuming 'openid-connect.basic_auth_guest_only' => true, there might be cases where not only guests should be allowed to access via basic auth. The admin group is a good example. By adding openid-connect.basic_auth_guest_only.exclude_groups => ['groupid'], users belonging to those groups will be allowed to access via basic auth. Note that guest users will still be able to access via basic auth regardless of the groups set (if any).

Note that you'll need to provide the ownCloud's group id.

We expect few groups to be added to the exclusion list. The most common case would be to add just the admin group. Performance will likely drop of there are too many groups added because we're checking if the user is member of any of the groups one by one. We don't expect more that 2-3 groups in the exclusion list.

Related Issue

Follow up of https://github.com/owncloud/enterprise/issues/5295

Motivation and Context

Admins might need to access to ownCloud bypassing the oidc auth. This will allow admins to access via basic auth by adding 'openid-connect.basic_auth_guest_only.exclude_groups' => ['admin']. Additional groups can be added if needed.

How Has This Been Tested?

Following steps of https://github.com/owncloud/openidconnect/pull/253

1. Check that just with 'openid-connect.basic_auth_guest_only' => true the admin can't login via basic auth
2. Add the admin group to the exclusion 'openid-connect.basic_auth_guest_only.exclude_groups' => ['admin']
3. The admin user can now access ownCloud via basic auth

Screenshots (if appropriate):

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)
• [ ] Technical debt
• [ ] Tests

Checklist:

• [x] Code changes
• [ ] Unit tests added
• [ ] Acceptance tests added
• [ ] Documentation ticket raised:

Open tasks:

• [ ] Unit tests

[DeepDiver1975]

Super wild idea: move this whole logic into a custom app which goes out to the customers who request this feature. It feels kind of wrong in scope of the product ....

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

0.0% Coverage
0.0% Duplication

[jvillafanez]

Super wild idea: move this whole logic into a custom app which goes out to the customers who request this feature. It feels kind of wrong in scope of the product ....

@pmaier1 ^

[mmattel]

Docs relevant...

[pako81]

@jvillafanez

Isn't if (!$this->userTypeHelper->isGuestUser($uid) && !$this->isInGroupList($uid, $excludedGroupsIds)) actually redundant?

We can probably pack to whole feature inside the openid-connect.basic_auth_guest_only.exclude_groups config.php option and let admins add the guests group in there along with the admin group (and any other group which should be allowed).

Having two config.php options here is probably too much?

In this case maybe we should think about a different name since this is more an allow list rather than an exclusion one? So instead of openid-connect.basic_auth_guest_only.exclude_groups something like openid-connect.basic_auth_only.allow_groups ?

Maybe the method ensurePasswordLoginJustForGuest could also be renamed to reflect this does not apply to guests only but to any group in the allow list.

[jvillafanez]

I assume the expected behavior is that guests are always allowed for basic auth regardless of the group they are. This also include the case were the group list is empty. Taking into account the list is expected to be provided by the admin, we need to consider the cases were the list is "wrong", for example, if the list doesn't contain the guests group. I mean, this is ok and we don't need to handle it, but if the feature is about allowing guests but somehow guests aren't allowed, it looks bad.

I'm ok with the change, but then we need to redefine the feature. It won't be "guests are allowed for basic auth" but "specific groups are allowed basic auth".

Anyway, I think the plan is to move the functionality elsewhere, not in the app. This could also require changes in how the configuration could look like. For example, if it's moved to core, we might be able to prevent authentication of users or groups based on the authentication mechanism (not sure if it's possible though). This also needs to be taken into account to provide sensible configuration options.

[pako81]

I assume the expected behavior is that guests are always allowed for basic auth regardless of the group they are. This also include the case were the group list is empty.

yes, you are probably right. That check would allow the admin to not explicitly add the guests group in the openid-connect.basic_auth_only.allow_groups list (or whatever we are gonna call it) so it should probably be kept.

[DeepDiver1975]

I am repeating my suggestion: Let's move this out of this app. It feels wrong in here.....

[jnweiger]

@jvillafanez I am confused. Do want a release without these PRs merged? I've moved #241 back in engineering for now. On the other hand, this is not even among the reasons listed in #241

[DeepDiver1975]

JP is working on an alternative approach on this which is based on core changes.

Since nobody else is willing to make the decision: I do so now - this is the wrong place to implement the original feature request