search

Adambean / gitlab-ce-ldap-sync

Synchronise users and groups (including group members) from an LDAP instance with Gitlab CE (and EE in free tier) self-hosted instance(s).
Apache License 2.0
59 stars 23 forks source link

Gitlab LDAP sync script removes dashes from LDAP group names. #43

Closed TafkaMax closed 6 months ago

TafkaMax commented 6 months ago

Hi

I am testing out the ldap sync tool and when creating LDAP groups in gitlab I noticed that all dashes are removed from groups names.

E.g. I have a group named: cn=example-name Then it will be created as example name in gitlab. Of course the URL path is example-name.

Is there a way to change this behaviour? I would propose a config.yml option to specify whether to remove dashes or some special characters from the group name.

TafkaMax commented 6 months ago

I think it comes from here: https://github.com/Adambean/gitlab-ce-ldap-sync/blob/master/src/LdapSyncCommand.php#L1265

TafkaMax commented 6 months ago

I think leaving the slugifyGitlabPath should be left AS-IS, because that is more "Strict" than the actual group name, due to being a URL.

Because most of group names (CN-s) contain dashes, it made things uglier to use IMO. I would propose a way to change the separator for the slugifyGitlabName via config option?

TafkaMax commented 6 months ago

I think I will try to create a PR tomorrow.

TafkaMax commented 6 months ago

This PR would resolve my issue: https://github.com/Adambean/gitlab-ce-ldap-sync/pull/45

Adambean commented 6 months ago

Hello,

I've just got around to looking at this. To start with these Slugify instances only exist to help ensure group names conform with Gitlab limitations on project and group names.

Project or group names must start with a letter (a-zA-Z), digit (0-9), emoji, or underscore (_). Additionally Group names can contain only letters (a-zA-Z), digits (0-9), emoji, underscores (_), dots (.), parentheses (()), dashes (-), or spaces.

I wrote the RegEx for this back in November 2018 therefore it may have got out of date since then. We're on Gitlab 16.x today, we were at 11.5 when I built this tool. (I can't read the 11.5 docs now as it requires Docker, which I refuse to install and set up just to read one ruddy document. 😛)

I think perhaps the answer here would be to update the RegEx to accommodate the current restrictions rather than customise the separator. The separator is only there to be used as substitute for invalid characters. Therefore if your hyphens/dashes are getting replaced with spaces the problem would be the input RegEx in the first place. (You shouldn't need to change the separator to fix that.)

A better pattern would be /([^A-Za-z0-9_\.\(\)\- ])/, which can be tested here: https://regexr.com/7qsjs What this doesn't do is check for the stricter rules for the first character, though I'm not sure if the Slugify component can do that.

[Edit] The pattern for group slugs may also need to be updated to /([^A-Za-z0-9_\.\-])/, which can be tested here: https://regexr.com/7qsl0

Adambean commented 6 months ago

Submitted those to branch issue/43-dashes-removed-from-group-names to be tested.

TafkaMax commented 6 months ago

Wow that is an even better idea! I will test out the branch on my test gitlab aswell.

TafkaMax commented 6 months ago 
On branch issue/43-dashes-removed-from-group-names
Your branch is up to date with 'origin/issue/43-dashes-removed-from-group-names'.

nothing to commit, working tree clean

The script ran through as intended.

Adambean commented 6 months ago

Cool. :)

None of my directories have groups with hyphens/dashes in them, therefore I'll happily let you decide if you think this is resolved, or if any further work is needed to it.

TafkaMax commented 6 months ago

Cool. :)

None of my directories have groups with hyphens/dashes in them, therefore I'll happily let you decide if you think this is resolved, or if any further work is needed to it.

I have

And all of these groups were created successfully. Dashes and dots are not the first character in the group name, so I didn't test that edge case, where the first char in the group name has more limitations.

Overall I think this is resolved for now. If somebody wishes to expand on the first character regexp in the future and add that check, they can reference this issue.