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

Log failed twofactor authentication #37401

Merged
merged 1 commit into from
Jun 15, 2020
Merged

Log failed twofactor authentication #37401

merged 1 commit into from
Jun 15, 2020

Conversation

pedro-vo
Copy link
Contributor

@pedro-vo pedro-vo commented May 17, 2020

Description

Log failed twofactor authentication

Related Issue

Motivation and Context

Possible log analysis of failed twofactor authentication

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Update test case

Checklist:

  • Code changes
  • Changelog item, see TEMPLATE

@CLAassistant
Copy link

CLAassistant commented May 17, 2020

CLA assistant check
All committers have signed the CLA.

@mmattel
Copy link
Contributor

mmattel commented May 18, 2020

Please add a changelog, you will find a template here: https://github.com/owncloud/core/tree/master/changelog

@pedro-vo
Copy link
Contributor Author

Please add a changelog, you will find a template here: https://github.com/owncloud/core/tree/master/changelog

Done.

@phil-davis phil-davis removed their request for review May 28, 2020 12:41
@owncloud owncloud deleted a comment from update-docs bot May 28, 2020
@phil-davis
Copy link
Contributor

@micbar or ?
who should review this?

@micbar micbar requested a review from DeepDiver1975 May 29, 2020 08:01
@micbar
Copy link
Contributor

micbar commented May 29, 2020

LGTM!
@DeepDiver1975 any thoughts?

Copy link
Contributor

@karakayasemi karakayasemi left a comment

Choose a reason for hiding this comment

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

To be sure that all failed two-factor authentications logged, imho, writing this log from TwoFactorAuthManager is better. Also, commits should be squashed before the merge.

Copy link
Contributor

@karakayasemi karakayasemi left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. The code changes look good. There are small problems left.
The base commit is pretty old, I guess that's why Codecov could not calculate the coverage. We need to rebase the branch to the top of the current master. Also, instead of 5 commits, please squash them as 1 meaningful commit when rebasing.
If you need any help, do not hesitate to ping me.

core/Application.php Outdated Show resolved Hide resolved
@DeepDiver1975 DeepDiver1975 removed their request for review June 15, 2020 07:54
@codecov
Copy link

codecov bot commented Jun 15, 2020

Codecov Report

Merging #37401 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #37401   +/-   ##
=========================================
  Coverage     64.67%   64.67%           
  Complexity    19339    19339           
=========================================
  Files          1279     1279           
  Lines         75569    75572    +3     
  Branches       1331     1331           
=========================================
+ Hits          48876    48879    +3     
  Misses        26301    26301           
  Partials        392      392           
Flag Coverage Δ Complexity Δ
#javascript 54.14% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.84% <100.00%> (+<0.01%) 19339.00 <1.00> (ø)
Impacted Files Coverage Δ Complexity Δ
...b/private/Authentication/TwoFactorAuth/Manager.php 93.61% <100.00%> (+0.43%) 19.00 <1.00> (ø)
lib/private/Server.php 85.18% <100.00%> (ø) 129.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 158bd97...2c8a784. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Jun 15, 2020

Codecov Report

Merging #37401 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #37401   +/-   ##
=========================================
  Coverage     64.67%   64.67%           
  Complexity    19339    19339           
=========================================
  Files          1279     1279           
  Lines         75569    75572    +3     
  Branches       1331     1331           
=========================================
+ Hits          48876    48879    +3     
  Misses        26301    26301           
  Partials        392      392           
Flag Coverage Δ Complexity Δ
#javascript 54.14% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.84% <100.00%> (+<0.01%) 19339.00 <1.00> (ø)
Impacted Files Coverage Δ Complexity Δ
...b/private/Authentication/TwoFactorAuth/Manager.php 93.61% <100.00%> (+0.43%) 19.00 <1.00> (ø)
lib/private/Server.php 85.18% <100.00%> (ø) 129.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 158bd97...2c8a784. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 15, 2020

Codecov Report

Merging #37401 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #37401   +/-   ##
=========================================
  Coverage     64.67%   64.67%           
  Complexity    19339    19339           
=========================================
  Files          1279     1279           
  Lines         75569    75573    +4     
  Branches       1331     1331           
=========================================
+ Hits          48876    48880    +4     
  Misses        26301    26301           
  Partials        392      392           
Flag Coverage Δ Complexity Δ
#javascript 54.14% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.84% <100.00%> (+<0.01%) 19339.00 <1.00> (ø)
Impacted Files Coverage Δ Complexity Δ
...b/private/Authentication/TwoFactorAuth/Manager.php 93.75% <100.00%> (+0.56%) 19.00 <1.00> (ø)
lib/private/Server.php 85.18% <100.00%> (ø) 129.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 158bd97...a7f67e6. Read the comment docs.

@karakayasemi karakayasemi merged commit 6038c8e into owncloud:master Jun 15, 2020
@davitol
Copy link
Contributor

davitol commented Jul 15, 2020

{"reqId":"mfURXZdJK2sIjdi2aPgb","level":2,"time":"2020-07-15T12:23:17+00:00","remoteAddr":"172.24.0.1","user":"admin","app":"core","method":"POST","url":"\/login\/challenge\/totp","message":"Two factor verify failed: 'admin' (Remote IP: '172.24.0.1')"}

That is the log that appears. I guess it is enough info, but just to confirm.

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.

Log failed twofactor authentication
7 participants