LDAP BindDN Multiple Log In Options Results in user already exists error

[iamdoubz]

• Gitea version (or commit ref): 1.11.0+rc1-41-gce756ee89 and 1.11.0+rc1-42-gf96c1a2c7
• Git version: 2.25.0.windows.1
• Operating system: Windows Server 2012 R2
• Database (use [x]):
  • PostgreSQL
  • MySQL (MariaDB Server version: 10.1.34-MariaDB)
  • MSSQL
  • SQLite
• Can you reproduce the bug at https://try.gitea.io:
  • Yes (provide example URL)
  • No (Can't really test out my AD environment in a public instance)
  • Not relevant
• Log gist (trace):

2020/01/20 10:00:11 ...es/auth/ldap/ldap.go:216:SearchEntry() [T] LDAP will use BindDN.
2020/01/20 10:00:11 ...es/auth/ldap/ldap.go:226:SearchEntry() [T] Bound as BindDN CN=Gitea,OU=Users,DC=my,DC=domain
2020/01/20 10:00:11 ...es/auth/ldap/ldap.go:85:findUserDN() [T] Search for LDAP user: 334455
2020/01/20 10:00:11 ...es/auth/ldap/ldap.go:93:findUserDN() [T] Searching for DN using filter (&(objectClass=Person)(|(sAMAccountName=334455)(mail=334455)(employeeNumber=334455))) and base OU=Users,DC=my,DC=domain
2020/01/20 10:00:11 ...es/auth/ldap/ldap.go:144:bindUser() [T] Binding with userDN: CN=First M. Lastname,OU=Users,DC=my,DC=domain
2020/01/20 10:00:11 ...es/auth/ldap/ldap.go:150:bindUser() [T] Bound successfully with userDN: CN=First M. Lastname,OU=Users,DC=my,DC=domain
2020/01/20 10:00:11 ...es/auth/ldap/ldap.go:257:SearchEntry() [T] Fetching attributes 'sAMAccountName', 'givenName', 'sn', 'mail', '' with filter (&(objectClass=Person)(|(sAMAccountName=334455)(mail=334455)(employeeNumber=334455))) and base CN=First M. Lastname,OU=Users,DC=my,DC=domain
2020/01/20 10:00:11 ...es/auth/ldap/ldap.go:156:checkAdmin() [T] Checking admin with filter (memberOf=CN=GiteaAdmins,OU=Users,DC=my,DC=domain) and base CN=First M. Lastname,OU=Users,DC=my,DC=domain
2020/01/20 10:00:11 .../xorm/session_raw.go:78:queryRows() [I] [SQL] SELECT `id`, `lower_name`, `name`, `full_name`, `email`, `keep_email_private`, `email_notifications_preference`, `passwd`, `passwd_hash_algo`, `must_change_password`, `login_type`, `login_source`, `login_name`, `type`, `location`, `website`, `rands`, `salt`, `language`, `description`, `created_unix`, `updated_unix`, `last_login_unix`, `last_repo_visibility`, `max_repo_creation`, `is_active`, `is_admin`, `allow_git_hook`, `allow_import_local`, `allow_create_organization`, `prohibit_login`, `avatar`, `avatar_email`, `use_custom_avatar`, `num_followers`, `num_following`, `num_stars`, `num_repos`, `num_teams`, `num_members`, `visibility`, `repo_admin_change_team_access`, `diff_view_style`, `theme` FROM `user` WHERE (id!=?) AND `lower_name`=? LIMIT 1 []interface {}{0, "fmlastname"} - took: 1.1474ms
2020/01/20 10:00:11 .../xorm/session_raw.go:78:queryRows() [I] [SQL] SELECT `id`, `lower_name`, `name`, `full_name`, `email`, `keep_email_private`, `email_notifications_preference`, `passwd`, `passwd_hash_algo`, `must_change_password`, `login_type`, `login_source`, `login_name`, `type`, `location`, `website`, `rands`, `salt`, `language`, `description`, `created_unix`, `updated_unix`, `last_login_unix`, `last_repo_visibility`, `max_repo_creation`, `is_active`, `is_admin`, `allow_git_hook`, `allow_import_local`, `allow_create_organization`, `prohibit_login`, `avatar`, `avatar_email`, `use_custom_avatar`, `num_followers`, `num_following`, `num_stars`, `num_repos`, `num_teams`, `num_members`, `visibility`, `repo_admin_change_team_access`, `diff_view_style`, `theme` FROM `user` WHERE `lower_name`=? LIMIT 1 []interface {}{"fmlastname"} - took: 999.2µs
2020/01/20 10:00:11 .../xorm/session_raw.go:85:queryRows() [I] [SQL] SELECT `id`, `lower_name`, `name`, `full_name`, `email`, `keep_email_private`, `email_notifications_preference`, `passwd`, `passwd_hash_algo`, `must_change_password`, `login_type`, `login_source`, `login_name`, `type`, `location`, `website`, `rands`, `salt`, `language`, `description`, `created_unix`, `updated_unix`, `last_login_unix`, `last_repo_visibility`, `max_repo_creation`, `is_active`, `is_admin`, `allow_git_hook`, `allow_import_local`, `allow_create_organization`, `prohibit_login`, `avatar`, `avatar_email`, `use_custom_avatar`, `num_followers`, `num_following`, `num_stars`, `num_repos`, `num_teams`, `num_members`, `visibility`, `repo_admin_change_team_access`, `diff_view_style`, `theme` FROM `user` WHERE (id!=?) AND `lower_name`=? LIMIT 1 []interface {}{0, "fmlastname"} - took: 1.0001ms
2020/01/20 10:00:11 ...dels/login_source.go:805:UserSignIn() [W] Failed to login '334455' via 'LDAP-Bind': user already exists [name: fmlastname]
2020/01/20 10:00:11 ...s/context/context.go:139:HTML() [D] Template: user/auth/signin
2020/01/20 10:00:11 routers/user/auth.go:171:SignInPost() [I] Failed authentication attempt for 334455 from 192.168.10.17:3418
2020/01/20 10:00:12 .../xorm/session_raw.go:78:queryRows() [I] [SQL] SELECT `id`, `type`, `name`, `is_actived`, `is_sync_enabled`, `cfg`, `created_unix`, `updated_unix` FROM `login_source` WHERE (is_actived = ? and type = ?) []interface {}{true, 7} - took: 15.0985ms
2020/01/20 10:00:12 ...s/context/context.go:330:func1() [D] Session ID: bf99b215ba3a0c69
2020/01/20 10:00:12 ...s/context/context.go:331:func1() [D] CSRF Token: ZXQVKwqrWZaiGXr1oKI3LlfQ6Rk6MTU3OTUzNTU1MTU0MTQ4MTcwMA
2020/01/20 10:00:12 ...s/context/context.go:139:HTML() [D] Template: pwa/manifest_json
2020/01/20 10:00:13 .../xorm/session_raw.go:78:queryRows() [I] [SQL] SELECT `id`, `type`, `name`, `is_actived`, `is_sync_enabled`, `cfg`, `created_unix`, `updated_unix` FROM `login_source` WHERE (is_actived = ? and type = ?) []interface {}{true, 7} - took: 1.1432ms
2020/01/20 10:00:13 ...s/context/context.go:330:func1() [D] Session ID: bf99b215ba3a0c69
2020/01/20 10:00:13 ...s/context/context.go:331:func1() [D] CSRF Token: ZXQVKwqrWZaiGXr1oKI3LlfQ6Rk6MTU3OTUzNTU1MTU0MTQ4MTcwMA
2020/01/20 10:00:13 ...s/context/context.go:139:HTML() [D] Template: pwa/serviceworker_js

Description

When logging in with LDAP with multiple log in options that are not the typical "username" or "email" uid's, you must check if the user exists and then authenticate using what is defined in "Username Attribute".

What should occur

When logging in with a non-standard option like employeeNumber, it should authenticate to LDAP, if passes then check if user already exists, if not, create user, if already exists and password is good, log the user in.

What is happening

When logging in with a non-standard option like employeeNumber, it will try to create a new user without checking to make sure that user does not already exist which results in an invalid credential on log in page.

Relevant lines of code:

models/login_source.go Lines 527 - 533.

	err := CreateUser(user)

	if err == nil && isAttributeSSHPublicKeySet && addLdapSSHPublicKeys(user, source, sr.SSHPublicKey) {
		err = RewriteAllPublicKeys()
	}

	return user, 

Screenshots

My environment

1. Compiled from source and built using git clone https://github.com/go-gitea/gitea.git&&git checkout release/v1.11&&TAGS="bindata sqlite sqlite_unlock_notify" make build
2. Go v1.13.6
3. Make v4.2
4. Gitea v1.11.0+rc1-41-gce756ee89
5. IIS reverse proxy to localhost on custom port

Other

Semi-relevant ticket that fixed the 500 error associated with same scenario.

[zeripath]

I don't understand this definitely checks if the user is in existence

[zeripath]

ah I do.

[zeripath]

Sorry I missed fixing this... I presumed that the ErrUserExist would be handled correctly.

That's what I get for presuming.

[iamdoubz]

Sorry I missed fixing this... I presumed that the ErrUserExist would be handled correctly.

That's what I get for presuming.

Don't fret. You did fix the initial ticket (500 error no longer happens) so it was technically correct and you did fix the problem... Now referencing the age old adage of, "Assume makes an a** out of u and me" 🤦‍♂ I did know that this would happen as that is the error I got in the logs with Gogs.

[zeripath]

So the issue is that the signature for ExternalUserLogin is wrong.

It is:

gitea/models/login_source.go

Line 694 in 27c6b8f

func ExternalUserLogin(user *User, login, password string, source *LoginSource, autoRegister bool) (*User, error) {

It's only ever called in two ways:

gitea/models/login_source.go

Line 786 in 27c6b8f

return ExternalUserLogin(user, user.LoginName, password, &source, false)

and

gitea/models/login_source.go

Line 800 in 27c6b8f

authUser, err := ExternalUserLogin(nil, username, password, source, true)

In the first case user cannot be nil so detecting whether we need to do autoRegister doesn't need to be a separate argument.

The same broken signature is passed to LoginViaLDAP too.

[iamdoubz]

How do I git pull this patch directly to test it?

[iamdoubz]

Nevermind:

curl -L https://github.com/go-gitea/gitea/pull/9900.patch > 9900.patch
git apply --check 9900.patch
git apply 9900.patch

PS IT WORKS!!!