LDAP user synchronization #1478

lafriks · 2017-04-10T01:07:21Z

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 · 2017-04-10T01:16:47Z

models/user.go

@@ -1322,3 +1324,124 @@ func GetWatchedRepos(userID int64, private bool) ([]*Repository, error) {
 	}
 	return repos, nil
 }
+
+func SyncExternalUsers() {


lafriks · 2017-04-10T05:33:56Z

Requested comment added

strk · 2017-04-10T07:41:55Z

conf/app.ini

@@ -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


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

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

Updated comment that only LDAP user sync is supported

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

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

strk · 2017-04-10T07:42:24Z

conf/app.ini

+; Synchronize external user data
+[cron.sync_external_users]
+SCHEDULE = @every 24h
+UPDATE_EXISTING = true


Could you document the default with a comment ?

Default values are the ones that are here

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.

strk · 2017-04-10T07:42:29Z

conf/app.ini

@@ -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


Could you document the default with a comment ?

Default values are the ones that are here

strk · 2017-04-10T07:43:56Z

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 · 2017-04-10T09:19:04Z

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 · 2017-04-10T09:30:05Z

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 · 2017-04-10T10:24:55Z

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 · 2017-04-10T10:31:42Z

Ah, now I get it (maybe). Is this whole PR about automatically creating local records for all users known by the LDAP database ?

lafriks · 2017-04-10T11:00:31Z

Yes, all that match user filter

lunny · 2017-04-10T11:43:58Z

I have no test environment, but looks good.

strk · 2017-04-10T16:10:27Z

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 · 2017-04-10T16:51:44Z

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 · 2017-04-10T19:55:26Z

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 · 2017-04-10T20:17:46Z

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 · 2017-04-10T20:21:57Z

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

tboerger · 2017-04-10T20:23:16Z

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

strk · 2017-04-15T07:39:37Z

models/user.go

+		repo := bean.(*Repository)
+		RemoveAllWithNotice("Delete repository wiki local copy", repo.LocalWikiPath())
+		return nil
+	}); err != nil {


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

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 · 2017-04-15T08:49:21Z

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

lunny · 2017-04-20T02:17:04Z

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

lafriks · 2017-04-30T21:25:45Z

OK, rebased correctly

lafriks · 2017-04-30T21:27:51Z

Why this pull request was moved to 1.3.0?

tboerger · 2017-05-05T07:26:49Z

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 · 2017-05-05T08:48:12Z

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 · 2017-05-05T12:56:19Z

@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 · 2017-05-07T14:08:03Z

LGTM

lunny · 2017-05-08T03:48:09Z

let L-G-T-M work

tboerger · 2017-05-10T06:44:13Z

LGTM

…min acces is drpoed if changed

* 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 <JustKiddingCode@users.noreply.github.com> * Better ident * Apply suggestions from code review Update user information Co-Authored-By: 6543 <24977596+6543@users.noreply.github.com> * 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 <lauris@nix.lv> * if adminFilter is set - use it Co-Authored-By: Lauris BH <lauris@nix.lv> * Update models/login_source.go well - I should look more detaild at suggestions :D Co-Authored-By: Lauris BH <lauris@nix.lv> * make it work again * set is_admin value to user * look nicer

lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Apr 10, 2017

lunny requested changes Apr 10, 2017

View reviewed changes

lunny added this to the 1.2.0 milestone Apr 10, 2017

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 10, 2017

strk reviewed Apr 10, 2017

View reviewed changes

lunny approved these changes Apr 10, 2017

View reviewed changes

strk reviewed Apr 15, 2017

View reviewed changes

lunny modified the milestones: 1.3.0, 1.2.0 Apr 30, 2017

lafriks force-pushed the master branch from 65dfd73 to 2259bdd Compare April 30, 2017 21:23

lafriks added 9 commits May 5, 2017 13:50

Refactor LDAP SearchEntry return type

0defcb4

Implememt method to search all LDAP users that match source user filter

9ef5a6f

Optimize dublicate code

38a0323

Add job to syncronise users with LDAP

9b7043a

Add comment to SyncExternalUsers function

9b330de

Specify in app.ini that only LDAP user sync is supported

46f62c9

Add comments to app.ini about external source synchronization job

2138b3e

Add identation back autochanged by goreturns

dc137fb

Add check to not change admin flag if no LDAP admin filter is set

d496de1

lafriks force-pushed the master branch from 70f964b to d496de1 Compare May 5, 2017 10:50

* Add login source option to enable user synchronization

5cfbd75

* Add administrator action to sync external users

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 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 merged commit 524885d into go-gitea:master May 10, 2017

DblK mentioned this pull request May 17, 2017

Update User information in Gitea based on LDAP when login #1743

Closed

This was referenced May 31, 2017

LDAP Public SSH Keys synchronization #1844

Merged

Delete Public SSH Key tmp file after calculating fingerprint #1855

Merged

Setting to disable authorized_keys backup #1856

Merged

6543 added a commit to 6543-forks/gitea that referenced this pull request Nov 7, 2019

User sync is implemented at go-gitea#1478 - so only make sure that ad…

6c703ff

…min acces is drpoed if changed

6543 mentioned this pull request Nov 7, 2019

Drop Admin attribute based on LDAP when login (continue #1743) #8849

Merged

go-gitea locked and limited conversation to collaborators Nov 23, 2020

LDAP user synchronization #1478

LDAP user synchronization #1478

Conversation

lafriks commented Apr 10, 2017

Choose a reason for hiding this comment

lafriks commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strk commented Apr 10, 2017

lafriks commented Apr 10, 2017

strk commented Apr 10, 2017

lafriks commented Apr 10, 2017

strk commented Apr 10, 2017 via email

lafriks commented Apr 10, 2017 • edited Loading

lunny commented Apr 10, 2017

strk commented Apr 10, 2017

lafriks commented Apr 10, 2017

strk commented Apr 10, 2017

lafriks commented Apr 10, 2017

lafriks commented Apr 10, 2017

tboerger commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks commented Apr 15, 2017

lunny commented Apr 20, 2017

lafriks commented Apr 30, 2017

lafriks commented Apr 30, 2017

tboerger commented May 5, 2017

lunny commented May 5, 2017

lafriks commented May 5, 2017

lunny commented May 7, 2017

lunny commented May 8, 2017

tboerger commented May 10, 2017

lafriks commented Apr 10, 2017 •

edited

Loading