Skip to content

[WIP] Enforce usertype via GetIndividualUserByName #2118

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

[WIP] Enforce usertype via GetIndividualUserByName #2118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e7ba9e8
Enforce user type
sapk Jul 6, 2017
14cb186
use generic GetUserByNameAndType for GetOrgByName
sapk Jul 7, 2017
23836ae
Make ErrUserNotExist more generic
sapk Jul 7, 2017
2375d8d
Add GetIndividualUserByName
sapk Jul 7, 2017
7e93f6d
Enforce individual on collaborator
sapk Jul 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 2 additions & 17 deletions models/error.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ type ErrUserNotExist struct {
UID int64
Name string
KeyID int64
Type UserType
}

// IsErrUserNotExist checks if an error is a ErrUserNotExist.
Expand All @@ -75,7 +76,7 @@ func IsErrUserNotExist(err error) bool {
}

func (err ErrUserNotExist) Error() string {
return fmt.Sprintf("user does not exist [uid: %d, name: %s, keyid: %d]", err.UID, err.Name, err.KeyID)
return fmt.Sprintf("user does not exist [uid: %d, name: %s, keyid: %d, type: %d]", err.UID, err.Name, err.KeyID, err.Type)
}

// ErrEmailAlreadyUsed represents a "EmailAlreadyUsed" kind of error.
Expand Down Expand Up @@ -448,22 +449,6 @@ func (err ErrAccessTokenEmpty) Error() string {
// \_______ /__| \___ (____ /___| /__/_____ \(____ /__| |__|\____/|___| /
// \/ /_____/ \/ \/ \/ \/ \/

// ErrOrgNotExist represents a "OrgNotExist" kind of error.
type ErrOrgNotExist struct {
ID int64
Name string
}

// IsErrOrgNotExist checks if an error is a ErrOrgNotExist.
func IsErrOrgNotExist(err error) bool {
_, ok := err.(ErrOrgNotExist)
return ok
}

func (err ErrOrgNotExist) Error() string {
return fmt.Sprintf("org does not exist [id: %d, name: %s]", err.ID, err.Name)
}

// ErrLastOrgOwner represents a "LastOrgOwner" kind of error.
type ErrLastOrgOwner struct {
UID int64
Expand Down
16 changes: 8 additions & 8 deletions models/login_source.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ func LoginViaLDAP(user *User, login, password string, source *LoginSource, autoR
sr := source.Cfg.(*LDAPConfig).SearchEntry(login, password, source.Type == LoginDLDAP)
if sr == nil {
// User not in LDAP, do nothing
return nil, ErrUserNotExist{0, login, 0}
return nil, ErrUserNotExist{0, login, 0, -1}
}
Copy link
Member

Choose a reason for hiding this comment

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

Why -1? I think it is better user type individual as you can't login with organization. Same in most other places.

Copy link
Member Author

Choose a reason for hiding this comment

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

A default value ^^ correspoding to any value.
I will review each case for determining.
I was planning on keep -1 when we don't check the type of the user but for external sources this could be set to IndividualUser as it is supposed.

Copy link
Member

Choose a reason for hiding this comment

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

Please don't introduce (more) magic numbers, if you want -1 to be "I don't care" then const UserTypeIDoNotCare -1 🙂


if !autoRegister {
Expand Down Expand Up @@ -524,9 +524,9 @@ func LoginViaSMTP(user *User, login, password string, sourceID int64, cfg *SMTPC
if len(cfg.AllowedDomains) > 0 {
idx := strings.Index(login, "@")
if idx == -1 {
return nil, ErrUserNotExist{0, login, 0}
return nil, ErrUserNotExist{0, login, 0, -1}
} else if !com.IsSliceContainsStr(strings.Split(cfg.AllowedDomains, ","), login[idx+1:]) {
return nil, ErrUserNotExist{0, login, 0}
return nil, ErrUserNotExist{0, login, 0, -1}
}
}

Expand All @@ -545,7 +545,7 @@ func LoginViaSMTP(user *User, login, password string, sourceID int64, cfg *SMTPC
tperr, ok := err.(*textproto.Error)
if (ok && tperr.Code == 535) ||
strings.Contains(err.Error(), "Username and Password not accepted") {
return nil, ErrUserNotExist{0, login, 0}
return nil, ErrUserNotExist{0, login, 0, -1}
}
return nil, err
}
Expand Down Expand Up @@ -585,7 +585,7 @@ func LoginViaSMTP(user *User, login, password string, sourceID int64, cfg *SMTPC
func LoginViaPAM(user *User, login, password string, sourceID int64, cfg *PAMConfig, autoRegister bool) (*User, error) {
if err := pam.Auth(cfg.ServiceName, login, password); err != nil {
if strings.Contains(err.Error(), "Authentication failure") {
return nil, ErrUserNotExist{0, login, 0}
return nil, ErrUserNotExist{0, login, 0, -1}
}
return nil, err
}
Expand Down Expand Up @@ -643,7 +643,7 @@ func UserSignIn(username, password string) (*User, error) {
} else {
trimmedUsername := strings.TrimSpace(username)
if len(trimmedUsername) == 0 {
return nil, ErrUserNotExist{0, username, 0}
return nil, ErrUserNotExist{0, username, 0, -1}
}

user = &User{LowerName: strings.ToLower(trimmedUsername)}
Expand All @@ -661,7 +661,7 @@ func UserSignIn(username, password string) (*User, error) {
return user, nil
}

return nil, ErrUserNotExist{user.ID, user.Name, 0}
return nil, ErrUserNotExist{user.ID, user.Name, 0, -1}

default:
var source LoginSource
Expand Down Expand Up @@ -694,5 +694,5 @@ func UserSignIn(username, password string) (*User, error) {
log.Warn("Failed to login '%s' via '%s': %v", username, source.Name, err)
}

return nil, ErrUserNotExist{user.ID, user.Name, 0}
return nil, ErrUserNotExist{user.ID, user.Name, 0, -1}
}
15 changes: 1 addition & 14 deletions models/org.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,20 +177,7 @@ func CreateOrganization(org, owner *User) (err error) {

// GetOrgByName returns organization by given name.
func GetOrgByName(name string) (*User, error) {
if len(name) == 0 {
return nil, ErrOrgNotExist{0, name}
}
u := &User{
LowerName: strings.ToLower(name),
Type: UserTypeOrganization,
}
has, err := x.Get(u)
if err != nil {
return nil, err
} else if !has {
return nil, ErrOrgNotExist{0, name}
}
return u, nil
return GetUserByNameAndType(name, UserTypeOrganization)
}

// CountOrganizations returns number of organizations.
Expand Down
7 changes: 5 additions & 2 deletions models/org_team.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,11 +226,14 @@ func NewTeam(t *Team) (err error) {
return err
}

has, err := x.Id(t.OrgID).Get(new(User))
has, err := x.Id(t.OrgID).Get(&User{
Type: UserTypeOrganization,
})

if err != nil {
return err
} else if !has {
return ErrOrgNotExist{t.OrgID, ""}
return ErrUserNotExist{t.OrgID, "", 0, UserTypeOrganization}
}

t.LowerName = strings.ToLower(t.Name)
Expand Down
5 changes: 3 additions & 2 deletions models/org_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,10 +222,11 @@ func TestGetOrgByName(t *testing.T) {
assert.Equal(t, "user3", org.Name)

org, err = GetOrgByName("user2") // user2 is an individual
assert.True(t, IsErrOrgNotExist(err))
assert.True(t, IsErrUserNotExist(err))
assert.Equal(t, UserTypeOrganization, err.(ErrUserNotExist).Type)

org, err = GetOrgByName("") // corner case
assert.True(t, IsErrOrgNotExist(err))
assert.True(t, IsErrUserNotExist(err))
}

func TestCountOrganizations(t *testing.T) {
Expand Down
31 changes: 24 additions & 7 deletions models/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1089,7 +1089,7 @@ func GetUserByKeyID(keyID int64) (*User, error) {
return nil, err
}
if !has {
return nil, ErrUserNotExist{0, "", keyID}
return nil, ErrUserNotExist{0, "", keyID, -1}
}
return &user, nil
}
Expand All @@ -1100,7 +1100,7 @@ func getUserByID(e Engine, id int64) (*User, error) {
if err != nil {
return nil, err
} else if !has {
return nil, ErrUserNotExist{id, "", 0}
return nil, ErrUserNotExist{id, "", 0, -1}
}
return u, nil
}
Expand All @@ -1116,26 +1116,43 @@ func GetAssigneeByID(repo *Repository, userID int64) (*User, error) {
if err != nil {
return nil, err
} else if !has {
return nil, ErrUserNotExist{userID, "", 0}
return nil, ErrUserNotExist{userID, "", 0, -1}
}
return GetUserByID(userID)
}

// GetUserByName returns user by given name.
func GetUserByName(name string) (*User, error) {
if len(name) == 0 {
return nil, ErrUserNotExist{0, name, 0}
return nil, ErrUserNotExist{0, name, 0, -1}
}
u := &User{LowerName: strings.ToLower(name)}
has, err := x.Get(u)
if err != nil {
return nil, err
} else if !has {
return nil, ErrUserNotExist{0, name, 0}
return nil, ErrUserNotExist{0, name, 0, -1}
}
return u, nil
}

// GetUserByNameAndType returns user by given name and type. //TODO add test
func GetUserByNameAndType(name string, utype UserType) (*User, error) {
u, err := GetUserByName(name)
if err != nil {
return nil, err
}
if u.Type != utype {
return nil, ErrUserNotExist{0, name, 0, utype}
}
return u, nil
}

// GetIndividualUserByName returns user of TypeIndividual by given name. //TODO add test
func GetIndividualUserByName(name string) (*User, error) {
return GetUserByNameAndType(name, UserTypeIndividual)
}

// GetUserEmailsByNames returns a list of e-mails corresponds to names.
func GetUserEmailsByNames(names []string) []string {
mails := make([]string, 0, len(names))
Expand Down Expand Up @@ -1221,7 +1238,7 @@ func ValidateCommitsWithEmails(oldCommits *list.List) *list.List {
// GetUserByEmail returns the user object by given e-mail if exists.
func GetUserByEmail(email string) (*User, error) {
if len(email) == 0 {
return nil, ErrUserNotExist{0, email, 0}
return nil, ErrUserNotExist{0, email, 0, -1}
}

email = strings.ToLower(email)
Expand All @@ -1245,7 +1262,7 @@ func GetUserByEmail(email string) (*User, error) {
return GetUserByID(emailAddress.UID)
}

return nil, ErrUserNotExist{0, email, 0}
return nil, ErrUserNotExist{0, email, 0, -1}
}

// GetUser checks if a user already exists
Expand Down
2 changes: 1 addition & 1 deletion models/user_mail.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ func MakeEmailPrimary(email *EmailAddress) error {
if err != nil {
return err
} else if !has {
return ErrUserNotExist{email.UID, "", 0}
return ErrUserNotExist{email.UID, "", 0, -1}
}

// Make sure the former primary email doesn't disappear.
Expand Down
4 changes: 2 additions & 2 deletions models/user_openid.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ func ToggleUserOpenIDVisibility(id int64) (err error) {
// GetUserByOpenID returns the user object by given OpenID if exists.
func GetUserByOpenID(uri string) (*User, error) {
if len(uri) == 0 {
return nil, ErrUserNotExist{0, uri, 0}
return nil, ErrUserNotExist{0, uri, 0, -1}
}

uri, err := openid.Normalize(uri)
Expand All @@ -120,5 +120,5 @@ func GetUserByOpenID(uri string) (*User, error) {
return GetUserByID(oid.UID)
}

return nil, ErrUserNotExist{0, uri, 0}
return nil, ErrUserNotExist{0, uri, 0, -1}
}
6 changes: 3 additions & 3 deletions modules/context/org.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,12 @@ func HandleOrgAssignment(ctx *Context, args ...bool) {
orgName := ctx.Params(":org")

var err error
ctx.Org.Organization, err = models.GetUserByName(orgName)
ctx.Org.Organization, err = models.GetOrgByName(orgName)
if err != nil {
if models.IsErrUserNotExist(err) {
ctx.Handle(404, "GetUserByName", err)
ctx.Handle(404, "GetOrgByName", err)
} else {
ctx.Handle(500, "GetUserByName", err)
ctx.Handle(500, "GetOrgByName", err)
}
return
}
Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ func orgAssignment(args ...bool) macaron.Handler {
if assignOrg {
ctx.Org.Organization, err = models.GetOrgByName(ctx.Params(":orgname"))
if err != nil {
if models.IsErrOrgNotExist(err) {
if models.IsErrUserNotExist(err) {
ctx.Status(404)
} else {
ctx.Error(500, "GetOrgByName", err)
Expand Down
6 changes: 3 additions & 3 deletions routers/api/v1/repo/collaborators.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func IsCollaborator(ctx *context.APIContext) {
ctx.Error(403, "", "User does not have push access")
return
}
user, err := models.GetUserByName(ctx.Params(":collaborator"))
user, err := models.GetIndividualUserByName(ctx.Params(":collaborator"))
if err != nil {
if models.IsErrUserNotExist(err) {
ctx.Error(422, "", err)
Expand All @@ -62,7 +62,7 @@ func AddCollaborator(ctx *context.APIContext, form api.AddCollaboratorOption) {
ctx.Error(403, "", "User does not have push access")
return
}
collaborator, err := models.GetUserByName(ctx.Params(":collaborator"))
collaborator, err := models.GetIndividualUserByName(ctx.Params(":collaborator"))
if err != nil {
if models.IsErrUserNotExist(err) {
ctx.Error(422, "", err)
Expand Down Expand Up @@ -94,7 +94,7 @@ func DeleteCollaborator(ctx *context.APIContext) {
return
}

collaborator, err := models.GetUserByName(ctx.Params(":collaborator"))
collaborator, err := models.GetIndividualUserByName(ctx.Params(":collaborator"))
if err != nil {
if models.IsErrUserNotExist(err) {
ctx.Error(422, "", err)
Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/repo/fork.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func CreateFork(ctx *context.APIContext, form api.CreateForkOption) {
} else {
org, err := models.GetOrgByName(*form.Organization)
if err != nil {
if models.IsErrOrgNotExist(err) {
if models.IsErrUserNotExist(err) {
ctx.Error(422, "", err)
} else {
ctx.Error(500, "GetOrgByName", err)
Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/repo/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ func CreateOrgRepo(ctx *context.APIContext, opt api.CreateRepoOption) {

org, err := models.GetOrgByName(ctx.Params(":org"))
if err != nil {
if models.IsErrOrgNotExist(err) {
if models.IsErrUserNotExist(err) {
ctx.Error(422, "", err)
} else {
ctx.Error(500, "GetOrgByName", err)
Expand Down
6 changes: 3 additions & 3 deletions routers/user/home.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,12 @@ func getDashboardContextUser(ctx *context.Context) *models.User {
orgName := ctx.Params(":org")
if len(orgName) > 0 {
// Organization.
org, err := models.GetUserByName(orgName)
org, err := models.GetOrgByName(orgName)
if err != nil {
if models.IsErrUserNotExist(err) {
ctx.Handle(404, "GetUserByName", err)
ctx.Handle(404, "GetOrgByName", err)
} else {
ctx.Handle(500, "GetUserByName", err)
ctx.Handle(500, "GetOrgByName", err)
}
return nil
}
Expand Down