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

Add LDAP group sync to Teams, fixes #1395 #16299

Merged
merged 33 commits into from
Feb 11, 2022

Conversation

svenseeberg
Copy link
Contributor

@svenseeberg svenseeberg commented Jun 29, 2021

  • Add setting for a JSON that maps LDAP groups to Org Teams. Format:
{"cn=MyGroup,cn=groups,dc=example,dc=org": {"MyGiteaOrganization": ["MyGiteaTeam1", "MyGiteaTeam2", ...], ...}, ...}
  • Sync is being run on login and periodically.
  • Existing group filter settings are reused.
  • Removal of users from Teams can optionally be enabled.
  • Teams that are not listed in the mapping JSON are not touched.

fixes #1395

@6543
Copy link
Member

6543 commented Jun 29, 2021

@svenseeberg can you resolve conflicts :)

PS: since its a pull from an org we maintainer cant apply code suggestions or resolve conflicts. if you need help just tell us.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 29, 2021
@6543 6543 added this to the 1.16.0 milestone Jun 29, 2021
@6543 6543 added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jun 29, 2021
@svenseeberg
Copy link
Contributor Author

@svenseeberg can you resolve conflicts :)

PS: since its a pull from an org we maintainer cant apply code suggestions or resolve conflicts. if you need help just tell us.

Right, should not be an issue. We will take care of rebasing on the current main branch.

@svenseeberg
Copy link
Contributor Author

I'll also look into the linting errors.

@6543
Copy link
Member

6543 commented Jun 29, 2021

make fmt

@6543
Copy link
Member

6543 commented Jun 29, 2021

you dont have to hurry we are currently in feature-freeze ... :)

& we need some tests for this code

@svenseeberg
Copy link
Contributor Author

we need some tests for this code

Are there any specific requirements for the tests? Mocking an LDAP server is somewhat complicated ;-) However, we could easily test the functions that do not interact with the LDAP server.

@6543
Copy link
Member

6543 commented Jun 29, 2021

well we have unit tests who test selve contained functions or easy to moke on ...
example: https://github.com/go-gitea/gitea/blob/main/modules/auth/openid/discovery_cache_test.go

and we have integration tests:
example: https://github.com/go-gitea/gitea/blob/main/integrations/auth_ldap_test.go

and we have a running ldap to test against (https://drone.gitea.io/go-gitea/gitea/41501/2/5)

@svenseeberg
Copy link
Contributor Author

I guess we can work with that :-)

@zeripath
Copy link
Contributor

zeripath commented Jul 2, 2021

We have some LDAP tests already which use a docker container for the LDAP.

@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2021

Codecov Report

Merging #16299 (a75516d) into main (212e81f) will increase coverage by 0.39%.
The diff coverage is 70.12%.

❗ Current head a75516d differs from pull request most recent head 0d402cc. Consider uploading reports for the commit 0d402cc to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16299      +/-   ##
==========================================
+ Coverage   45.74%   46.14%   +0.39%     
==========================================
  Files         831      839       +8     
  Lines       92178    92563     +385     
==========================================
+ Hits        42171    42712     +541     
+ Misses      43249    43066     -183     
- Partials     6758     6785      +27     
Impacted Files Coverage Δ
build/codeformat/formatimports.go 66.35% <0.00%> (ø)
cmd/admin.go 0.00% <0.00%> (ø)
cmd/admin_auth_ldap.go 75.49% <0.00%> (-2.59%) ⬇️
cmd/cert.go 0.00% <0.00%> (ø)
cmd/doctor.go 0.00% <0.00%> (ø)
cmd/dump.go 0.89% <0.00%> (ø)
cmd/dump_repo.go 0.00% <0.00%> (ø)
cmd/manager.go 0.00% <ø> (ø)
cmd/serv.go 2.46% <0.00%> (ø)
cmd/web.go 0.00% <0.00%> (ø)
... and 321 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f910924...0d402cc. Read the comment docs.

@svenseeberg svenseeberg force-pushed the feature/ldap-group-sync branch 2 times, most recently from 4b5e3d5 to 1e9b4dd Compare July 9, 2021 17:11
* Add setting for a JSON that maps LDAP groups
  to Org Teams.
* Add log trace when removing or adding team members.
* Sync is being run on login and periodically.
* Existing group filter settings are reused.

Co-authored-by: Giuliano Mele <mele@integreat-app.de>
Co-authored-by: Sven Seeberg <mail@sven-seeberg.de>
@svenseeberg
Copy link
Contributor Author

@6543 we updated our pull request an included tests. Can you please review?

* Adding and removing team members.
* Sync not existing LDAP group.
* Login with broken group map JSON.

Co-authored-by: Giuliano Mele <mele@integreat-app.de>
Co-authored-by: Sven Seeberg <mail@sven-seeberg.de>
modules/auth/ldap/ldap.go Outdated Show resolved Hide resolved
@melegiul
Copy link

melegiul commented Aug 26, 2021

Thank you for your review @6543
I'm sorry for the long response time, but now I have removed the funk package in 3a032cc
Unfortunately I could not find existing functions to use instead, so I added some to the utils module for these three use cases:

  1. get all keys from a map
  2. get the intersection of two slices
  3. get the difference of two slices

Thanks in advance for your feedback

Co-authored-by: Sven Seeberg <mail@sven-seeberg.de>
Co-authored-by: Giuliano Mele <mele@integreat-app.de>
@6543
Copy link
Member

6543 commented Aug 29, 2021

@svenseeberg can you resolve conflicts :)

(just do a merge of main and resolve - rebase or squash not required since pull's are squash-merged anyway)

@svenseeberg
Copy link
Contributor Author

@svenseeberg can you cherry-pick netzbegruenung@6ef197e ? I thinkt that's the cleanset solution ... & merge upstream in first

Oh sorry, I read this comment after I merged the PR.

I added @wxiaoguang and @6543 as collaborators to our repo.

@wxiaoguang
Copy link
Contributor

Since the main branch has changed a lot (including the lint rules), there are some new work to do. Give me some more time.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Feb 10, 2022

Hmm... CI fails, maybe I did something wrong during the refactoring. @svenseeberg could you help to take a look?

Fixing ...

@6543
Copy link
Member

6543 commented Feb 10, 2022

@wxiaoguang yes you did not adjust the tests appropriate

@6543
Copy link
Member

6543 commented Feb 10, 2022

ok two things need to be done:

  1. test if this pull works in current state for you @svenseeberg @gtudan
  2. update inteagration tests

@svenseeberg thanks - that will help :)

@gtudan
Copy link

gtudan commented Feb 10, 2022

Just tried the latest build and I can confirm that it works

Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

tests PASS localy

@wxiaoguang
Copy link
Contributor

🚀

@wxiaoguang wxiaoguang merged commit 832ce40 into go-gitea:main Feb 11, 2022
@svenseeberg
Copy link
Contributor Author

Thank you all for your work on this!

zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 12, 2022
* giteaofficial/main:
  Send mail to issue/pr assignee/reviewer also when OnMention is set (go-gitea#18707)
  Reduce CI go module downloads, add make targets (go-gitea#18708)
  Add number in queue status to monitor page (go-gitea#18712)
  Fix source code line highlighting (go-gitea#18729)
  Fix forked repositories missed tags (go-gitea#18719)
  [skip ci] Updated translations via Crowdin
  Fix release typo (go-gitea#18728)
  Display template path of current page in dev mode (go-gitea#18717)
  Separate the details links of commit-statuses in headers (go-gitea#18661)
  Add LDAP group sync to Teams, fixes go-gitea#1395 (go-gitea#16299)
  Change git.cmd to RunWithContext (go-gitea#18693)
@scs-kno
Copy link

scs-kno commented Feb 17, 2022

Is there a way to debug this feature? It doesn't create any Organizations for me with the latest Docker image.

@melegiul
Copy link

melegiul commented Feb 17, 2022

Is there a way to debug this feature? It doesn't create any Organizations for me with the latest Docker image.

Actually this feature is not included, orgs and teams have to be created manually
But should be not much effort to automatically generate them

@scs-kno
Copy link

scs-kno commented Feb 17, 2022

Has anybody written a short manual/tutorial on how to use this feature?

@melegiul
Copy link

Has anybody written a short manual/tutorial on how to use this feature?

This file contains an example for the Team Group Map field with the expected JSON format
https://github.com/go-gitea/gitea/blob/main/services/auth/source/ldap/README.md
Please note that you have to properly set the fields "Group Attribute Containing List Of Users" and "User Attribute Listed In Group" to detect the users groups
image

@svenseeberg
Copy link
Contributor Author

To elaborate a little on the previous answer: You need to have created the organizations and teams before you can sync LDAP users into them. To create the config and JSON you need to make sure that you have the following information:

  • the org and team names
  • the DN (distinguished name) of your group, for example cn=mygroup,dc=groups,dc=example,dc=com
  • the attribute that contains the membership information within your LDAP group object (usually something like member)
  • You need to know which user attribute is contained in the group membership attribute (for example the "username" cn or the full dn)

@6543 6543 deleted the feature/ldap-group-sync branch February 20, 2022 11:53
@janosmiko
Copy link

Hi @svenseeberg !

First of all, thank you for your awesome work on this! 👏

Is it possible somehow to dynamically map LDAP nested groups as organizations and teams?

Eg:

LDAP:                   | Gitea group:
- group1                | Organization 1
  - subgroup1           | Team 1
    - member 1          | Gitea user 1

And later if I create a new main group, and a subgroup (in both LDAP and Gitea) and a new member in LDAP attach that user to the team in Gitea?

@svenseeberg
Copy link
Contributor Author

I don't think that this can currently be done. The options for configuring the LDAP queries are very limited.

@janosmiko
Copy link

janosmiko commented Mar 8, 2022

Hi @svenseeberg !

First of all, thank you for your awesome work on this! 👏

Is it possible somehow to dynamically map LDAP nested groups as organizations and teams?

Eg:

LDAP:                   | Gitea group:
- group1                | Organization 1
  - subgroup1           | Team 1
    - member 1          | Gitea user 1

And later if I create a new main group, and a subgroup (in both LDAP and Gitea) and a new member in LDAP attach that user to the team in Gitea?

If anyone's interested, I implemented an external solution to do this:
https://github.com/janosmiko/gitea-ldap-sync

Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
* Add setting for a JSON that maps LDAP groups to Org Teams.
* Add log when removing or adding team members.
* Sync is being run on login and periodically.
* Existing group filter settings are reused.
* Adding and removing team members.
* Sync not existing LDAP group.
* Login with broken group map JSON.
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for LDAP and AD Group sync