Skip to content

Conversation

lunny
Copy link
Member

@lunny lunny commented Feb 18, 2019

Before this PR, a prohibit login account also can login and see the dashboard page. This PR will add more checks to fix that.

@codecov-io
Copy link

codecov-io commented Feb 18, 2019

Codecov Report

Merging #6106 into master will decrease coverage by 0.02%.
The diff coverage is 11.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6106      +/-   ##
==========================================
- Coverage   38.88%   38.86%   -0.03%     
==========================================
  Files         349      349              
  Lines       49755    49801      +46     
==========================================
+ Hits        19349    19355       +6     
- Misses      27611    27646      +35     
- Partials     2795     2800       +5
Impacted Files Coverage Δ
modules/context/auth.go 19.11% <0%> (-2.76%) ⬇️
routers/user/auth.go 13.11% <0%> (-0.18%) ⬇️
routers/home.go 46.29% <0%> (-0.7%) ⬇️
models/error.go 34.59% <0%> (-0.9%) ⬇️
models/login_source.go 26.31% <31.57%> (+0.01%) ⬆️
modules/process/manager.go 81.15% <0%> (+4.34%) ⬆️

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 538a26d...05d4883. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 18, 2019
@@ -642,6 +642,12 @@ func UserSignIn(username, password string) (*User, error) {
}

if hasUser {
if !user.IsActive {
Copy link
Member

Choose a reason for hiding this comment

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

This should be checked only after username&password is validated otherwise usernames can be leaked/guessed from private servers

@lafriks
Copy link
Member

lafriks commented Feb 18, 2019

Oh, and also ExternalUserLogin should need check to not allow Prohibited users to log in

@lunny
Copy link
Member Author

lunny commented Feb 18, 2019

@lafriks done.

@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 Feb 18, 2019
@@ -113,6 +113,6 @@ func TestCreateReleasePaging(t *testing.T) {
checkLatestReleaseAndCount(t, session, "/user2/repo1", "v0.0.12", i18n.Tr("en", "repo.release.draft"), 10)

// Check that user3 does not see draft and still see 10 latest releases
session2 := loginUser(t, "user3")
session2 := loginUser(t, "user4")
Copy link
Member

Choose a reason for hiding this comment

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

Please change comment above to reference user4

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

One minor thing, but otherwise looks great!

@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 Feb 19, 2019
@lunny lunny force-pushed the lunny/fix_bug_prohibit_login branch from 925c303 to 9927680 Compare February 19, 2019 07:00
@lafriks lafriks merged commit f5fa22a into go-gitea:master Feb 19, 2019
@lafriks lafriks changed the title fix bug prohibit login not applied on dashboard Fix prohibit login check on authorization Feb 19, 2019
@lafriks
Copy link
Member

lafriks commented Feb 19, 2019

@lunny please backport

@lunny lunny deleted the lunny/fix_bug_prohibit_login branch February 19, 2019 08:17
lunny added a commit to lunny/gitea that referenced this pull request Feb 19, 2019
* fix bug prohibit login not applied on dashboard

* fix tests

* fix bug user status leak

* fix typo

* return after render
@lunny lunny added the backport/done All backports for this PR have been created label Feb 19, 2019
lafriks pushed a commit that referenced this pull request Feb 19, 2019
* Fix prohibit login check on authorization (#6106)

* fix bug prohibit login not applied on dashboard

* fix tests

* fix bug user status leak

* fix typo

* return after render

* remove unused tests
@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 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants