Various fixes in login sources (#10428)

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)
 			}