Adding active directory authentication to librenms #2411

Merged
merged 10 commits into from Nov 10, 2015

Projects

None yet

4 participants

@fstern
fstern commented Nov 10, 2015

This adds Microsoft Active Directory authentication to librenms.

This makes it easier than using the already existing LDAP authentication plugin. Adding Active Directory authentication to the existing LDAP plugin would have required a nearly complete rewrite.
Also sticking to just Active Directory let me make some assumptions of attributes being in place already.

This plugin has been in use for about a month now without any apparent problems.

Falk Stern added some commits Oct 16, 2015
Falk Stern Initial commit a36f3e1
Falk Stern Added actual documentation for active_directory auth f87360b
Falk Stern Authenticate against active directory 93b5704
Falk Stern Checking for groups now 779c90b
Falk Stern Documented all config options 8e0a95a
Falk Stern Merge remote-tracking branch 'upstream/master' into active_directory_…
…auth
a785398
Falk Stern Fixed a bug from scrutinizer
0eeb4d2
@laf
Member
laf commented Nov 10, 2015

This is awesome @fstern

Scrutinizer has picked up on some small issues, the only things of any worth changing is the functions that just return 0, you can remove the variables that are passed to them. I.e:

function reauthenticate($sess_id, $token) {

to

function reauthenticate() {
@laf
Member
laf commented Nov 10, 2015

Actually one other thing, where you have:

} else {
or
} elseif {

Can you change these to:

}
else {
or

}
elseif {
Falk Stern added some commits Nov 10, 2015
Falk Stern Removed unused variables and updated coding style 2326061
Falk Stern Coding style
16df0fd
Falk Stern Removed unused variables
d326869
@fstern
fstern commented Nov 10, 2015

@laf Changes are incorporated, thanks for looking over it.

@laf laf merged commit ea8575a into librenms:master Nov 10, 2015

2 checks passed

Auto-Deploy Build finished. No test results found.
Details
Scrutinizer 57 new issues
Details
@paulgear
Member

Hi @fstern - thanks for this. I know it's already been merged, but would you mind changing the 'auth_ad_dont_check_certificates' option to be 'auth_ad_check_certificates'? Options should consistently be used for turning features on, not off. You can keep the same semantics by changing the default value to the opposite.

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