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

Expose a syncLdapUser method which can be called by other authenticators or to sync signed-in and new users from Ldap #4159

Merged
merged 4 commits into from Sep 18, 2017

Conversation

Projects
None yet
3 participants
@gaspardpetit
Contributor

gaspardpetit commented Sep 17, 2017

Description

This changelist exposes a syncLdapUser method which creates or sync a user from Ldap.

Motivation and Context

At the moment, SSO authentication creates a user based on HTTP headers and default values. Instead, it would be useful to be able to authenticate a user using SSO but then sync the user roles and information from Ldap if Ldap is available.

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.
Expose a syncLdapUser method which can be called by other authenticat…
…or to sync signed-in and new users from Ldap
@CLAassistant

This comment has been minimized.

CLAassistant commented Sep 17, 2017

CLA assistant check
All committers have signed the CLA.

public boolean isEnabled() {
final LdapSettings ldapSettings = ldapSettingsService.load();
return ldapSettings != null && ldapSettings.isEnabled();
}
public void syncLdapUser(String principal) {

This comment has been minimized.

@joschi

joschi Sep 18, 2017

Contributor

The method signature should allow users to at least verify if the synchronization was successful, so instead of returningvoid, it should return a boolean value (with true == successful, false == unsuccessful).

Alternatively return the User instance which was synced and mark the method as @Nullable.

This comment has been minimized.

@gaspardpetit

gaspardpetit Sep 18, 2017

Contributor

Completely agree.

Return User when syncing Ldap User instead of void to let caller know…
… the sync was succesful

Signed-off-by: Gaspard Petit <gaspard.petit@eidosmontreal.com>
public User syncLdapUser(String principal) {
final LdapSettings ldapSettings = ldapSettingsService.load();
if (ldapSettings == null || !ldapSettings.isEnabled()) {

This comment has been minimized.

@joschi

joschi Sep 18, 2017

Contributor

This could be simplified to:

if (!isEnabled()) {
    // ...
}

This comment has been minimized.

@gaspardpetit

gaspardpetit Sep 18, 2017

Contributor

Agreed, I tried to follow the same style as in doGetAuthenticationInfo - would you be ok if I changed it in both places?

This comment has been minimized.

@joschi

joschi Sep 18, 2017

Contributor

@gaspardpetit Yes, that would be great! 👍

// do not try to look a token up in LDAP if there is no principal or password
if (isNullOrEmpty(principal)) {
LOG.debug("Principal wsa empty. Not trying to sync user with LDAP.");

This comment has been minimized.

@joschi

joschi Sep 18, 2017

Contributor

Not originally from your changes, but it would be a great opportunity to fix this typo ("wsa" ➡️ "was"). 😉

gaspardpetit added some commits Sep 18, 2017

Fixed typo in debug message
Signed-off-by: Gaspard Petit <gaspard.petit@eidosmontreal.com>
reuse isEnabled() method instead of duplicating logic for isEnabled
Signed-off-by: Gaspard Petit <gaspard.petit@eidosmontreal.com>

@joschi joschi added this to the 2.4.0 milestone Sep 18, 2017

@joschi joschi self-assigned this Sep 18, 2017

@joschi joschi added the improvement label Sep 18, 2017

@joschi

joschi approved these changes Sep 18, 2017

@gaspardpetit gaspardpetit changed the title from Expose a syncLdapUser method which can be called by other authenticat or to sync signed-in and new users from Ldap to Expose a syncLdapUser method which can be called by other authenticators or to sync signed-in and new users from Ldap Sep 18, 2017

@joschi joschi merged commit 1e69090 into Graylog2:master Sep 18, 2017

4 checks passed

ci-web-linter Jenkins build graylog-pr-linter-check 1917 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
graylog-project/pr Jenkins build graylog-project-pr-snapshot 460 has succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@gaspardpetit gaspardpetit deleted the eidosmontreal:expose_syncldapuser_method branch Sep 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment