Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add config option to allow basic auth only for guests #253

Merged
merged 3 commits into from
Sep 28, 2022

Conversation

jvillafanez
Copy link
Member

Guests will be able to access ownCloud using basic auth, but other users will need to access through other mechanisms such as oidc.

Description

The new openid-connect.basic_auth_guest_only config option only allows guests to be able to log in using basic auth. Other users will need to use another auth mechanisms (such as oidc)

Note that the users can still log in with ANY OTHER auth mechanism available, not just oidc. It's expected that oidc is the only alternative though.

NOTE: minimum OC version raised to 10.4 in order to use the UserTypeHelper to detect guest users.

To be checked:

  • remember me feature doesn't work with this app. It might still be useful for the guests though. We might need to disable the feature from the app info.
  • App password aren't expected to work

Related Issue

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

Motivation and Context

How Has This Been Tested?

Manually tested:

  1. Setup:
    • Keycloak with LDAP connected
    • Guest app installed
    • user_ldap app installed and connected to the same server which keycloak is connected to.
    • openidconnect app connected to keycloak
  2. Set 'openid-connect.basic_auth_guest_only' => true in the config.php file
  3. Ensure you can access with userA through keycloak using openidconnect (using the alternative login in the login page)
  4. Ensure you can access with a guest user using username and password in the ownCloud's login page
  5. Ensure you cannot access with userA through the ownCloud's login page

Screenshot from 2022-09-26 14-06-15

Note: Users (except guests) MUST also use a different auth mechanism (such as oidc) in order to access to the webdav interface. This might affect mobile and desktop clients. It's expected to work, but not tested yet.

Screenshots (if appropriate):

Types of changes

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

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Unit tests

Guests will be able to access ownCloud using basic auth, but other users
will need to access through other mechanisms such as oidc.
@CLAassistant
Copy link

CLAassistant commented Sep 26, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@DeepDiver1975 DeepDiver1975 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • testable via unit test ?
  • 🤖 is 🔴

* @throws LoginException if the uid isn't a guest
*/
public function ensurePasswordLoginJustForGuest($loginType, $uid) {
if (!$this->config->getSystemValue('openid-connect.basic_auth_guest_only', false)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe only add the listener if config states ...... but minor detail .....

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@pmaier1
Copy link
Contributor

pmaier1 commented Sep 28, 2022

Needs documentation @mmattel -> owncloud/docs-server#665

@jnweiger jnweiger merged commit 8795d14 into master Sep 28, 2022
@delete-merged-branch delete-merged-branch bot deleted the basic_auth_guest_only branch September 28, 2022 11:05
@ChrisEdS
Copy link
Contributor

Needs also translation for the new error message?

image

@DeepDiver1975
Copy link
Member

indeed - until today we don't have any translations .... I can take care

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants