Add an option for ad authentication to have a default level #4801

Merged
merged 3 commits into from Oct 21, 2016

Projects

None yet

7 participants

@jonathon-k
Contributor

Please note

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

Add a config option to allow modifying the default user level for AD authentication when auth_ad_require_groupmembership == 0.

@murrant
Contributor
murrant commented Oct 15, 2016

I am against this change. Requiring the admin to configure a group to grant higher permissions is not that difficult and also reduces the change of accidental permission elevation.

@jonathon-k
Contributor

My rationale is two fold, one I don't see what is accomplished by allowing
people to authenticate with user level 0, and two the case of an amin who
doesn't have control over AD and has to setup the same permission for every
group.

The second one is a thin reason, I know, unless your AD admin thinks that
no common groups should exist...

Thoughts?

On Oct 14, 2016 6:56 PM, "Tony Murray" notifications@github.com wrote:

I am against this change. Requiring the admin to configure a group to
grant higher permissions is not that difficult and also reduces the change
of accidental permission elevation.

โ€”
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#4801 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AUYaP7XMgaZeO3GSw0geCW6cpWl6LMHnks5q0CSrgaJpZM4KXh9H
.

@murrant
Contributor
murrant commented Oct 15, 2016

Once the user is created at level 0, you can grant them access to devices and ports specifically. So they only see those items.

As for second option, maybe, but it isn't that likely.

@Gorian
Contributor
Gorian commented Oct 16, 2016 edited

I am that admin that doesn't have control over AD.... in large corporations, it happens all the time. You have a small group in charge of AD, and lots of other groups that use it. In this case, the AD I'm trying to authenticate to has no group that everyone is in. Instead, we have groups that all start with "world.gs". For other applications, like Graylog, I can just do a wildcard on the group for "world.gs.*" and it works fine. No such option for LibreNMS.

In my situation, the AD I'm binding to is not a global AD, therefore I can assume everyone who has been granted access to it can also have read-only access to the monitoring system by default.

@murrant
Contributor
murrant commented Oct 17, 2016

Btw, I am not the final say here, just offering my opinion. Others should speak up if they want this feature.

I would feel much better about it if it was limited to global read permission and couldn't grant admin access.

@jonathon-k
Contributor
jonathon-k commented Oct 17, 2016 edited

Changing it to a flag specifically for giving global read access
permissions, rather than setting any default access level. Probably makes
more sense -- I can't imagine this type of systems having a lot of admins
in different user groups...

On Mon, Oct 17, 2016 at 1:50 PM, Tony Murray notifications@github.com
wrote:

Btw, I am not the final say here, just offering my opinion. Others should
speak up if they want this feature.

I would feel much better about it if it was limited to global read
permission and couldn't grant admin access.

โ€”
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#4801 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AUYaP9RJGXOU6KVqt1CxhPIbqlmpLdcRks5q09FrgaJpZM4KXh9H
.

@breeman1

I would love to see this feature implemented as we are in the same predicament as @Gorian.

@jonathon-k
Contributor

If implemented as a flag, would it make sense to allow specifying groups with more restricted privileges or should this override any level below 5 (global read-only)

@murrant
Contributor
murrant commented Oct 18, 2016

Any defined group should override the default level, even if it is lower.

Jonathon Koyle rework as a flag indicating unspecified access is global read
de609cd
@laf
Member
laf commented Oct 20, 2016

I'm not an ad user so can't offer any comments on this but it does need some style fixes :)

@murrant murrant Fix indentation
7d3dc59
@scrutinizer-notifier

The inspection completed: No new issues

@murrant murrant merged commit 65f7421 into librenms:master Oct 21, 2016

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment