Skip to content

Commit ec0ae5d

Browse files
ethantkoeniglunny
ethantkoenig
authored and
lunny
committed
Refactor and fix incorrect comment (#1247)
1 parent 7d8f9d1 commit ec0ae5d

File tree

20 files changed

+74
-84
lines changed

20 files changed

+74
-84
lines changed

‎cmd/serv.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,7 @@ func runServ(c *cli.Context) error {
232232
fail("internal error", "Failed to get user by key ID(%d): %v", keyID, err)
233233
}
234234

235-
mode, err := models.AccessLevel(user, repo)
235+
mode, err := models.AccessLevel(user.ID, repo)
236236
if err != nil {
237237
fail("Internal error", "Failed to check access: %v", err)
238238
} else if mode < requestedMode {

‎models/access.go

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -59,41 +59,41 @@ type Access struct {
5959
Mode AccessMode
6060
}
6161

62-
func accessLevel(e Engine, user *User, repo *Repository) (AccessMode, error) {
62+
func accessLevel(e Engine, userID int64, repo *Repository) (AccessMode, error) {
6363
mode := AccessModeNone
6464
if !repo.IsPrivate {
6565
mode = AccessModeRead
6666
}
6767

68-
if user == nil {
68+
if userID == 0 {
6969
return mode, nil
7070
}
7171

72-
if user.ID == repo.OwnerID {
72+
if userID == repo.OwnerID {
7373
return AccessModeOwner, nil
7474
}
7575

76-
a := &Access{UserID: user.ID, RepoID: repo.ID}
76+
a := &Access{UserID: userID, RepoID: repo.ID}
7777
if has, err := e.Get(a); !has || err != nil {
7878
return mode, err
7979
}
8080
return a.Mode, nil
8181
}
8282

8383
// AccessLevel returns the Access a user has to a repository. Will return NoneAccess if the
84-
// user does not have access. User can be nil!
85-
func AccessLevel(user *User, repo *Repository) (AccessMode, error) {
86-
return accessLevel(x, user, repo)
84+
// user does not have access.
85+
func AccessLevel(userID int64, repo *Repository) (AccessMode, error) {
86+
return accessLevel(x, userID, repo)
8787
}
8888

89-
func hasAccess(e Engine, user *User, repo *Repository, testMode AccessMode) (bool, error) {
90-
mode, err := accessLevel(e, user, repo)
89+
func hasAccess(e Engine, userID int64, repo *Repository, testMode AccessMode) (bool, error) {
90+
mode, err := accessLevel(e, userID, repo)
9191
return testMode <= mode, err
9292
}
9393

94-
// HasAccess returns true if someone has the request access level. User can be nil!
95-
func HasAccess(user *User, repo *Repository, testMode AccessMode) (bool, error) {
96-
return hasAccess(x, user, repo, testMode)
94+
// HasAccess returns true if user has access to repo
95+
func HasAccess(userID int64, repo *Repository, testMode AccessMode) (bool, error) {
96+
return hasAccess(x, userID, repo, testMode)
9797
}
9898

9999
type repoAccess struct {

‎models/access_test.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,19 +25,19 @@ func TestAccessLevel(t *testing.T) {
2525
repo1 := AssertExistsAndLoadBean(t, &Repository{OwnerID: 2, IsPrivate: false}).(*Repository)
2626
repo2 := AssertExistsAndLoadBean(t, &Repository{OwnerID: 3, IsPrivate: true}).(*Repository)
2727

28-
level, err := AccessLevel(user1, repo1)
28+
level, err := AccessLevel(user1.ID, repo1)
2929
assert.NoError(t, err)
3030
assert.Equal(t, AccessModeOwner, level)
3131

32-
level, err = AccessLevel(user1, repo2)
32+
level, err = AccessLevel(user1.ID, repo2)
3333
assert.NoError(t, err)
3434
assert.Equal(t, AccessModeWrite, level)
3535

36-
level, err = AccessLevel(user2, repo1)
36+
level, err = AccessLevel(user2.ID, repo1)
3737
assert.NoError(t, err)
3838
assert.Equal(t, AccessModeRead, level)
3939

40-
level, err = AccessLevel(user2, repo2)
40+
level, err = AccessLevel(user2.ID, repo2)
4141
assert.NoError(t, err)
4242
assert.Equal(t, AccessModeNone, level)
4343
}
@@ -51,19 +51,19 @@ func TestHasAccess(t *testing.T) {
5151
repo2 := AssertExistsAndLoadBean(t, &Repository{OwnerID: 3, IsPrivate: true}).(*Repository)
5252

5353
for _, accessMode := range accessModes {
54-
has, err := HasAccess(user1, repo1, accessMode)
54+
has, err := HasAccess(user1.ID, repo1, accessMode)
5555
assert.NoError(t, err)
5656
assert.True(t, has)
5757

58-
has, err = HasAccess(user1, repo2, accessMode)
58+
has, err = HasAccess(user1.ID, repo2, accessMode)
5959
assert.NoError(t, err)
6060
assert.Equal(t, accessMode <= AccessModeWrite, has)
6161

62-
has, err = HasAccess(user2, repo1, accessMode)
62+
has, err = HasAccess(user2.ID, repo1, accessMode)
6363
assert.NoError(t, err)
6464
assert.Equal(t, accessMode <= AccessModeRead, has)
6565

66-
has, err = HasAccess(user2, repo2, accessMode)
66+
has, err = HasAccess(user2.ID, repo2, accessMode)
6767
assert.NoError(t, err)
6868
assert.Equal(t, accessMode <= AccessModeNone, has)
6969
}

‎models/issue.go

Lines changed: 9 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -374,7 +374,7 @@ func (issue *Issue) RemoveLabel(doer *User, label *Label) error {
374374
return err
375375
}
376376

377-
if has, err := HasAccess(doer, issue.Repo, AccessModeWrite); err != nil {
377+
if has, err := HasAccess(doer.ID, issue.Repo, AccessModeWrite); err != nil {
378378
return err
379379
} else if !has {
380380
return ErrLabelNotExist{}
@@ -415,7 +415,7 @@ func (issue *Issue) ClearLabels(doer *User) (err error) {
415415
return err
416416
}
417417

418-
if has, err := hasAccess(sess, doer, issue.Repo, AccessModeWrite); err != nil {
418+
if has, err := hasAccess(sess, doer.ID, issue.Repo, AccessModeWrite); err != nil {
419419
return err
420420
} else if !has {
421421
return ErrLabelNotExist{}
@@ -809,23 +809,14 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) {
809809
}
810810
}
811811

812-
if opts.Issue.AssigneeID > 0 {
813-
assignee, err := getUserByID(e, opts.Issue.AssigneeID)
814-
if err != nil && !IsErrUserNotExist(err) {
815-
return fmt.Errorf("getUserByID: %v", err)
812+
if assigneeID := opts.Issue.AssigneeID; assigneeID > 0 {
813+
valid, err := hasAccess(e, assigneeID, opts.Repo, AccessModeWrite)
814+
if err != nil {
815+
return fmt.Errorf("hasAccess [user_id: %d, repo_id: %d]: %v", assigneeID, opts.Repo.ID, err)
816816
}
817-
818-
// Assume assignee is invalid and drop silently.
819-
opts.Issue.AssigneeID = 0
820-
if assignee != nil {
821-
valid, err := hasAccess(e, assignee, opts.Repo, AccessModeWrite)
822-
if err != nil {
823-
return fmt.Errorf("hasAccess [user_id: %d, repo_id: %d]: %v", assignee.ID, opts.Repo.ID, err)
824-
}
825-
if valid {
826-
opts.Issue.AssigneeID = assignee.ID
827-
opts.Issue.Assignee = assignee
828-
}
817+
if !valid {
818+
opts.Issue.AssigneeID = 0
819+
opts.Issue.Assignee = nil
829820
}
830821
}
831822

‎models/org_team.go

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -139,18 +139,19 @@ func (t *Team) removeRepository(e Engine, repo *Repository, recalculate bool) (e
139139
}
140140
}
141141

142-
if err = t.getMembers(e); err != nil {
143-
return fmt.Errorf("get team members: %v", err)
142+
teamUsers, err := getTeamUsersByTeamID(e, t.ID)
143+
if err != nil {
144+
return fmt.Errorf("getTeamUsersByTeamID: %v", err)
144145
}
145-
for _, u := range t.Members {
146-
has, err := hasAccess(e, u, repo, AccessModeRead)
146+
for _, teamUser:= range teamUsers {
147+
has, err := hasAccess(e, teamUser.UID, repo, AccessModeRead)
147148
if err != nil {
148149
return err
149150
} else if has {
150151
continue
151152
}
152153

153-
if err = watchRepo(e, u.ID, repo.ID, false); err != nil {
154+
if err = watchRepo(e, teamUser.UID, repo.ID, false); err != nil {
154155
return err
155156
}
156157
}
@@ -399,20 +400,25 @@ func IsTeamMember(orgID, teamID, userID int64) bool {
399400
return isTeamMember(x, orgID, teamID, userID)
400401
}
401402

402-
func getTeamMembers(e Engine, teamID int64) (_ []*User, err error) {
403+
func getTeamUsersByTeamID(e Engine, teamID int64) ([]*TeamUser, error) {
403404
teamUsers := make([]*TeamUser, 0, 10)
404-
if err = e.
405+
return teamUsers, e.
405406
Where("team_id=?", teamID).
406-
Find(&teamUsers); err != nil {
407+
Find(&teamUsers)
408+
}
409+
410+
func getTeamMembers(e Engine, teamID int64) (_ []*User, err error) {
411+
teamUsers, err := getTeamUsersByTeamID(e, teamID)
412+
if err != nil {
407413
return nil, fmt.Errorf("get team-users: %v", err)
408414
}
409-
members := make([]*User, 0, len(teamUsers))
410-
for i := range teamUsers {
411-
member := new(User)
412-
if _, err = e.Id(teamUsers[i].UID).Get(member); err != nil {
413-
return nil, fmt.Errorf("get user '%d': %v", teamUsers[i].UID, err)
415+
members := make([]*User, len(teamUsers))
416+
for i, teamUser := range teamUsers {
417+
member, err := getUserByID(e, teamUser.UID)
418+
if err != nil {
419+
return nil, fmt.Errorf("get user '%d': %v", teamUser.UID, err)
414420
}
415-
members = append(members, member)
421+
members[i] = member
416422
}
417423
return members, nil
418424
}

‎models/org_team_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,7 @@ func TestDeleteTeam(t *testing.T) {
243243
// check that team members don't have "leftover" access to repos
244244
user := AssertExistsAndLoadBean(t, &User{ID: 4}).(*User)
245245
repo := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository)
246-
accessMode, err := AccessLevel(user, repo)
246+
accessMode, err := AccessLevel(user.ID, repo)
247247
assert.NoError(t, err)
248248
assert.True(t, accessMode < AccessModeWrite)
249249
}

‎models/release.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -365,7 +365,7 @@ func DeleteReleaseByID(id int64, u *User, delTag bool) error {
365365
return fmt.Errorf("GetRepositoryByID: %v", err)
366366
}
367367

368-
has, err := HasAccess(u, repo, AccessModeWrite)
368+
has, err := HasAccess(u.ID, repo, AccessModeWrite)
369369
if err != nil {
370370
return fmt.Errorf("HasAccess: %v", err)
371371
} else if !has {

‎models/repo.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -531,7 +531,7 @@ func (repo *Repository) ComposeCompareURL(oldCommitID, newCommitID string) strin
531531

532532
// HasAccess returns true when user has access to this repository
533533
func (repo *Repository) HasAccess(u *User) bool {
534-
has, _ := HasAccess(u, repo, AccessModeRead)
534+
has, _ := HasAccess(u.ID, repo, AccessModeRead)
535535
return has
536536
}
537537

‎models/ssh_key.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -794,7 +794,7 @@ func DeleteDeployKey(doer *User, id int64) error {
794794
if err != nil {
795795
return fmt.Errorf("GetRepositoryByID: %v", err)
796796
}
797-
yes, err := HasAccess(doer, repo, AccessModeAdmin)
797+
yes, err := HasAccess(doer.ID, repo, AccessModeAdmin)
798798
if err != nil {
799799
return fmt.Errorf("HasAccess: %v", err)
800800
} else if !yes {

‎models/user.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -478,7 +478,7 @@ func (u *User) DeleteAvatar() error {
478478

479479
// IsAdminOfRepo returns true if user has admin or higher access of repository.
480480
func (u *User) IsAdminOfRepo(repo *Repository) bool {
481-
has, err := HasAccess(u, repo, AccessModeAdmin)
481+
has, err := HasAccess(u.ID, repo, AccessModeAdmin)
482482
if err != nil {
483483
log.Error(3, "HasAccess: %v", err)
484484
}
@@ -487,7 +487,7 @@ func (u *User) IsAdminOfRepo(repo *Repository) bool {
487487

488488
// IsWriterOfRepo returns true if user has write access to given repository.
489489
func (u *User) IsWriterOfRepo(repo *Repository) bool {
490-
has, err := HasAccess(u, repo, AccessModeWrite)
490+
has, err := HasAccess(u.ID, repo, AccessModeWrite)
491491
if err != nil {
492492
log.Error(3, "HasAccess: %v", err)
493493
}
@@ -1103,7 +1103,7 @@ func GetUserByID(id int64) (*User, error) {
11031103

11041104
// GetAssigneeByID returns the user with write access of repository by given ID.
11051105
func GetAssigneeByID(repo *Repository, userID int64) (*User, error) {
1106-
has, err := HasAccess(&User{ID: userID}, repo, AccessModeWrite)
1106+
has, err := HasAccess(userID, repo, AccessModeWrite)
11071107
if err != nil {
11081108
return nil, err
11091109
} else if !has {

‎modules/context/repo.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ func RepoAssignment(args ...bool) macaron.Handler {
219219
if ctx.IsSigned && ctx.User.IsAdmin {
220220
ctx.Repo.AccessMode = models.AccessModeOwner
221221
} else {
222-
mode, err := models.AccessLevel(ctx.User, repo)
222+
mode, err := models.AccessLevel(ctx.User.ID, repo)
223223
if err != nil {
224224
ctx.Handle(500, "AccessLevel", err)
225225
return

‎modules/lfs/server.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -463,7 +463,7 @@ func authenticate(ctx *context.Context, repository *models.Repository, authoriza
463463
}
464464

465465
if ctx.IsSigned {
466-
accessCheck, _ := models.HasAccess(ctx.User, repository, accessMode)
466+
accessCheck, _ := models.HasAccess(ctx.User.ID, repository, accessMode)
467467
return accessCheck
468468
}
469469

@@ -499,7 +499,7 @@ func authenticate(ctx *context.Context, repository *models.Repository, authoriza
499499
return false
500500
}
501501

502-
accessCheck, _ := models.HasAccess(userModel, repository, accessMode)
502+
accessCheck, _ := models.HasAccess(userModel.ID, repository, accessMode)
503503
return accessCheck
504504
}
505505

‎routers/api/v1/api.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ func repoAssignment() macaron.Handler {
7070
if ctx.IsSigned && ctx.User.IsAdmin {
7171
ctx.Repo.AccessMode = models.AccessModeOwner
7272
} else {
73-
mode, err := models.AccessLevel(ctx.User, repo)
73+
mode, err := models.AccessLevel(ctx.User.ID, repo)
7474
if err != nil {
7575
ctx.Error(500, "AccessLevel", err)
7676
return

‎routers/api/v1/org/team.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ func GetTeamRepos(ctx *context.APIContext) {
131131
}
132132
repos := make([]*api.Repository, len(team.Repos))
133133
for i, repo := range team.Repos {
134-
access, err := models.AccessLevel(ctx.User, repo)
134+
access, err := models.AccessLevel(ctx.User.ID, repo)
135135
if err != nil {
136136
ctx.Error(500, "GetTeamRepos", err)
137137
return
@@ -161,7 +161,7 @@ func AddTeamRepository(ctx *context.APIContext) {
161161
if ctx.Written() {
162162
return
163163
}
164-
if access, err := models.AccessLevel(ctx.User, repo); err != nil {
164+
if access, err := models.AccessLevel(ctx.User.ID, repo); err != nil {
165165
ctx.Error(500, "AccessLevel", err)
166166
return
167167
} else if access < models.AccessModeAdmin {
@@ -181,7 +181,7 @@ func RemoveTeamRepository(ctx *context.APIContext) {
181181
if ctx.Written() {
182182
return
183183
}
184-
if access, err := models.AccessLevel(ctx.User, repo); err != nil {
184+
if access, err := models.AccessLevel(ctx.User.ID, repo); err != nil {
185185
ctx.Error(500, "AccessLevel", err)
186186
return
187187
} else if access < models.AccessModeAdmin {

‎routers/api/v1/repo/fork.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ func ListForks(ctx *context.APIContext) {
2020
}
2121
apiForks := make([]*api.Repository, len(forks))
2222
for i, fork := range forks {
23-
access, err := models.AccessLevel(ctx.User, fork)
23+
access, err := models.AccessLevel(ctx.User.ID, fork)
2424
if err != nil {
2525
ctx.Error(500, "AccessLevel", err)
2626
return

‎routers/api/v1/repo/release.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ func ListReleases(ctx *context.APIContext) {
4040
return
4141
}
4242
rels := make([]*api.Release, len(releases))
43-
access, err := models.AccessLevel(ctx.User, ctx.Repo.Repository)
43+
access, err := models.AccessLevel(ctx.User.ID, ctx.Repo.Repository)
4444
if err != nil {
4545
ctx.Error(500, "AccessLevel", err)
4646
return

‎routers/api/v1/repo/repo.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ func Search(ctx *context.APIContext) {
6464
})
6565
return
6666
}
67-
accessMode, err := models.AccessLevel(ctx.User, repo)
67+
accessMode, err := models.AccessLevel(ctx.User.ID, repo)
6868
if err != nil {
6969
ctx.JSON(500, map[string]interface{}{
7070
"ok": false,
@@ -218,7 +218,7 @@ func Migrate(ctx *context.APIContext, form auth.MigrateRepoForm) {
218218
// see https://github.com/gogits/go-gogs-client/wiki/Repositories#get
219219
func Get(ctx *context.APIContext) {
220220
repo := ctx.Repo.Repository
221-
access, err := models.AccessLevel(ctx.User, repo)
221+
access, err := models.AccessLevel(ctx.User.ID, repo)
222222
if err != nil {
223223
ctx.Error(500, "GetRepository", err)
224224
return
@@ -238,7 +238,7 @@ func GetByID(ctx *context.APIContext) {
238238
return
239239
}
240240

241-
access, err := models.AccessLevel(ctx.User, repo)
241+
access, err := models.AccessLevel(ctx.User.ID, repo)
242242
if err != nil {
243243
ctx.Error(500, "GetRepositoryByID", err)
244244
return

0 commit comments

Comments
 (0)