Ad-auth need to add user to db for dashboards to work properly #2434

Merged
merged 9 commits into from Nov 17, 2015

Projects

None yet

3 participants

@vizay
Contributor
vizay commented Nov 16, 2015

The dashboard functionality relies on the fact that the user exists in the database. Currently the AD-authentication functionality doesn't implement this feature which breaks the usage of dashboards for anyone except the local admin account.

adduser inserts:

  • user_id to the user ID fetched from the AD
  • username to the sAMAccountname
  • level to 0 (security measure)
  • can_modify_passwd to 0 (security measure)

I've chosen to not implement the insertion of AD-users passwords into the database to keep things safer.
A thing to be considered here would be the "caching" (i.e insert password into the database) of AD-users in case of connection loss towards the AD, however I felt it was out of scope for this fix.

@laf laf and 1 other commented on an outdated diff Nov 16, 2015
html/includes/authentication/active_directory.inc.php
}
+function user_exists_in_db($username) {
+ $return = @dbFetchCell('SELECT COUNT(*) FROM users WHERE username = ?', array($username), true);
@laf
laf Nov 16, 2015 Member

Any reason why you're suppressing the dbFetchCell call with @? Just return the count and and check if it's > 0 maybe?

@vizay
vizay Nov 17, 2015 Contributor

Pure brainfart leaving it there, leftover from testing stuff so it can be removed.

Well spotted :)

@laf
Member
laf commented Nov 17, 2015

@vizay if you can update this PR that would be great.

We also need you to sign the contributors agreement, as an example: Lupul@0557f34

You can do this by submitting an additional commit to this PR.

@vizay vizay Update active_directory.inc.php
6e78fc8
vizay added some commits Nov 17, 2015
@vizay vizay Update AUTHORS.md
f7ad46e
@vizay vizay Update AUTHORS.md
139e6f1
@vizay vizay I agree to the conditions of the Contributor Agreement contained in d…
…oc/General/Contributing.md.
f9c6941
@laf laf merged commit 6e1e6b6 into librenms:master Nov 17, 2015

2 checks passed

Auto-Deploy Build finished. No test results found.
Details
Scrutinizer 47 new issues
Details
@vizay vizay deleted the unknown repository branch Dec 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment