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

LDAP user synchronization #1478

Merged
merged 10 commits into from
May 10, 2017
Merged

LDAP user synchronization #1478

merged 10 commits into from
May 10, 2017

Conversation

lafriks
Copy link
Member

@lafriks lafriks commented Apr 10, 2017

Ported from my Gogs pull request that I have been using in production for about a year but as it is still not accepted in Gogs I am posting it here in hope to sooner migrate to Gitea

@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Apr 10, 2017
models/user.go Outdated
@@ -1322,3 +1324,124 @@ func GetWatchedRepos(userID int64, private bool) ([]*Repository, error) {
}
return repos, nil
}

func SyncExternalUsers() {
Copy link
Member

Choose a reason for hiding this comment

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

comment

@lunny lunny added this to the 1.2.0 milestone Apr 10, 2017
@lafriks
Copy link
Member Author

lafriks commented Apr 10, 2017

Requested comment added

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 10, 2017
conf/app.ini Outdated
@@ -439,6 +439,11 @@ SCHEDULE = @every 24h
; Archives created more than OLDER_THAN ago are subject to deletion
OLDER_THAN = 24h

; Synchronize external user data
Copy link
Member

Choose a reason for hiding this comment

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

What user information is synced ? Could this comment be expanded to tell more about the feature ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Same user information as when authorizing using external authorization and creating new user (LDAP)

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated comment that only LDAP user sync is supported

Copy link
Member

Choose a reason for hiding this comment

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

"Same information" being ? Email and FullName ? Anything else ?
Could it be written in the comment ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Same information just like when authorizing that is already in existing code, I'm just adding user information updating/new user creation on regular interval, so I don't think this would be appropriate place to specify such information in configuration file

conf/app.ini Outdated
; Synchronize external user data
[cron.sync_external_users]
SCHEDULE = @every 24h
UPDATE_EXISTING = true
Copy link
Member

Choose a reason for hiding this comment

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

Could you document the default with a comment ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Default values are the ones that are here

Copy link
Member

Choose a reason for hiding this comment

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

I actually recently found out that's not the case, there's an internal default in setting.go for all the settings, so that conf/app.ini really only has the role of a template to be copied to the custom/conf/ directory, for customizing it. When customizing values (changing them) it's still useful to have comments telling what the default is.

conf/app.ini Outdated
@@ -439,6 +439,11 @@ SCHEDULE = @every 24h
; Archives created more than OLDER_THAN ago are subject to deletion
OLDER_THAN = 24h

; Synchronize external user data
[cron.sync_external_users]
SCHEDULE = @every 24h
Copy link
Member

Choose a reason for hiding this comment

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

Could you document the default with a comment ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Default values are the ones that are here

@strk
Copy link
Member

strk commented Apr 10, 2017

Are both SCHEDULE and UPDATE_EXISTING needed or could SCHEDULE alone express both variables (with a value like @never or similar to express disabling the feature) ?

@lafriks
Copy link
Member Author

lafriks commented Apr 10, 2017

Yes, both settings (SCHEDULE and UPDATE_EXISTING) are needed - When setting UPDATE_EXISTING is set to false only new users are created, existing user data is not updated

@strk
Copy link
Member

strk commented Apr 10, 2017

Are you saying that even with UPDATE_EXISTING disabled there will be a periodic upgrade of new users ? Or if you're just talking about user creation event, how does it have to do with a SCHEDULE ?

@lafriks
Copy link
Member Author

lafriks commented Apr 10, 2017

Yes, UPDATED_EXISTING false will only create new users that are in LDAP but are not in Gitea, it will not update user info that are already created/imported in Gitea. SCHEDULE is interval how often it will either create new users (when UPDATE_EXISTING is false) or update/create new ones/disable deleted ones (when UPDATE_EXISTING is true)

@strk
Copy link
Member

strk commented Apr 10, 2017 via email

@lafriks
Copy link
Member Author

lafriks commented Apr 10, 2017

Yes, all that match user filter

@lunny
Copy link
Member

lunny commented Apr 10, 2017

I have no test environment, but looks good.

@strk
Copy link
Member

strk commented Apr 10, 2017

It does sound useful, although possibly network intensive, to have all users known at once, and automatically created. I also have a LDAP backed instance and often find myself asking people to "login once" before I can grant them access to projects. It might be fun to test importing them all automatically (speaking of thousand...)

@lafriks
Copy link
Member Author

lafriks commented Apr 10, 2017

Had no problem with 200 it was matter of seconds. Large user count 1k+ sync can be optimised with ldap paging but let's leave that for other PR ;)

@strk
Copy link
Member

strk commented Apr 10, 2017

I just checked and it's actually 20K users, not 2K, so 100 times as much as those you tried. Would that mean a matter of minutes per sync ?

@lafriks
Copy link
Member Author

lafriks commented Apr 10, 2017

Can't say for sure but it would be interesting to see results :) I think bottleneck for this would be inserts to database not searching ldap. Also depending on ldap server configuration you would most probably hit max search result limit. For active directory server it is 1000 by default, it will not return more than that and only way around it is ldap paging...

@lafriks
Copy link
Member Author

lafriks commented Apr 10, 2017

Anyway when this gets accepted I could try add paging support for search results in other PR

@tboerger
Copy link
Member

I need to test that next week 8) that's a feature I'm missing

models/user.go Outdated
repo := bean.(*Repository)
RemoveAllWithNotice("Delete repository wiki local copy", repo.LocalWikiPath())
return nil
}); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

If I read correctly this is an indent-only change, and is also introducing a wrong indent (does "make lint" not complain about it?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I had goreturns formatter set in editor and this change was not intentional. make lint was not complaining about it. Anyway changed it back to what it was before

@lafriks
Copy link
Member Author

lafriks commented Apr 15, 2017

Ooops... did rebase from gitea upstream master and now this pull request looks strange... :) Should I create new one?

@lunny
Copy link
Member

lunny commented Apr 20, 2017

@lafriks rebase and push force to your branch. Create a new one is also OK.

@lafriks
Copy link
Member Author

lafriks commented Apr 30, 2017

OK, rebased correctly

@lafriks
Copy link
Member Author

lafriks commented Apr 30, 2017

Why this pull request was moved to 1.3.0?

@tboerger
Copy link
Member

tboerger commented May 5, 2017

I can add sync now button later in other PR

Yes please

I can add single user sync on login in different PR later on when this is merged in

That also makes sense.

@lunny
Copy link
Member

lunny commented May 5, 2017

I have tested this successfully. But I think we need a checkbox on ldap configuration to allow disable this.

* Add administrator action to sync external users
@lafriks
Copy link
Member Author

lafriks commented May 5, 2017

@lunny I have added option to enable/disable user synchronization for each login source

@tboerger I have added action in admin dashboard to start user synchronization manually

@lunny
Copy link
Member

lunny commented May 7, 2017

LGTM

@lunny
Copy link
Member

lunny commented May 8, 2017

let L-G-T-M work

@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 8, 2017
@tboerger
Copy link
Member

LGTM

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 10, 2017
@bkcsoft bkcsoft merged commit 524885d into go-gitea:master May 10, 2017
6543 added a commit to 6543-forks/gitea that referenced this pull request Nov 7, 2019
zeripath pushed a commit that referenced this pull request Nov 14, 2019
* Update User information in Gitea based on LDAP when login

* Update Admin Flag only if exist in settings

* Fix affectation

* Update models/login_source.go

Co-Authored-By: JustKiddingCode <[email protected]>

* Better ident

* Apply suggestions from code review

Update user information

Co-Authored-By: 6543 <[email protected]>

* Make fmt

* add err handling

* if user exist but login is Prohibit return return nil, and Prohibit err

* keep login speed

* User sync is implemented at #1478 - so only make sure that admin acces is drpoed if changed

* handle error and still use async task

* no async

* only update admin if Sync is enabled

* update two comments

* add lafriks suggestions

Co-Authored-By: Lauris BH <[email protected]>

* if adminFilter is set - use it

Co-Authored-By: Lauris BH <[email protected]>

* Update models/login_source.go

well - I should look more detaild at suggestions :D

Co-Authored-By: Lauris BH <[email protected]>

* make it work again

* set is_admin value to user

* look nicer
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
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.

7 participants