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.

Sign up for GitHub

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

Jump to bottom

Add option to prevent LDAP from deactivating everything on empty search #9879

Merged
merged 4 commits into from
Jan 20, 2020
Merged

Add option to prevent LDAP from deactivating everything on empty search #9879

merged 4 commits into from
Jan 20, 2020

Conversation

zeripath
Copy link
Contributor

It appears that some LDAPs will simply return an empty search results if there is an error during searching rather than report the error. In those circumstances Gitea would completely deactivate all LDAP users.

This is clearly not ideal - add a configuration option to LDAP configuration which will mean that by default an empty search results will not deactivate any users and instead an error will be logged. An authentication option can be turned on for those situations where this behaviour would be intended.

Fix #7949

@zeripath zeripath added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jan 19, 2020
@zeripath zeripath added this to the 1.12.0 milestone Jan 19, 2020
guillep2k
guillep2k reviewed Jan 19, 2020
View reviewed changes
Copy link
Member

@guillep2k guillep2k left a comment

Choose a reason for hiding this comment

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

This should require a migration step, I think. What should the migration value be? I vote for AllowDeactivateAll=false.

options/locale/locale_en-US.ini Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 19, 2020
@zeripath
Copy link
Contributor Author

It's actually not in the db like that. It's JSON so it will default to false already.

@GiteaBot GiteaBot 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 Jan 19, 2020
@codecov-io
Copy link

codecov-io commented Jan 19, 2020 •
edited

Codecov Report

Merging #9879 into master will decrease coverage by <.01%.
The diff coverage is 5.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9879      +/-   ##
==========================================
- Coverage   42.27%   42.26%   -0.01%     
==========================================
  Files         605      605              
  Lines       79253    79259       +6     
==========================================
- Hits        33501    33500       -1     
- Misses      41622    41626       +4     
- Partials     4130     4133       +3
Impacted Files Coverage Δ
modules/repofiles/temp_repo.go 62.1% <0%> (ø) ⬆️
models/login_source.go 25.63% <7.14%> (-0.13%) ⬇️
services/pull/check.go 56.64% <0%> (-2.1%) ⬇️
models/error.go 30.76% <0%> (-0.55%) ⬇️
modules/log/event.go 65.64% <0%> (+1.02%) ⬆️
models/unit.go 39.5% <0%> (+2.46%) ⬆️

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 42fc65e...1ee28e9. Read the comment docs.

@GiteaBot GiteaBot 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 Jan 20, 2020
lunny
lunny requested changes Jan 20, 2020
View reviewed changes
modules/auth/ldap/ldap.go Show resolved Hide resolved
@lunny lunny merged commit c5014a7 into go-gitea:master Jan 20, 2020
@zeripath zeripath deleted the ldap-no-deletey-everythingy branch January 20, 2020 07:12
@dioss-Machiel
Copy link

Is it at all possible to get this fix into a 1.10 release?

@zeripath
Copy link
Contributor Author

We could probably backport it to 1.11?

I think 1.10 is now security fixes only. git cherry-pick c5014a7 might work to put it on 1.10 but it will likely conflict.

@guillep2k
Copy link
Member

We could probably backport it to 1.11?

I think 1.10 is now security fixes only. git cherry-pick c5014a7 might work to put it on 1.10 but it will likely conflict.

Sounds like we're close to 1.11, but if anybody wants port this to 1.10.4 and fix the conflicts or do it by hand, I have no problem reviewing the PR from zero.

@lunny
Copy link
Member

lunny commented Jan 20, 2020

@zeripath If we marked this as feature, I don't think we should send backport to v1.11. Or we may should label this as bug.

dioss-Machiel added a commit to dioss-Machiel/gitea that referenced this pull request Jan 20, 2020
@dioss-Machiel
Backport of go-gitea#9879 (Add option to prevent LDAP from deactivati…
3a0118b 
…ng everything on empty search)
@zeripath
Copy link
Contributor Author

@dioss-Machiel Looks like it applied cleanly to release/v1.10?

@dioss-Machiel
Copy link

I applied it manually, seems to be fine.

@dioss-Machiel dioss-Machiel mentioned this pull request Jan 20, 2020
Merged
zeripath added a commit to zeripath/gitea that referenced this pull request Jan 20, 2020
@zeripath @guillep2k @lunny
Add option to prevent LDAP from deactivating everything on empty sear…
cb45a28 
…ch (go-gitea#9879)

* Add option to prevent LDAP from deactivating everything on empty search

* Update options/locale/locale_en-US.ini

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

Co-authored-by: guillep2k <[email protected]>
Co-authored-by: Lunny Xiao <[email protected]>
@zeripath zeripath mentioned this pull request Jan 20, 2020
Merged
@zeripath
Copy link
Contributor Author

I've marked this as a bug. I'm not sure where the bug is because LDAP shouldn't be returning an empty result without an error.

lafriks pushed a commit that referenced this pull request Jan 20, 2020
@zeripath @guillep2k
@lunny @lafriks
Add option to prevent LDAP from deactivating everything on empty sear…
ff508c9 
…ch (#9879) (#9896)

* Add option to prevent LDAP from deactivating everything on empty search

* Update options/locale/locale_en-US.ini

Co-Authored-By: guillep2k <[email protected]>
Co-authored-by: Lunny Xiao <[email protected]>
techknowlogick pushed a commit that referenced this pull request Jan 20, 2020
@dioss-Machiel @sapk
@zeripath @techknowlogick
Prevent empty LDAP search from deactivating all users (#9879) (#9890)
68bca62 
* Backport of #9879 (Add option to prevent LDAP from deactivating everything on empty search)

* go fmtted

Co-authored-by: Antoine GIRARD <[email protected]>
Co-authored-by: zeripath <[email protected]>
@lcnittl lcnittl mentioned this pull request Nov 3, 2020
Open
6 tasks
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny approved these changes

@guillep2k guillep2k guillep2k approved these changes

@6543 6543 6543 approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Milestone
1.12.0
Development

Successfully merging this pull request may close these issues.

Gitea deactivates LDAP users
7 participants
@zeripath @codecov-io @dioss-Machiel @guillep2k @lunny @6543 @GiteaBot