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

disable password confirmation with SSO #7487

Merged
merged 2 commits into from
Jan 3, 2018
Merged

Conversation

schiessle
Copy link
Member

Disable password confirmation in case of single-sign-on.
In case of SSO password confirmation doesn't work because Nextcloud can't check the users password.

@schiessle schiessle added 3. to review Waiting for reviews bug labels Dec 13, 2017
@schiessle schiessle added this to the Nextcloud 13 milestone Dec 13, 2017
@codecov
Copy link

codecov bot commented Dec 13, 2017

Codecov Report

Merging #7487 into master will increase coverage by 0.01%.
The diff coverage is 70.96%.

@@             Coverage Diff              @@
##             master    #7487      +/-   ##
============================================
+ Coverage     51.17%   51.18%   +0.01%     
- Complexity    24887    24892       +5     
============================================
  Files          1601     1602       +1     
  Lines         94752    94774      +22     
  Branches       1368     1369       +1     
============================================
+ Hits          48489    48510      +21     
- Misses        46263    46264       +1
Impacted Files Coverage Δ Complexity Δ
...amework/Middleware/Security/SecurityMiddleware.php 80.26% <0%> (+2.48%) 25 <0> (-2) ⬇️
core/js/js.js 63.55% <0%> (ø) 0 <0> (ø) ⬇️
lib/private/Template/JSConfigHelper.php 0% <0%> (ø) 18 <0> (+1) ⬆️
...e/AppFramework/DependencyInjection/DIContainer.php 78.6% <100%> (+0.77%) 27 <0> (ø) ⬇️
...leware/Security/PasswordConfirmationMiddleware.php 82.35% <82.35%> (ø) 6 <6> (?)
lib/private/Security/CertificateManager.php 91.08% <0%> (-1%) 39% <0%> (ø)
apps/files_trashbin/lib/Trashbin.php 72.28% <0%> (-0.25%) 136% <0%> (ø)
lib/private/Server.php 81.55% <0%> (+0.11%) 134% <0%> (ø) ⬇️
apps/files_trashbin/lib/Expiration.php 91.93% <0%> (+1.61%) 29% <0%> (ø) ⬇️
... and 1 more

@blizzz
Copy link
Member

blizzz commented Jan 2, 2018

Can we have unit tests for the middle ware that test positive as well as negative cases?

@rullzer
Copy link
Member

rullzer commented Jan 2, 2018

Pfff right. I knew I disliked all this functionality in 1 middleware. I'd like to just add an additional one that we can properly test.

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>
@rullzer rullzer force-pushed the no-password-confirm-with-sso branch from eaa8d08 to 763874a Compare January 2, 2018 20:33
@rullzer
Copy link
Member

rullzer commented Jan 2, 2018

  • rebased
  • Moved password confirmation to own middleware
  • Added new middleware tests

Add tests

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer force-pushed the no-password-confirm-with-sso branch from 763874a to 5705014 Compare January 2, 2018 20:58

$lastConfirm = (int) $this->session->get('last-password-confirm');
// we can't check the password against a SAML backend, so skip password confirmation in this case
if ($backendClassName !== 'user_saml' && $lastConfirm < ($this->timeFactory->getTime() - (30 * 60 + 15))) { // allow 15 seconds delay
Copy link
Member

Choose a reason for hiding this comment

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

I don't like it is hard-coded… if we can test for an instance (→ OCP) that would be much better. Perhaps for 14, for now (and backporting?) good enough.

Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

cannot test, but code looks good and i am confident with the unit tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants