From 09dbd85a3a9043de9f8ea06a0a7ee7fd45711344 Mon Sep 17 00:00:00 2001 From: guillep2k <18600385+guillep2k@users.noreply.github.com> Date: Sun, 23 Feb 2020 16:52:05 -0300 Subject: [PATCH] Various fixes in login sources (#10428) --- models/error.go | 15 +++++++++++++++ models/login_source.go | 27 +++++++++++++-------------- models/user.go | 9 +++++++++ modules/auth/pam/pam.go | 10 ++++++---- modules/auth/pam/pam_stub.go | 4 ++-- modules/repository/create_test.go | 2 +- options/locale/locale_en-US.ini | 1 + routers/admin/users.go | 3 +++ routers/api/v1/admin/org.go | 1 + routers/api/v1/admin/user.go | 1 + routers/api/v1/org/org.go | 1 + routers/api/v1/repo/migrate.go | 2 ++ routers/user/auth.go | 4 ++++ routers/user/auth_openid.go | 4 ++++ routers/user/setting/profile.go | 3 +++ 15 files changed, 66 insertions(+), 21 deletions(-) diff --git a/models/error.go b/models/error.go index a679372737..675b7406b8 100644 --- a/models/error.go +++ b/models/error.go @@ -57,6 +57,21 @@ func (err ErrNamePatternNotAllowed) Error() string { return fmt.Sprintf("name pattern is not allowed [pattern: %s]", err.Pattern) } +// ErrNameCharsNotAllowed represents a "character not allowed in name" error. +type ErrNameCharsNotAllowed struct { + Name string +} + +// IsErrNameCharsNotAllowed checks if an error is an ErrNameCharsNotAllowed. +func IsErrNameCharsNotAllowed(err error) bool { + _, ok := err.(ErrNameCharsNotAllowed) + return ok +} + +func (err ErrNameCharsNotAllowed) Error() string { + return fmt.Sprintf("User name is invalid [%s]: must be valid alpha or numeric or dash(-_) or dot characters", err.Name) +} + // ErrSSHDisabled represents an "SSH disabled" error. type ErrSSHDisabled struct { } diff --git a/models/login_source.go b/models/login_source.go index f5dae860f8..2774d6f80d 100644 --- a/models/login_source.go +++ b/models/login_source.go @@ -12,7 +12,6 @@ import ( "fmt" "net/smtp" "net/textproto" - "regexp" "strings" "code.gitea.io/gitea/modules/auth/ldap" @@ -455,10 +454,6 @@ func composeFullName(firstname, surname, username string) string { } } -var ( - alphaDashDotPattern = regexp.MustCompile(`[^\w-\.]`) -) - // LoginViaLDAP queries if login/password is valid against the LDAP directory pool, // and create a local user if success when enabled. func LoginViaLDAP(user *User, login, password string, source *LoginSource) (*User, error) { @@ -503,10 +498,6 @@ func LoginViaLDAP(user *User, login, password string, source *LoginSource) (*Use if len(sr.Username) == 0 { sr.Username = login } - // Validate username make sure it satisfies requirement. - if alphaDashDotPattern.MatchString(sr.Username) { - return nil, fmt.Errorf("Invalid pattern for attribute 'username' [%s]: must be valid alpha or numeric or dash(-_) or dot characters", sr.Username) - } if len(sr.Mail) == 0 { sr.Mail = fmt.Sprintf("%s@localhost", sr.Username) @@ -666,7 +657,8 @@ func LoginViaSMTP(user *User, login, password string, sourceID int64, cfg *SMTPC // LoginViaPAM queries if login/password is valid against the PAM, // and create a local user if success when enabled. func LoginViaPAM(user *User, login, password string, sourceID int64, cfg *PAMConfig) (*User, error) { - if err := pam.Auth(cfg.ServiceName, login, password); err != nil { + pamLogin, err := pam.Auth(cfg.ServiceName, login, password) + if err != nil { if strings.Contains(err.Error(), "Authentication failure") { return nil, ErrUserNotExist{0, login, 0} } @@ -677,14 +669,21 @@ func LoginViaPAM(user *User, login, password string, sourceID int64, cfg *PAMCon return user, nil } + // Allow PAM sources with `@` in their name, like from Active Directory + username := pamLogin + idx := strings.Index(pamLogin, "@") + if idx > -1 { + username = pamLogin[:idx] + } + user = &User{ - LowerName: strings.ToLower(login), - Name: login, - Email: login, + LowerName: strings.ToLower(username), + Name: username, + Email: pamLogin, Passwd: password, LoginType: LoginPAM, LoginSource: sourceID, - LoginName: login, + LoginName: login, // This is what the user typed in IsActive: true, } return user, CreateUser(user) diff --git a/models/user.go b/models/user.go index 5031849f90..bf59c1240b 100644 --- a/models/user.go +++ b/models/user.go @@ -18,6 +18,7 @@ import ( "image/png" "os" "path/filepath" + "regexp" "strconv" "strings" "time" @@ -87,6 +88,9 @@ var ( // ErrUnsupportedLoginType login source is unknown error ErrUnsupportedLoginType = errors.New("Login source is unknown") + + // Characters prohibited in a user name (anything except A-Za-z0-9_.-) + alphaDashDotPattern = regexp.MustCompile(`[^\w-\.]`) ) // User represents the object of individual and member of organization. @@ -906,6 +910,11 @@ func isUsableName(names, patterns []string, name string) error { // IsUsableUsername returns an error when a username is reserved func IsUsableUsername(name string) error { + // Validate username make sure it satisfies requirement. + if alphaDashDotPattern.MatchString(name) { + // Note: usually this error is normally caught up earlier in the UI + return ErrNameCharsNotAllowed{Name: name} + } return isUsableName(reservedUsernames, reservedUserPatterns, name) } diff --git a/modules/auth/pam/pam.go b/modules/auth/pam/pam.go index 6f0f7240ae..ca299b08ba 100644 --- a/modules/auth/pam/pam.go +++ b/modules/auth/pam/pam.go @@ -13,7 +13,7 @@ import ( ) // Auth pam auth service -func Auth(serviceName, userName, passwd string) error { +func Auth(serviceName, userName, passwd string) (string, error) { t, err := pam.StartFunc(serviceName, userName, func(s pam.Style, msg string) (string, error) { switch s { case pam.PromptEchoOff: @@ -25,12 +25,14 @@ func Auth(serviceName, userName, passwd string) error { }) if err != nil { - return err + return "", err } if err = t.Authenticate(0); err != nil { - return err + return "", err } - return nil + // PAM login names might suffer transformations in the PAM stack. + // We should take whatever the PAM stack returns for it. + return t.GetItem(pam.User) } diff --git a/modules/auth/pam/pam_stub.go b/modules/auth/pam/pam_stub.go index ee2527dd89..604799ca97 100644 --- a/modules/auth/pam/pam_stub.go +++ b/modules/auth/pam/pam_stub.go @@ -11,6 +11,6 @@ import ( ) // Auth not supported lack of pam tag -func Auth(serviceName, userName, passwd string) error { - return errors.New("PAM not supported") +func Auth(serviceName, userName, passwd string) (string, error) { + return "", errors.New("PAM not supported") } diff --git a/modules/repository/create_test.go b/modules/repository/create_test.go index ee76d2181b..d3e8bf5af1 100644 --- a/modules/repository/create_test.go +++ b/modules/repository/create_test.go @@ -35,7 +35,7 @@ func TestIncludesAllRepositoriesTeams(t *testing.T) { // Create org. org := &models.User{ - Name: "All repo", + Name: "All_repo", IsActive: true, Type: models.UserTypeOrganization, Visibility: structs.VisibleTypePublic, diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 7fe7bf697d..cbe8aaad7a 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -379,6 +379,7 @@ user_bio = Biography form.name_reserved = The username '%s' is reserved. form.name_pattern_not_allowed = The pattern '%s' is not allowed in a username. +form.name_chars_not_allowed = User name '%s' contains invalid characters. [settings] profile = Profile diff --git a/routers/admin/users.go b/routers/admin/users.go index 6753699696..10ae622c32 100644 --- a/routers/admin/users.go +++ b/routers/admin/users.go @@ -124,6 +124,9 @@ func NewUserPost(ctx *context.Context, form auth.AdminCreateUserForm) { case models.IsErrNamePatternNotAllowed(err): ctx.Data["Err_UserName"] = true ctx.RenderWithErr(ctx.Tr("user.form.name_pattern_not_allowed", err.(models.ErrNamePatternNotAllowed).Pattern), tplUserNew, &form) + case models.IsErrNameCharsNotAllowed(err): + ctx.Data["Err_UserName"] = true + ctx.RenderWithErr(ctx.Tr("user.form.name_chars_not_allowed", err.(models.ErrNameCharsNotAllowed).Name), tplUserNew, &form) default: ctx.ServerError("CreateUser", err) } diff --git a/routers/api/v1/admin/org.go b/routers/api/v1/admin/org.go index 01c2c49734..11c2f1dc49 100644 --- a/routers/api/v1/admin/org.go +++ b/routers/api/v1/admin/org.go @@ -67,6 +67,7 @@ func CreateOrg(ctx *context.APIContext, form api.CreateOrgOption) { if err := models.CreateOrganization(org, u); err != nil { if models.IsErrUserAlreadyExist(err) || models.IsErrNameReserved(err) || + models.IsErrNameCharsNotAllowed(err) || models.IsErrNamePatternNotAllowed(err) { ctx.Error(http.StatusUnprocessableEntity, "", err) } else { diff --git a/routers/api/v1/admin/user.go b/routers/api/v1/admin/user.go index 0fbb9cdfe2..38237d6744 100644 --- a/routers/api/v1/admin/user.go +++ b/routers/api/v1/admin/user.go @@ -91,6 +91,7 @@ func CreateUser(ctx *context.APIContext, form api.CreateUserOption) { if models.IsErrUserAlreadyExist(err) || models.IsErrEmailAlreadyUsed(err) || models.IsErrNameReserved(err) || + models.IsErrNameCharsNotAllowed(err) || models.IsErrNamePatternNotAllowed(err) { ctx.Error(http.StatusUnprocessableEntity, "", err) } else { diff --git a/routers/api/v1/org/org.go b/routers/api/v1/org/org.go index fde58bd5c1..b79761c875 100644 --- a/routers/api/v1/org/org.go +++ b/routers/api/v1/org/org.go @@ -179,6 +179,7 @@ func Create(ctx *context.APIContext, form api.CreateOrgOption) { if err := models.CreateOrganization(org, ctx.User); err != nil { if models.IsErrUserAlreadyExist(err) || models.IsErrNameReserved(err) || + models.IsErrNameCharsNotAllowed(err) || models.IsErrNamePatternNotAllowed(err) { ctx.Error(http.StatusUnprocessableEntity, "", err) } else { diff --git a/routers/api/v1/repo/migrate.go b/routers/api/v1/repo/migrate.go index 4b9d3958ad..fd0db7814c 100644 --- a/routers/api/v1/repo/migrate.go +++ b/routers/api/v1/repo/migrate.go @@ -199,6 +199,8 @@ func handleMigrateError(ctx *context.APIContext, repoOwner *models.User, remoteA ctx.Error(http.StatusUnprocessableEntity, "", fmt.Sprintf("You have already reached your limit of %d repositories.", repoOwner.MaxCreationLimit())) case models.IsErrNameReserved(err): ctx.Error(http.StatusUnprocessableEntity, "", fmt.Sprintf("The username '%s' is reserved.", err.(models.ErrNameReserved).Name)) + case models.IsErrNameCharsNotAllowed(err): + ctx.Error(http.StatusUnprocessableEntity, "", fmt.Sprintf("The username '%s' contains invalid characters.", err.(models.ErrNameCharsNotAllowed).Name)) case models.IsErrNamePatternNotAllowed(err): ctx.Error(http.StatusUnprocessableEntity, "", fmt.Sprintf("The pattern '%s' is not allowed in a username.", err.(models.ErrNamePatternNotAllowed).Pattern)) default: diff --git a/routers/user/auth.go b/routers/user/auth.go index 6395836480..be0396cce1 100644 --- a/routers/user/auth.go +++ b/routers/user/auth.go @@ -928,6 +928,7 @@ func LinkAccountPostRegister(ctx *context.Context, cpt *captcha.Captcha, form au LoginName: gothUser.(goth.User).UserID, } + //nolint: dupl if err := models.CreateUser(u); err != nil { switch { case models.IsErrUserAlreadyExist(err): @@ -942,6 +943,9 @@ func LinkAccountPostRegister(ctx *context.Context, cpt *captcha.Captcha, form au case models.IsErrNamePatternNotAllowed(err): ctx.Data["Err_UserName"] = true ctx.RenderWithErr(ctx.Tr("user.form.name_pattern_not_allowed", err.(models.ErrNamePatternNotAllowed).Pattern), tplLinkAccount, &form) + case models.IsErrNameCharsNotAllowed(err): + ctx.Data["Err_UserName"] = true + ctx.RenderWithErr(ctx.Tr("user.form.name_chars_not_allowed", err.(models.ErrNameCharsNotAllowed).Name), tplLinkAccount, &form) default: ctx.ServerError("CreateUser", err) } diff --git a/routers/user/auth_openid.go b/routers/user/auth_openid.go index ccaea8264f..bd05538ad3 100644 --- a/routers/user/auth_openid.go +++ b/routers/user/auth_openid.go @@ -400,6 +400,7 @@ func RegisterOpenIDPost(ctx *context.Context, cpt *captcha.Captcha, form auth.Si Passwd: password, IsActive: !setting.Service.RegisterEmailConfirm, } + //nolint: dupl if err := models.CreateUser(u); err != nil { switch { case models.IsErrUserAlreadyExist(err): @@ -414,6 +415,9 @@ func RegisterOpenIDPost(ctx *context.Context, cpt *captcha.Captcha, form auth.Si case models.IsErrNamePatternNotAllowed(err): ctx.Data["Err_UserName"] = true ctx.RenderWithErr(ctx.Tr("user.form.name_pattern_not_allowed", err.(models.ErrNamePatternNotAllowed).Pattern), tplSignUpOID, &form) + case models.IsErrNameCharsNotAllowed(err): + ctx.Data["Err_UserName"] = true + ctx.RenderWithErr(ctx.Tr("user.form.name_chars_not_allowed", err.(models.ErrNameCharsNotAllowed).Name), tplSignUpOID, &form) default: ctx.ServerError("CreateUser", err) } diff --git a/routers/user/setting/profile.go b/routers/user/setting/profile.go index a5cc433a9a..d6f25f9135 100644 --- a/routers/user/setting/profile.go +++ b/routers/user/setting/profile.go @@ -58,6 +58,9 @@ func handleUsernameChange(ctx *context.Context, newName string) { case models.IsErrNamePatternNotAllowed(err): ctx.Flash.Error(ctx.Tr("user.form.name_pattern_not_allowed", newName)) ctx.Redirect(setting.AppSubURL + "/user/settings") + case models.IsErrNameCharsNotAllowed(err): + ctx.Flash.Error(ctx.Tr("user.form.name_chars_not_allowed", newName)) + ctx.Redirect(setting.AppSubURL + "/user/settings") default: ctx.ServerError("ChangeUserName", err) }