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

Updated JumpCloud authentication example #9722

Merged
merged 2 commits into from Feb 15, 2019

Conversation

Projects
None yet
3 participants
@dsmfool
Copy link
Contributor

commented Jan 23, 2019

Updated JumpCloud authentication example. Relevant Community Support Thread: https://community.librenms.org/t/ldap-with-jumpcloud/6883

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

Updated JumpCloud authentication example
Updated JumpCloud authentication example.  Relevant Community Support Thread: https://community.librenms.org/t/ldap-with-jumpcloud/6883
@CLAassistant

This comment has been minimized.

Copy link

commented Jan 23, 2019

CLA assistant check
All committers have signed the CLA.

@@ -182,17 +182,20 @@ An example config setup for use with Jumpcloud LDAP as a service is:

```php
$config['auth_mechanism'] = "ldap";
unset($config['auth_ldap_group']); #Remove built-in group
unset($config['auth_ldap_groups']); #Remove built-in groups

This comment has been minimized.

Copy link
@murrant

murrant Jan 23, 2019

Member

Instead of unset(), I'd say overwrite them like this:

$config['auth_ldap_groups'] = [
    '{admin_group}' => ['level' => 10],
    '{global_readonly_group}' => ['level' => 5],
];

This comment has been minimized.

Copy link
@dsmfool

dsmfool Jan 23, 2019

Author Contributor

This code block works, but I only have one group for my LibreNMS admins. My suggested change is for a basic example. If you don't unset the variable it just appends your desired group to the defaults. Perhaps include this codeblock in an advanced example?

This comment has been minimized.

Copy link
@murrant

murrant Jan 24, 2019

Member
$config['auth_ldap_groups'] = [
    '{admin_group}' => ['level' => 10],
];

boom, only one group. I added the second to illustrate how multiple groups would be added. (rather than duplicating the entire entry like the current docs)

This comment has been minimized.

Copy link
@murrant

murrant Jan 24, 2019

Member

Honestly, it is probably best if we remove the default groups entirely. I can't imagine why they are set.

@@ -182,17 +182,20 @@ An example config setup for use with Jumpcloud LDAP as a service is:

```php
$config['auth_mechanism'] = "ldap";
unset($config['auth_ldap_group']); #Remove built-in group

This comment has been minimized.

Copy link
@murrant

murrant Jan 23, 2019

Member

This can be omitted it is unneeded. LibreNMS removes the default group if it is unchanged.

This comment has been minimized.

Copy link
@dsmfool

dsmfool Jan 23, 2019

Author Contributor

With this line removed the setting still remains in the "Global Settings" page as:
auth_ldap_group | cn=groupname,ou=groups,dc=example,dc=com

I would rather not see the default or placeholder setting. Though, it seems like this parameter isn't required at all for successful authentication.

This comment has been minimized.

Copy link
@murrant

murrant Jan 24, 2019

Member

@dsmfool yeah, the code discards any entries that are set to that.

Feel free to keep the unset in your local config. I just don't think it should be in the docs ;)

This comment has been minimized.

Copy link
@dsmfool

dsmfool Jan 24, 2019

Author Contributor

I agree that it shouldn't have to be unset in the docs, but it should perhaps be shown how to remove them or overwrite them until the default groups are removed.

I'm not very familiar with Git, and unaware how to modify my commit for this pull request to reflect these requested changes to my pull request.

This comment has been minimized.

Copy link
@murrant

murrant Jan 25, 2019

Member

@dsmfool go to the files changed tab and click the edit button. That will allow you to edit this file.

You can cancel and browse around to other files in that branch to edit them too if you want.

This comment has been minimized.

Copy link
@dsmfool

dsmfool Jan 28, 2019

Author Contributor

Done. Hopefully I did it correctly.

Updated JumpCloud authentication example.
Updated JumpCloud authentication example in accordance with @murrant review.

@murrant murrant merged commit 4504b20 into librenms:master Feb 15, 2019

5 of 6 checks passed

codeclimate Code Climate encountered an error attempting to analyze this pull request.
Details
Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details

funzoneq added a commit to funzoneq/librenms that referenced this pull request Apr 30, 2019

Updated JumpCloud authentication example (librenms#9722)
* Updated JumpCloud authentication example

Updated JumpCloud authentication example.  Relevant Community Support Thread: https://community.librenms.org/t/ldap-with-jumpcloud/6883

* Updated JumpCloud authentication example.

Updated JumpCloud authentication example in accordance with @murrant review.

@lock lock bot locked as resolved and limited conversation to collaborators May 3, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.