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 new system config to enforce strict login check for password in user backend #37569

Merged
merged 1 commit into from
Jun 23, 2020

Conversation

mrow4a
Copy link
Contributor

@mrow4a mrow4a commented Jun 22, 2020

When in LDAP config user login attributes are setuped disallowing login by email, verify settings as below work correctly:

However, as of at least OC9.X, when logging in with LDAP with loginname and password, OC always retries to get a user, and try login agains LDAP also with email. For some customers this should not be allowed.

This PR:

  • adds new system config /occ config:system:set --type boolean --value true strict_login_enforced that disallows to retry with email
  • add unit tests

@update-docs
Copy link

update-docs bot commented Jun 22, 2020

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.

Copy link
Contributor

@micbar micbar left a comment

Choose a reason for hiding this comment

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

LGTM

@phil-davis
Copy link
Contributor

https://drone.owncloud.com/owncloud/core/25508/3/2
file changelog/unreleased/37569: entry type "Feature" is invalid, valid types: map[Bugfix:2 Change:3 Enhancement:4 Security:1]

@mrow4a mrow4a force-pushed the feature/strict-login-check branch from ffd8af7 to 99d77a7 Compare June 22, 2020 08:16
@micbar
Copy link
Contributor

micbar commented Jun 22, 2020

@C0rby Can you review and check the prio.
We can get this into 10.5.0 if it is really urgent.

@phil-davis
Copy link
Contributor

https://drone.owncloud.com/owncloud/core/25509/3/2
file changelog/unreleased/37569: title is too long

Still not getting past the changelog parsing.

@micbar
Copy link
Contributor

micbar commented Jun 22, 2020

title is limited to 80 chars

@@ -236,6 +236,13 @@
*/
'token_auth_enforced' => false,

/**
* Enforce strict login for password authentication that will enforce
Copy link
Contributor

Choose a reason for hiding this comment

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

Please see the other config entries regarding the title line.
The first sentence must end at the same line and is used as item in the table of contents.
You can put more description below that first line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

pls see my comment on the changes below

@mmattel
Copy link
Contributor

mmattel commented Jun 22, 2020

Doc relevant, pls file a doc issue, because we need to do a config-to-docs run.
Else this config item will NOT be shown in docs!

@pako81
Copy link

pako81 commented Jun 22, 2020

What about the use case where only the log in via email address for LDAP users should be prevented (by unchecking the LDAP Login Attribute), while local users should still be able to normally log in with email?

@micbar
Copy link
Contributor

micbar commented Jun 22, 2020

@pako81 good point.

@mmattel could a config.sample.php doc not be sufficient? This is a very special use case.

@phil-davis
Copy link
Contributor

IMO @mmattel means just that - when this is merged we just need to do the "config-to-docs run" which will update the config.sample.php that is published in the docs. Nothing more will need to be written.

@mrow4a
Copy link
Contributor Author

mrow4a commented Jun 22, 2020

What about the use case where only the log in via email address for LDAP users should be prevented (by unchecking the LDAP Login Attribute), while local users should still be able to normally log in with email?

@pako81 @micbar I will check it, but again: strict login is strict login, if Local Users Backend has support only for UID that internally is not email, and LDAP allows only UID,MYSPECIALATTRIBUTE, we should only login on these ones..

@mrow4a
Copy link
Contributor Author

mrow4a commented Jun 22, 2020

@pako81 @micbar I can confirm that local user backend OC\User\Database currently supports searching for users only on their UIDs. This support can be of course extended to search also on other attributes as in LDAP, and this would be the cleanest approach - but needs development.

So as mentioned, whoever enables occ config:system:set --type boolean --value true strict_login_enforced has to understand that ALL user backends will check passwords only against user typed logins.

Personal Opinion: strict_login_enforced should be default in OC and admins should not "assume" that email also will be checked, this posses both security risks and is dirty, but probably because of legacies it is a bad idea to make it default ..

@mrow4a mrow4a force-pushed the feature/strict-login-check branch 3 times, most recently from 2a1826c to 4ebbebd Compare June 22, 2020 20:15
@mrow4a mrow4a requested a review from mmattel June 22, 2020 20:22
@codecov
Copy link

codecov bot commented Jun 22, 2020

Codecov Report

Merging #37569 into master will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #37569      +/-   ##
============================================
- Coverage     64.70%   64.66%   -0.04%     
+ Complexity    19350    19348       -2     
============================================
  Files          1281     1279       -2     
  Lines         75607    75609       +2     
  Branches       1333     1333              
============================================
- Hits          48920    48893      -27     
- Misses        26295    26324      +29     
  Partials        392      392              
Flag Coverage Δ Complexity Δ
#javascript 54.03% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.84% <100.00%> (-0.05%) 19348.00 <0.00> (-2.00)
Impacted Files Coverage Δ Complexity Δ
core/Controller/LoginController.php 90.65% <100.00%> (ø) 44.00 <0.00> (+1.00)
lib/private/User/Session.php 78.96% <100.00%> (+0.16%) 152.00 <0.00> (+2.00)
settings/templates/panels/personal/cors.php 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (ø%)
settings/Panels/Personal/Cors.php 50.00% <0.00%> (-50.00%) 4.00% <0.00%> (ø%)
lib/private/Settings/SettingsManager.php 72.78% <0.00%> (-0.18%) 44.00% <0.00%> (ø%)
lib/private/Lock/Persistent/LockManager.php 98.14% <0.00%> (-0.13%) 22.00% <0.00%> (-1.00%)
settings/Controller/CorsController.php 97.50% <0.00%> (-0.07%) 10.00% <0.00%> (ø%)
settings/Panels/Admin/PersistentLocking.php
...tings/templates/panels/admin/persistentlocking.php
settings/Application.php 53.98% <0.00%> (+0.32%) 2.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebcb50f...dbf9c06. Read the comment docs.

config/config.sample.php Outdated Show resolved Hide resolved
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.

5 participants