From f5fa22a49997cb423255e98da5abd67c2115f2c0 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 19 Feb 2019 15:19:28 +0800 Subject: [PATCH] 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 --- integrations/release_test.go | 4 ++-- integrations/repo_test.go | 2 +- models/error.go | 32 ++++++++++++++++++++++++++++++++ models/login_source.go | 27 +++++++++++++++++++++++---- modules/context/auth.go | 9 +++++++-- routers/home.go | 5 +++++ routers/user/auth.go | 13 +++++++++++++ 7 files changed, 83 insertions(+), 9 deletions(-) diff --git a/integrations/release_test.go b/integrations/release_test.go index 461d3306d..492d224a5 100644 --- a/integrations/release_test.go +++ b/integrations/release_test.go @@ -112,7 +112,7 @@ 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") + // Check that user4 does not see draft and still see 10 latest releases + session2 := loginUser(t, "user4") checkLatestReleaseAndCount(t, session2, "/user2/repo1", "v0.0.11", i18n.Tr("en", "repo.release.stable"), 10) } diff --git a/integrations/repo_test.go b/integrations/repo_test.go index 36672ff62..71ad0d910 100644 --- a/integrations/repo_test.go +++ b/integrations/repo_test.go @@ -104,7 +104,7 @@ func TestViewRepoWithSymlinks(t *testing.T) { func TestViewAsRepoAdmin(t *testing.T) { for user, expectedNoDescription := range map[string]bool{ "user2": true, - "user3": false, + "user4": false, } { prepareTestEnv(t) diff --git a/models/error.go b/models/error.go index b079f06d8..649d9b87a 100644 --- a/models/error.go +++ b/models/error.go @@ -90,6 +90,38 @@ func (err ErrUserNotExist) Error() string { return fmt.Sprintf("user does not exist [uid: %d, name: %s, keyid: %d]", err.UID, err.Name, err.KeyID) } +// ErrUserProhibitLogin represents a "ErrUserProhibitLogin" kind of error. +type ErrUserProhibitLogin struct { + UID int64 + Name string +} + +// IsErrUserProhibitLogin checks if an error is a ErrUserProhibitLogin +func IsErrUserProhibitLogin(err error) bool { + _, ok := err.(ErrUserProhibitLogin) + return ok +} + +func (err ErrUserProhibitLogin) Error() string { + return fmt.Sprintf("user is not allowed login [uid: %d, name: %s]", err.UID, err.Name) +} + +// ErrUserInactive represents a "ErrUserInactive" kind of error. +type ErrUserInactive struct { + UID int64 + Name string +} + +// IsErrUserInactive checks if an error is a ErrUserInactive +func IsErrUserInactive(err error) bool { + _, ok := err.(ErrUserInactive) + return ok +} + +func (err ErrUserInactive) Error() string { + return fmt.Sprintf("user is inactive [uid: %d, name: %s]", err.UID, err.Name) +} + // ErrEmailAlreadyUsed represents a "EmailAlreadyUsed" kind of error. type ErrEmailAlreadyUsed struct { Email string diff --git a/models/login_source.go b/models/login_source.go index 46bf3a5e3..b481cb4db 100644 --- a/models/login_source.go +++ b/models/login_source.go @@ -600,16 +600,29 @@ func ExternalUserLogin(user *User, login, password string, source *LoginSource, return nil, ErrLoginSourceNotActived } + var err error switch source.Type { case LoginLDAP, LoginDLDAP: - return LoginViaLDAP(user, login, password, source, autoRegister) + user, err = LoginViaLDAP(user, login, password, source, autoRegister) case LoginSMTP: - return LoginViaSMTP(user, login, password, source.ID, source.Cfg.(*SMTPConfig), autoRegister) + user, err = LoginViaSMTP(user, login, password, source.ID, source.Cfg.(*SMTPConfig), autoRegister) case LoginPAM: - return LoginViaPAM(user, login, password, source.ID, source.Cfg.(*PAMConfig), autoRegister) + user, err = LoginViaPAM(user, login, password, source.ID, source.Cfg.(*PAMConfig), autoRegister) + default: + return nil, ErrUnsupportedLoginType + } + + if err != nil { + return nil, err + } + + if !user.IsActive { + return nil, ErrUserInactive{user.ID, user.Name} + } else if user.ProhibitLogin { + return nil, ErrUserProhibitLogin{user.ID, user.Name} } - return nil, ErrUnsupportedLoginType + return user, nil } // UserSignIn validates user name and password. @@ -645,6 +658,12 @@ func UserSignIn(username, password string) (*User, error) { switch user.LoginType { case LoginNoType, LoginPlain, LoginOAuth2: if user.IsPasswordSet() && user.ValidatePassword(password) { + if !user.IsActive { + return nil, ErrUserInactive{user.ID, user.Name} + } else if user.ProhibitLogin { + return nil, ErrUserProhibitLogin{user.ID, user.Name} + } + return user, nil } diff --git a/modules/context/auth.go b/modules/context/auth.go index 5bc34b55a..5a4d351dc 100644 --- a/modules/context/auth.go +++ b/modules/context/auth.go @@ -8,6 +8,7 @@ import ( "net/url" "code.gitea.io/gitea/modules/auth" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "github.com/go-macaron/csrf" macaron "gopkg.in/macaron.v1" @@ -32,8 +33,12 @@ func Toggle(options *ToggleOptions) macaron.Handler { // Check prohibit login users. if ctx.IsSigned { - - if ctx.User.ProhibitLogin { + if !ctx.User.IsActive && setting.Service.RegisterEmailConfirm { + ctx.Data["Title"] = ctx.Tr("auth.active_your_account") + ctx.HTML(200, "user/auth/activate") + return + } else if !ctx.User.IsActive || ctx.User.ProhibitLogin { + log.Info("Failed authentication attempt for %s from %s", ctx.User.Name, ctx.RemoteAddr()) ctx.Data["Title"] = ctx.Tr("auth.prohibit_login") ctx.HTML(200, "user/auth/prohibit_login") return diff --git a/routers/home.go b/routers/home.go index a09d025d2..bea013911 100644 --- a/routers/home.go +++ b/routers/home.go @@ -12,6 +12,7 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/context" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/search" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" @@ -39,6 +40,10 @@ func Home(ctx *context.Context) { if !ctx.User.IsActive && setting.Service.RegisterEmailConfirm { ctx.Data["Title"] = ctx.Tr("auth.active_your_account") ctx.HTML(200, user.TplActivate) + } else if !ctx.User.IsActive || ctx.User.ProhibitLogin { + log.Info("Failed authentication attempt for %s from %s", ctx.User.Name, ctx.RemoteAddr()) + ctx.Data["Title"] = ctx.Tr("auth.prohibit_login") + ctx.HTML(200, "user/auth/prohibit_login") } else { user.Dashboard(ctx) } diff --git a/routers/user/auth.go b/routers/user/auth.go index 24b35e6f6..c86eb3540 100644 --- a/routers/user/auth.go +++ b/routers/user/auth.go @@ -161,6 +161,19 @@ func SignInPost(ctx *context.Context, form auth.SignInForm) { } else if models.IsErrEmailAlreadyUsed(err) { ctx.RenderWithErr(ctx.Tr("form.email_been_used"), tplSignIn, &form) log.Info("Failed authentication attempt for %s from %s", form.UserName, ctx.RemoteAddr()) + } else if models.IsErrUserProhibitLogin(err) { + log.Info("Failed authentication attempt for %s from %s", form.UserName, ctx.RemoteAddr()) + ctx.Data["Title"] = ctx.Tr("auth.prohibit_login") + ctx.HTML(200, "user/auth/prohibit_login") + } else if models.IsErrUserInactive(err) { + if setting.Service.RegisterEmailConfirm { + ctx.Data["Title"] = ctx.Tr("auth.active_your_account") + ctx.HTML(200, TplActivate) + } else { + log.Info("Failed authentication attempt for %s from %s", form.UserName, ctx.RemoteAddr()) + ctx.Data["Title"] = ctx.Tr("auth.prohibit_login") + ctx.HTML(200, "user/auth/prohibit_login") + } } else { ctx.ServerError("UserSignIn", err) }