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

Prevent LDAP Password to appear in log file #32589

Merged
merged 1 commit into from
Sep 6, 2018
Merged

Conversation

voroyam
Copy link
Contributor

@voroyam voroyam commented Sep 5, 2018

If connection to LDAP server is lost, the LDAP password is shown in the stack trace.

This should prevent this from happening.

@CLAassistant
Copy link

CLAassistant commented Sep 5, 2018

CLA assistant check
All committers have signed the CLA.

@cdamken
Copy link
Contributor

cdamken commented Sep 5, 2018

@tomneedham @PVince81 @butonic Could you please review this?

@PVince81 PVince81 self-assigned this Sep 5, 2018
'tryLogin'
'tryLogin',

//bind
Copy link
Contributor

Choose a reason for hiding this comment

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

these comments look useless...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are, any suggestions how to improve them?

Copy link
Contributor

Choose a reason for hiding this comment

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

no need to improve now

I was just pointing at the fact that the comment is exactly the same as the string itself, so doesn't have much value.

doesn't matter now, let's merge when CI is green

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81
Copy link
Contributor

PVince81 commented Sep 5, 2018

@voroyam please backport to stable10

@voroyam
Copy link
Contributor Author

voroyam commented Sep 5, 2018

@PVince81 don't know how to backport, already told @tom this.

@PVince81
Copy link
Contributor

PVince81 commented Sep 5, 2018

stable10: #32592

If connection to LDAP server is lost, the LDAP password is shown in the stacktrace.

This should prevent this from happening.
@PVince81 PVince81 force-pushed the ldap-password-logfile branch from 201a67e to 21f0c99 Compare September 5, 2018 15:23
@PVince81
Copy link
Contributor

PVince81 commented Sep 5, 2018

rebased

@codecov
Copy link

codecov bot commented Sep 5, 2018

Codecov Report

Merging #32589 into master will increase coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #32589      +/-   ##
============================================
+ Coverage     64.14%   64.21%   +0.07%     
- Complexity    18672    18727      +55     
============================================
  Files          1177     1177              
  Lines         70275    70499     +224     
  Branches       1270     1270              
============================================
+ Hits          45076    45273     +197     
- Misses        24829    24856      +27     
  Partials        370      370
Flag Coverage Δ Complexity Δ
#javascript 52.89% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.5% <ø> (+0.07%) 18727 <ø> (+55) ⬆️
Impacted Files Coverage Δ Complexity Δ
lib/private/Log.php 82.53% <ø> (ø) 49 <0> (ø) ⬇️
lib/private/User/Database.php 73.23% <0%> (+1.19%) 61% <0%> (+18%) ⬆️
lib/private/User/User.php 89.93% <0%> (+1.76%) 103% <0%> (+31%) ⬆️
lib/public/Util.php 63.88% <0%> (+6.82%) 83% <0%> (+6%) ⬆️

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 5ce6bde...21f0c99. Read the comment docs.

@patrickjahns
Copy link
Contributor

Regarding the comments about useless string - suggestion, mention that the two methods bind // invokeLDAPMethod result from user_ldap.

// user_ldap app 
'bind'
'invokeLDAPMethod'

@PVince81 PVince81 merged commit 33f8990 into master Sep 6, 2018
@PVince81 PVince81 deleted the ldap-password-logfile branch September 6, 2018 06:23
@lock lock bot locked as resolved and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants