search

389ds / 389-ds-base

The enterprise-class Open Source LDAP server for Linux
https://www.port389.org/
Other
211 stars 90 forks source link

RFE: memberof should not support circular groups by default #2256

Closed 389-ds-bot closed 4 years ago

389-ds-bot commented 4 years ago

Cloned from Pagure issue: https://pagure.io/389-ds-base/issue/49197

Issue Description

This is possible to define circular groups. For example, G0 is member of G1 and G1 is member of G0.

memberof supports circular groups (updates correctly memberof attribute) but disable ancestors cache. The update is then very slow.

The issue is that circular groups reveal something broken in the environment itself. memberof should fail when it detects circular groups (by default) unless a "new" flag says circular groups are supported

Package Version and Platform

Any versions

Steps to reproduce

  1. Enable memberof plugin 2 Create circular group and update one of them

Actual results

memberof correctly update memberof attribute

Expected results

By default it should fail as soon as it detects a circular group

389-ds-bot commented 4 years ago

Comment from tbordaz (@tbordaz) at 2017-03-29 09:06:30

Implementation should be easy but it exists a dependency on fixing a complex behavior.

Detecting circular group, memberof will fail, so the txn will be aborted. A problem is that memberof may detect circular group quite late. Meaning after having fixup many members. Those fixup were successful updating entry cache. Currently there is a pb invalidating updated entries (in the entry cache) when the txn that updated them is aborted.

389-ds-bot commented 4 years ago

Comment from tbordaz (@tbordaz) at 2017-03-29 09:06:41

Metadata Update from @tbordaz:

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2017-03-29 09:14:27

I'm against this change. I think it's easy to resolve circular groups, we have a poc algorithm to do this efficiently on complex scenarios (IE arbitrary length graphs).

This is avoiding the problem, rather than resolving it.

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2017-04-24 16:43:41

Metadata Update from @mreynolds389:

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2017-04-25 02:13:14

Given we mentioned this in the triage:

Simply, if you can detect a circular group you have already expended all of the effort to resolve it. Because once you are at that point, you may as well just finish writing the membership information.

So first, there is my memberOf algo python example, which handles this efficiently, and correctly. Second, if we detect this we can already just finish the work to fix it.

So I think that this "feature" is an anti feature. Because if we have this scenario:

A -> B -> C

Now, I join C to A, which link is "missing"? A's member's wont be a member of C? Or B to A? Or C to B? We now add a condition by which an admin has to add groups in certain orders to make expressions work. Plus, they may actually want a circular group - they are not a mistake. In huge domains sometimes they happen by accident (and this "feature" would mask that), in some places they want it.

As a result, I am against this ticket, and do not believe that we should implement it.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2017-04-25 09:31:44

We have discussed this further and come to the same conclusion. @tbordaz agrees we have already done the work and that we may as well perform the circular update if we have gotten to this point.

@elkris made an excellent point that this may break replication also. If we have master A and B, and we add some groups 1 and 2 on each, then on A we add 1 -> 2, and B 2 -> 1, on each they are happy, but on replication we would block the replication because they would respectively create a loop. Thus, A would only see 1->2 and B only 2->1.

As a result, I think that we should close this issue, and replace it with a new one: make circular group handling more effecient. As discussed, I have a POC algorithm that is capable of this, and it is highly efficient to operate.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2017-05-23 02:16:47

Closing based on IRC discussions.