xperseguers / t3ext-ig_ldap_sso_auth

TYPO3 Extension ig_ldap_sso_auth. This extension provides LDAP and SSO support for TYPO3.
https://extensions.typo3.org/extension/ig_ldap_sso_auth
27 stars 64 forks source link

User Groups are not assigned locally using memberOf #72

Closed Azazel1333 closed 4 years ago

Azazel1333 commented 4 years ago

TYPO3 Version: 8.6 EXT Version 3.4.0 PHP: 7.3

LDAP is connecting to AD server.

Relevant configuration: In the extension I have keepFEGroups checked FE USER mapping: usergroup = \<memberOf> User contains membership information

However, when I import a new user from the fe user import tool, the user does not have the appropriate groups assigned. The groups are shown in the fe_groups import tool so I know the Base DN and filter is set correctly.

It seems that EVERYTHING else is working. All that I'm missing to make it so that the fe_groups are applied to the fe_users based on the memberOf field? I am sure I've tried every combination of settings to make this work. I even tried to do reverse mapping where groups contain member list and when I take that approach the fe_users are assigned EVERY group in the system.

Any assistance is GREATLY appreciated, since I have been working on trying to get this to work for ages now and am starting to think there's something more to this issue that I'm missing.

Thanks in advance.

pi-phi commented 4 years ago

reminds me to my problem #65

Azazel1333 commented 4 years ago

@pi-phi Did you ever find a solution? Login and everything works, but as stated keeping the groups assigned locally does not seem to happen. I do have the "keepFEGroups" checked off. However, it just doesn't seem to do anything at all.

Azazel1333 commented 4 years ago

After a very long time I've finally started to understand what is happening where within the code. Through a bunch of trial and error, I discovered what was preventing the groups from syncing:

ig_ldap_sso_auth/Classes/Library/LdapGroup.php

On line 52 there is an if statement that reads:

if(substr($groupDN, -strlen($baseDn)) !== $baseDn) { // Group $groupDn does not match the required baseDn for LDAP groups continue; }

By removing this check, all my groups sync perfectly. I did a bit of digging and it's simply because the Active Directory server we are using defines OU, and DC with upper case but in our extension configuration for the BASE DN for FE GROUPS we had ou and dc in lower case, making the check fail.

So I've now corrected my settings using proper UPPERCASE variables for defining the BaseDN.

I probably would have caught this sooner if the "Suggestion for Active Directory" also noted that upper case may be needed. I think this would be a nice change to prevent such issues for users in the future especially since the documentation suggests that:

ou=groups,dc=example,dc=com

is valid, when in my case,

OU=groups,DC=example,DC=com

is what was required.

Azazel1333 commented 4 years ago

SUGGESTION(S):

I feel that if groups are found and they don't match the BaseDN for FE GROUPS then some sort of notification could be provided by the Import tool. This would have saved me (and I'm sure others) hours.

For example, below the user row in the import tool, it could read: "0/20 groups synced, verify your FE_GROUPS Base DN. Ensure that the record matches exactly (upper / lower case)"

Or, even simpler, the check in file ig_ldap_sso_auth/Classes/Library/LdapGroup.php on line 52 could do a strtoupper when comparing DNs.

The only reason I didn't catch this was because the groups were coming over with lower case so I was confident the Base DN was 100% correct. It shouldn't work in one place and not in another. It feels like inconsistent code.

Flashdown commented 4 years ago

I think my PR solves your issue: https://github.com/xperseguers/t3ext-ig_ldap_sso_auth/pull/57 The issue is that the check is not case insensitive, but it's also possible to not use my patch and work around this issue by ensuring your notation of OU= DC= and so on is written in uppercase on all configuration Tabs.

Azazel1333 commented 4 years ago

I just reviewed PR #57 and yes it does correct the issue. For the time being, we will be ensuring upper case only!

I still believe somewhere it should suggest upper case as opposed to suggesting lower case (misleading), however that's fairly minor.

At any rate, I think this has been resolved. I appreciate the help / feedback I have received.

Thanks again!