Skip to content

If token based authentication is mandatory a login failed entry is wr… - #31909

Merged
DeepDiver1975 merged 1 commit into
masterfrom
bugfix/log-failed-login-if-no-token-is-used
Jun 29, 2018
Merged

If token based authentication is mandatory a login failed entry is wr…#31909
DeepDiver1975 merged 1 commit into
masterfrom
bugfix/log-failed-login-if-no-token-is-used

Conversation

@DeepDiver1975

@DeepDiver1975 DeepDiver1975 commented Jun 26, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

…itten to the log so that fail2ban can handle this case.

Description

A log message is written in case a user does not login using an app password if 'token_auth_enforced' is set to true

Related Issue

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

How Has This Been Tested?

  • unit tests executed
  • manually

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)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
@DeepDiver1975 DeepDiver1975 added this to the development milestone Jun 26, 2018
@DeepDiver1975 DeepDiver1975 self-assigned this Jun 26, 2018
@codecov

codecov Bot commented Jun 26, 2018 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #31909 into master will increase coverage by 15.58%.
The diff coverage is 42.85%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #31909       +/-   ##
=============================================
+ Coverage     47.83%   63.42%   +15.58%     
- Complexity        0    18499    +18499     
=============================================
  Files           109     1165     +1056     
  Lines         10379    69386    +59007     
  Branches       1264     1264               
=============================================
+ Hits           4965    44006    +39041     
- Misses         5045    25011    +19966     
  Partials        369      369
Flag Coverage Δ Complexity Δ
#javascript 52.58% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 64.66% <42.85%> (+27.34%) 18499 <0> (+18499) ⬆️
Impacted Files Coverage Δ Complexity Δ
lib/private/User/Session.php 77.96% <42.85%> (ø) 145 <0> (?)
apps/dav/lib/Connector/Sabre/Server.php 100% <0%> (ø) 1% <0%> (?)
lib/private/Session/Internal.php 0% <0%> (ø) 19% <0%> (?)
lib/private/Http/Client/Client.php 86% <0%> (ø) 17% <0%> (?)
lib/public/AppFramework/Http/JSONResponse.php 94.44% <0%> (ø) 7% <0%> (?)
lib/private/Repair/RepairSubShares.php 81.25% <0%> (ø) 7% <0%> (?)
core/templates/lostpassword/notify.php 100% <0%> (ø) 0% <0%> (?)
lib/private/Encryption/EncryptionWrapper.php 100% <0%> (ø) 6% <0%> (?)
.../Exceptions/EncryptionHeaderKeyExistsException.php 100% <0%> (ø) 1% <0%> (?)
...AppFramework/Utility/ControllerMethodReflector.php 100% <0%> (ø) 10% <0%> (?)
... and 1048 more

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 08f41dc...b6e0ec8. Read the comment docs.
@DeepDiver1975
DeepDiver1975 force-pushed the bugfix/log-failed-login-if-no-token-is-used branch from 4bc44d4 to b3a03a0 Compare June 26, 2018 08:15
PVince81
PVince81 reviewed Jun 26, 2018
View reviewed changes
Comment thread lib/private/User/Session.php
throw new \InvalidArgumentException('$user cannot be empty');
}
if (!$isTokenPassword && $this->isTokenAuthEnforced()) {
$this->logger->warning("Login failed: '$user' (Remote IP: '{$request->getRemoteAddress()}')", ['app' => 'core']);

@PVince81 PVince81 Jun 26, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we also emit a symfony event ? I don't remember whether we also emit events on failure

@PVince81 PVince81 Jun 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@sharidas are we logging symfony events for login failures as well ?

@sharidas sharidas Jun 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes the login failures are addressed #31467. This method https://github.com/owncloud/core/blob/master/lib/private/User/Session.php#L1078 is called for the same.

@DeepDiver1975 DeepDiver1975 Jun 28, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

will emit the event ..... gimme a sec ...
@DeepDiver1975
DeepDiver1975 force-pushed the bugfix/log-failed-login-if-no-token-is-used branch from b3a03a0 to b6e0ec8 Compare June 28, 2018 11:12
@DeepDiver1975
If token based authentication is mandatory a login failed entry is wr…
0bdf703 
…itten to the log so that fail2ban can handle this case.
@DeepDiver1975
DeepDiver1975 force-pushed the bugfix/log-failed-login-if-no-token-is-used branch from b6e0ec8 to 0bdf703 Compare June 28, 2018 12:28
@phil-davis

phil-davis commented Jun 29, 2018

Copy link
Copy Markdown
Contributor

Tested against this branch just now.

  • with 10.0.9RC2 and log level 0 the PasswordLoginForbiddenException is logged, but no "Login failed" in the log. This is the existing mis-behavior.
  • with this branch and log level 0 (debug) both "Login failed" and the PasswordLoginForbiddenException messages are logged.
  • with this branch and log level 2 (warning) the "Login failed" message only is logged.
  • with this branch and log level 3 (error) no message is logged (login fail is only warning level).

Works for me.
phil-davis
phil-davis approved these changes Jun 29, 2018
View reviewed changes

@phil-davis phil-davis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested as per my comment.
Others have reviewed the actual code.
@DeepDiver1975
DeepDiver1975 merged commit b05bdff into master Jun 29, 2018
@DeepDiver1975
DeepDiver1975 deleted the bugfix/log-failed-login-if-no-token-is-used branch June 29, 2018 07:54
@DeepDiver1975 DeepDiver1975 mentioned this pull request Jun 29, 2018
Merged
@PVince81 PVince81 mentioned this pull request Sep 10, 2018
Closed
@lock

lock Bot commented Jul 30, 2019

Copy link
Copy Markdown

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
@lock lock Bot locked as resolved and limited conversation to collaborators Jul 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

3 - To Review

4 participants

@DeepDiver1975 @phil-davis @PVince81 @sharidas