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

Several Suggestions for improvement #231

Open
tylkomat opened this issue Oct 30, 2024 · 3 comments
Open

Several Suggestions for improvement #231

tylkomat opened this issue Oct 30, 2024 · 3 comments

Comments

@tylkomat
Copy link

tylkomat commented Oct 30, 2024

We have a fairly complex LDAP structure, but not unusual. As far as I know it is a common structure for openLDAP, but it's one that is not fully supported by this plugin.

dc=example,dc=com/
├─ ou=IT-Services/
│  ├─ ou=WWW/
│  │  ├─ ou=group1/
│  │  │  ├─ ou=typo3/
│  │  │  │  ├─ ou=be/
│  │  │  │  │  ├─ cn=John Doe (alias to cn=John Doe,ou=People,dc=example,dc=com)
├─ ou=People/
│  ├─ cn=John Doe

This structure poses some issues. Here you see our partial configuration, but this configuration does not work without changes to the code base.

BE_USERS
Base DN: ou=People,dc=example,dc=com
Filter: (&(objectClass=shadowAccount)(uid={USERNAME}))
Mapping:
realName = <displayname>
email = <mailAddressbook>
username = <uid>

BE_GROUPS
Base DN: ou=WWW,ou=IT-Services,dc=example,dc=com
Filter: (&(objectClass=alias)(aliasedObjectName={USERDN})(ou:dn:=typo3)(ou:dn:=be))
Mapping:

tx_igldapssoauth_dn {
  data = field:dn
  replacement.10 {
    search = #cn=.+,ou=be,ou=typo3,(ou=.+)#
    replace = ${1}
    useRegExp = 1
  }
}
title {
  data = field:dn
  replacement.10 {
    search = #(cn=.+,ou=be,ou=typo3,)?ou=(.+),ou=WWW,ou=IT-Services,dc=example,dc=com#
    replace = ${2}
    useRegExp = 1
  }
}

I found the following issues:

  1. An option for following aliases is necessary
    Then, instead of filtering for all BE_USERS (we have over 10k and there is a hard stop of 2k in the code) I could just filter for the ones which are in groups, since these are the only ones that have access to the typo3. Additionally a filter for duplicates is necessary, as a user can be in many groups. Sorting by name in the end would be nice too.
  2. Not all groups are displayed (only 500 of 570) because pagination is missing in the groups code
  3. Because of our filter to map the groups to users, groups are duplicated per member and need to be filtered. Sorting by name would be nice too
  4. Existing groups are not recognised because the name has to be extracted using typoscript. This extraction happens after the check if the group already exists locally. In the Import LDAP groups (Backend) table title and DN look correct, but nothing is marked green and importing creates a new group, although it already exists.

I'm willing to adopt my previous PR #209 to the new version if you are interested.

@xperseguers
Copy link
Owner

Hello @tylkomat, thanks for describing a bit more what you need and why you created the previous PR #209. Some of the issues you found are "clear", some are not, and some of the 4 points you described should certainly be broken down a bit more. My big problem with PR #209 was that I just couldn't follow the reason of all the changes.

So my suggestion is as follows, instead of working in a single big PR that fits all your needs, let's break down at least the first few problems which could be useful to anyone and are easily understandable and let's "complete" that ticket once first bits are already in place.

I can think of this:

More than 500 groups

Support for following aliases

  • It's quite "clear" the purpose of the support, maybe we can think of a way to follow automatically if it's logical, ideally I'd like to avoid having yet another obscure flag to the configuration which is already a bit complex due to edge-cases. Maybe we really need an option for that, unsure, but let's discuss that separately.

Other points which may need some further explanation

  • in 1) Filter for duplicates, unclear right now by reading what it's needed, what's the use case, and why you relate that to the following of aliases
  • in 1) Sorting by name (users or groups, didn't follow clearly by reading), this can certainly be broken down more clearly
  • in 3) You speak of group duplication, so maybe you had duplicates for users in the first place? Let's discuss that "duplication" problem in the context of following aliases then
  • in 3) You speak of sorting by name for the groups, so maybe the problem exists for both. Let's discuss the "duplication" problem with some more info

Finally in 4) there seems to be a problem with "existing groups not recognized". Let's discuss and fix that problem separately.

Bottom Line

I appreciate having a ticket explaining the "big picture" of changes you'd like to see. I think that it makes sense now to do that a bit step by step with dedicated tasks which are easier to grasp and can logically be of some benefit already when applied step-by-step with having to wait for a big PR with lots of changes.

@tylkomat
Copy link
Author

tylkomat commented Nov 1, 2024

I think that it makes sense now to do that a bit step by step with dedicated tasks which are easier to grasp and can logically be of some benefit already when applied step-by-step with having to wait for a big PR with lots of changes.
Of course, I forgot to add that sentence

Duplication

dc=example,dc=com/
├─ ou=IT-Services/
│  ├─ ou=WWW/
│  │  ├─ ou=group1/
│  │  │  ├─ ou=typo3/
│  │  │  │  ├─ ou=be/
│  │  │  │  │  ├─ cn=John Doe (alias to cn=John Doe,ou=People,dc=example,dc=com)
│  │  │  │  │  ├─ cn=Jimmy Newtron (alias to cn=Jimmy Newtron,ou=People,dc=example,dc=com)
│  │  ├─ ou=group2/
│  │  │  ├─ ou=typo3/
│  │  │  │  ├─ ou=be/
│  │  │  │  │  ├─ cn=John Doe (alias to cn=John Doe,ou=People,dc=example,dc=com)
├─ ou=People/
│  ├─ cn=John Doe
│  ├─ cn=Jimmy Newtron

In this example we have group1 and group2 as examples, with DNs:

  • ou=group1,ou=WWW,ou=IT-Services,dc=example,dc=com and
  • ou=group2,ou=WWW,ou=IT-Services,dc=example,dc=com

For the groups I use the following filter and base DN:

  • Base DN: ou=WWW,ou=IT-Services,dc=example,dc=com
  • Filter: (&(objectClass=alias)(aliasedObjectName={USERDN})(ou:dn:=typo3)(ou:dn:=be))

I suppose the {USERDN} placeholder is necessary to connect the group to the user, otherwise the filter could be much simpler and not generate duplicates, but in this case the filter gives the following result without mapping:

  • cn=John Doe,ou=be,ou=typo3,ou=group1,ou=WWW,ou=IT-Services,dc=example,dc=com
  • cn=Jimmy Newtron,ou=be,ou=typo3,ou=group1,ou=WWW,ou=IT-Services,dc=example,dc=com
  • cn=John Doe,ou=be,ou=typo3,ou=group2,ou=WWW,ou=IT-Services,dc=example,dc=com

After applying my mapping the "Import LDAP groups (Backend)" table would like this:

Name DN
group1 ou=group1,ou=WWW,ou=IT-Services,dc=example,dc=com
group1 ou=group1,ou=WWW,ou=IT-Services,dc=example,dc=com
group2 ou=group2,ou=WWW,ou=IT-Services,dc=example,dc=com

As you can see each member of a group will result in an additional duplicate.

@tylkomat
Copy link
Author

tylkomat commented Nov 1, 2024

following Aliases

According to this FAQ https://www.openldap.org/faq/data/cache/1111.html
Alias entries have the properties:

objectclass alias
objectclass extensibleObject
aliasedobjectname {DN of the Target}

It could be recognized automatically without a separate option, if the schema is always like this.

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

No branches or pull requests

2 participants