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

Drop ldap restrition #2482

Open
wants to merge 1 commit into
base: support/2.12.0
Choose a base branch
from

Conversation

btry
Copy link
Collaborator

@btry btry commented Nov 30, 2021

internal ref 22945 23259

Not converted to query builder, should be restricted depending on the grand parent form, but possible with a new question

Signed-off-by: Thierry Bugier <tbugier@teclib.com>
@AdrienClairembault
Copy link
Contributor

AdrienClairembault commented Dec 1, 2021

The changes were introduced by #1437.
Is the discussion in this PR no longer valid ?
Wouldn't this change allow you to see an LDAP from another entity that you do not have access to ?.

@btry
Copy link
Collaborator Author

btry commented Dec 1, 2021

I see several problems with this implemetnation

  • access to all LDAP when the user is in the root entity. The root entity does not grants more privileges than others.

  • LDAPs are not linked to an entity, then entity restriction is not applicable.

  • An entity may be linked to a LDAP to search for users, this type of relation should not be used to restrict the available directories when editing a question (see below)

image

To edit forms, the user needs to have UPDATE right on entities. This means that the user also have rights to change the LDAP associated to the current entity (and maybe others).

Maybe LDAP questions should be restricted to people able to edit general config of GLPI. See https://github.com/glpi-project/glpi/blob/9.5/bugfixes/front/authldap.form.php#L35

@AdrienClairembault
Copy link
Contributor

AdrienClairembault commented Dec 1, 2021

LDAPs are not linked to an entity, then entity restriction is not applicable.

Ok, seems fine to DROP this restriction then.

It does feel bad not being able to filter LDAP on some criteria but if nothing exist we can't do much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants