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

Never allow an empty password to validate #9682

Merged
merged 1 commit into from
Jan 11, 2020

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Jan 9, 2020

#6023 caused a regression.

@zeripath zeripath added issue/regression Issue needs no code to be fixed, only a description on how to fix it yourself backport/v1.11 labels Jan 9, 2020
@zeripath zeripath added this to the 1.12.0 milestone Jan 9, 2020
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jan 9, 2020
@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 9, 2020
@silverwind
Copy link
Member

Maybe add a test?

@zeripath zeripath changed the title No empty password Never allow an empty password to validate Jan 9, 2020
@lafriks
Copy link
Member

lafriks commented Jan 10, 2020

Does not seem to work, all tests fail

@zeripath
Copy link
Contributor Author

zeripath commented Jan 11, 2020

Yeah, so the reason tests failed is because I wrote:

// IsPasswordSet checks if the password is set or left empty
func (u *User) IsPasswordSet() bool {
       return u.ValidatePassword("")
}

instead of:

// IsPasswordSet checks if the password is set or left empty
func (u *User) IsPasswordSet() bool {
       return !u.ValidatePassword("")
}

yes, I'm an eejit.

@codecov-io
Copy link

Codecov Report

Merging #9682 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #9682      +/-   ##
=========================================
- Coverage    42.2%   42.2%   -0.01%     
=========================================
  Files         591     591              
  Lines       78085   78085              
=========================================
- Hits        32959   32953       -6     
- Misses      41080   41085       +5     
- Partials     4046    4047       +1
Impacted Files Coverage Δ
models/user.go 50.77% <100%> (ø) ⬆️
models/unit.go 62.16% <0%> (-5.41%) ⬇️
modules/process/manager.go 74.69% <0%> (-3.62%) ⬇️
services/pull/temp_repo.go 31.62% <0%> (-2.57%) ⬇️
modules/queue/workerpool.go 39.91% <0%> (-1.29%) ⬇️
modules/log/event.go 64.61% <0%> (-1.03%) ⬇️
services/pull/check.go 53.37% <0%> (+4.72%) ⬆️

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 32fb813...465caef. Read the comment docs.

@lunny lunny merged commit eadb45e into go-gitea:master Jan 11, 2020
lafriks pushed a commit that referenced this pull request Jan 11, 2020
* Restore IsPasswordSet previous value

* Update models/user.go
@lafriks lafriks added the backport/done All backports for this PR have been created label Jan 11, 2020
@zeripath zeripath deleted the no-empty-password branch January 11, 2020 11:36
lafriks added a commit that referenced this pull request Jan 11, 2020
* Restore IsPasswordSet previous value

* Update models/user.go

Co-authored-by: Lauris BH <[email protected]>
@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.
Labels
backport/done All backports for this PR have been created issue/regression Issue needs no code to be fixed, only a description on how to fix it yourself lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants