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

Enable logging for LDAP users #2098

Merged
merged 1 commit into from
Feb 24, 2018
Merged

Enable logging for LDAP users #2098

merged 1 commit into from
Feb 24, 2018

Conversation

keegan
Copy link
Contributor

@keegan keegan commented Feb 23, 2018

No description provided.

@@ -114,7 +114,7 @@ function ldapAuth(manager, client, user, password, callback) {
// auth plugin API
function callbackWrapper(valid) {
if (valid && !client) {
manager.addUser(user, null);
manager.addUser(user, null, true);
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove || false here please?
The only place that was not sending it as an argument was this one, so we can have no fallback now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have now added this

@astorije
Copy link
Member

I suggested that change so obviously I'm 👍 though biased 😅

The rationale behind this is as follows:

  • As opposed to local-auth users which are created from the command line (which prompts yes/no for disk logs), there is no such thing with LDAP, a new user gets created on first login, and is given a default value for logs (currently false, true after this PR)
  • Before this PR, there is no way to auto-set to true, all new users have logs disabled. After this PR, it will be the opposite scenario.
  • Add sqlite logging and reloading messages #1839 gets merged, it will be possible to disable logs globally with messageStore: none. Without this PR, it will still be impossible to enable logs by default even after Add sqlite logging and reloading messages #1839.
  • In Prompt admin for user log at user creation #903, we made the prompt for local-auth default to yes, so this is consistent.

It's pretty late but I hope I clarified the intent.
Once users can edit their settings from the UI, this will be much easier to deal with on a per-user case.

@astorije astorije added this to the 3.0.0 milestone Feb 23, 2018
@astorije astorije added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Feb 23, 2018
Copy link
Member

@astorije astorije left a comment

Choose a reason for hiding this comment

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

Thanks! Could you squash your commits into 1? 🙏

In case you don't know how to do this, use git rebase -i HEAD~, then replace pick on the second line with f, save and close, and 🎉, commits are squashed. Then you need to push with git push --force.

@keegan
Copy link
Contributor Author

keegan commented Feb 24, 2018

Sorry about that, I have now squashed the commits

@astorije
Copy link
Member

No need to be sorry, it's totally fine and sometimes makes it easier to review new changes after a first review ;)
I approved, hopefully we can get that merged and in a pre-release soon!

@astorije astorije requested a review from a team February 24, 2018 01:17
Copy link
Member

@xPaw xPaw left a comment

Choose a reason for hiding this comment

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

@keegan FYI, If you want github to link your commits to your account, add email you used in git as a secondary email on github.

@astorije astorije merged commit 7a6b560 into thelounge:master Feb 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants