Skip to content

Added support for listing LDAP group links #1015

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

Merged

Conversation

profhenry
Copy link
Contributor

@profhenry profhenry commented Aug 4, 2023

Fixes #1014

See https://docs.gitlab.com/ee/api/groups.html#list-ldap-group-links


About the LdapGroupLink model:

The GitLab REST API documentation (https://docs.gitlab.com/ee/api/groups.html#list-ldap-group-links)
does not specify the attributes which are returned for a LDAP group link :-/.
This are the attributes returned by our GitLab instance (15.10.2-ee).
These seem fine because they match with the attributes which you can pass when adding a LDAP group link :-).

@profhenry profhenry force-pushed the 1014_add-support-for-listing-ldap-group-links branch from 6097797 to 542913f Compare August 4, 2023 15:43
Copy link
Collaborator

@jmini jmini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have an example JSON for LdapGroupLink.java tested like the other model class.

Otherwise the change looks good to me.

Copy link
Collaborator

@jmini jmini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

I hope the ldap-group-link.json I wrote is realistic enough (but I do not have a GitLab with LDAP to verify this)

@profhenry
Copy link
Contributor Author

profhenry commented Aug 16, 2023

I think your ldap-group-link.json looks good.
Here is a group link returned by our gitlab (15.10.2-ee):

{
	"cn": "prj-rp-master-admins",
	"group_access": 50,
	"provider": "ldapmain",
	"filter": null
}

I think that one of cn or filter is always null (depending on how the group was added), but i think it is fine for a unit test.

@jmini jmini merged commit a3ab79b into gitlab4j:main Aug 16, 2023
@jmini
Copy link
Collaborator

jmini commented Aug 16, 2023

merged!

@profhenry Thank you very much for your contribution and for the feedback here.

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.

Add missing support for listing LDAP group links
2 participants