Restrict permission check on repositories and fix some problems

cmd/serv.go

@@ -193,7 +193,7 @@ func runServ(c *cli.Context) error {
  keyID int64
  user *models.User
  )
- if requestedMode == models.AccessModeWrite || repo.IsPrivate {
+ if requestedMode == models.AccessModeWrite || repo.IsPrivate || setting.Service.RequireSignInView {
  keys := strings.Split(c.Args()[0], "-")
  if len(keys) != 2 {
  fail("Key ID format error", "Invalid key argument: %s", c.Args()[0])
@@ -236,7 +236,7 @@ func runServ(c *cli.Context) error {
  user.Name, repoPath)
  }
 
- mode, err := private.AccessLevel(user.ID, repo.ID)
+ mode, err := private.CheckUnitUser(user.ID, repo.ID, user.IsAdmin, unitType)
  if err != nil {
  fail("Internal error", "Failed to check access: %v", err)
  } else if *mode < requestedMode {
@@ -249,16 +249,6 @@ func runServ(c *cli.Context) error {
  user.Name, requestedMode, repoPath)
  }
 
- check, err := private.CheckUnitUser(user.ID, repo.ID, user.IsAdmin, unitType)
- if err != nil {
- fail("You do not have allowed for this action", "Failed to access internal api: [user.Name: %s, repoPath: %s]", user.Name, repoPath)
- }
- if !check {
- fail("You do not have allowed for this action",
- "User %s does not have allowed access to repository %s 's code",
- user.Name, repoPath)
- }
-
  os.Setenv(models.EnvPusherName, user.Name)
  os.Setenv(models.EnvPusherID, fmt.Sprintf("%d", user.ID))
  }

integrations/api_repo_test.go

@@ -164,7 +164,7 @@ func TestAPISearchRepo(t *testing.T) {
  assert.Len(t, body.Data, expected.count)
  for _, repo := range body.Data {
  r := getRepo(t, repo.ID)
- hasAccess, err := models.HasAccess(userID, r, models.AccessModeRead)
+ hasAccess, err := models.HasAccess(userID, r)
  assert.NoError(t, err)
  assert.True(t, hasAccess)
 

models/access.go

@@ -80,22 +80,6 @@ func accessLevel(e Engine, userID int64, repo *Repository) (AccessMode, error) {
  return a.Mode, nil
 }
 
-// AccessLevel returns the Access a user has to a repository. Will return NoneAccess if the
-// user does not have access.
-func AccessLevel(userID int64, repo *Repository) (AccessMode, error) {
- return accessLevel(x, userID, repo)
-}
-
-func hasAccess(e Engine, userID int64, repo *Repository, testMode AccessMode) (bool, error) {
- mode, err := accessLevel(e, userID, repo)
- return testMode <= mode, err
-}
-
-// HasAccess returns true if user has access to repo
-func HasAccess(userID int64, repo *Repository, testMode AccessMode) (bool, error) {
- return hasAccess(x, userID, repo, testMode)
-}
-
 type repoAccess struct {
  Access `xorm:"extends"`
  Repository `xorm:"extends"`

models/access_test.go

@@ -20,28 +20,28 @@ var accessModes = []AccessMode{
 func TestAccessLevel(t *testing.T) {
  assert.NoError(t, PrepareTestDatabase())
 
- user1 := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
- user2 := AssertExistsAndLoadBean(t, &User{ID: 5}).(*User)
+ user2 := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
+ user5 := AssertExistsAndLoadBean(t, &User{ID: 5}).(*User)
  // A public repository owned by User 2
  repo1 := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
  assert.False(t, repo1.IsPrivate)
  // A private repository owned by Org 3
- repo2 := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository)
- assert.True(t, repo2.IsPrivate)
+ repo3 := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository)
+ assert.True(t, repo3.IsPrivate)
 
- level, err := AccessLevel(user1.ID, repo1)
+ level, err := AccessLevel(user2, repo1)
  assert.NoError(t, err)
  assert.Equal(t, AccessModeOwner, level)
 
- level, err = AccessLevel(user1.ID, repo2)
+ level, err = AccessLevel(user2, repo3)
  assert.NoError(t, err)
- assert.Equal(t, AccessModeWrite, level)
+ assert.Equal(t, AccessModeOwner, level)
 
- level, err = AccessLevel(user2.ID, repo1)
+ level, err = AccessLevel(user5, repo1)
  assert.NoError(t, err)
  assert.Equal(t, AccessModeRead, level)
 
- level, err = AccessLevel(user2.ID, repo2)
+ level, err = AccessLevel(user5, repo3)
  assert.NoError(t, err)
  assert.Equal(t, AccessModeNone, level)
 }
@@ -58,23 +58,18 @@ func TestHasAccess(t *testing.T) {
  repo2 := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository)
  assert.True(t, repo2.IsPrivate)
 
- for _, accessMode := range accessModes {
- has, err := HasAccess(user1.ID, repo1, accessMode)
- assert.NoError(t, err)
- assert.True(t, has)
+ has, err := HasAccess(user1.ID, repo1)
+ assert.NoError(t, err)
+ assert.True(t, has)
 
- has, err = HasAccess(user1.ID, repo2, accessMode)
- assert.NoError(t, err)
- assert.Equal(t, accessMode <= AccessModeWrite, has)
+ has, err = HasAccess(user1.ID, repo2)
+ assert.NoError(t, err)
 
- has, err = HasAccess(user2.ID, repo1, accessMode)
- assert.NoError(t, err)
- assert.Equal(t, accessMode <= AccessModeRead, has)
+ has, err = HasAccess(user2.ID, repo1)
+ assert.NoError(t, err)
 
- has, err = HasAccess(user2.ID, repo2, accessMode)
- assert.NoError(t, err)
- assert.Equal(t, accessMode <= AccessModeNone, has)
- }
+ has, err = HasAccess(user2.ID, repo2)
+ assert.NoError(t, err)
 }
 
 func TestUser_GetRepositoryAccesses(t *testing.T) {

models/branches.go

@@ -243,10 +243,16 @@ func updateUserWhitelist(repo *Repository, currentWhitelist, newWhitelist []int6
 
  whitelist = make([]int64, 0, len(newWhitelist))
  for _, userID := range newWhitelist {
- has, err := hasAccess(x, userID, repo, AccessModeWrite)
+ user, err := GetUserByID(userID)
  if err != nil {
- return nil, fmt.Errorf("HasAccess [user_id: %d, repo_id: %d]: %v", userID, repo.ID, err)
- } else if !has {
+ return nil, fmt.Errorf("GetUserByID [user_id: %d, repo_id: %d]: %v", userID, repo.ID, err)
+ }
+ perm, err := GetUserRepoPermission(repo, user)
+ if err != nil {
+ return nil, fmt.Errorf("GetUserRepoPermission [user_id: %d, repo_id: %d]: %v", userID, repo.ID, err)
+ }
+
+ if !perm.CanWrite(UnitTypeCode) {
  continue // Drop invalid user ID
  }
 

models/fixtures/repo_unit.yml

@@ -108,4 +108,116 @@
  repo_id: 33
  type: 5
  config: "{}"
- created_unix: 1535593231
+ created_unix: 1535593231
+
+-
+ id: 17
+ repo_id: 4
+ type: 4
+ config: "{}"
+ created_unix: 946684810
+
+-
+ id: 18
+ repo_id: 4
+ type: 5
+ config: "{}"
+ created_unix: 946684810
+
+-
+ id: 19
+ repo_id: 4
+ type: 1
+ config: "{}"
+ created_unix: 946684810
+
+-
+ id: 20
+ repo_id: 4
+ type: 2
+ config: "{\"EnableTimetracker\":true,\"AllowOnlyContributorsToTrackTime\":true}"
+ created_unix: 946684810
+
+-
+ id: 21
+ repo_id: 4
+ type: 3
+ config: "{\"IgnoreWhitespaceConflicts\":false,\"AllowMerge\":true,\"AllowRebase\":true,\"AllowSquash\":true}"
+ created_unix: 946684810
+
+-
+ id: 22
+ repo_id: 2
+ type: 4
+ config: "{}"
+ created_unix: 946684810
+
+-
+ id: 23
+ repo_id: 2
+ type: 5
+ config: "{}"
+ created_unix: 946684810
+
+-
+ id: 24
+ repo_id: 2
+ type: 1
+ config: "{}"
+ created_unix: 946684810
+
+-
+ id: 25
+ repo_id: 32
+ type: 1
+ config: "{}"
+ created_unix: 1524304355
+
+-
+ id: 26
+ repo_id: 32
+ type: 2
+ config: "{}"
+ created_unix: 1524304355
+
+-
+ id: 27
+ repo_id: 24
+ type: 1
+ config: "{}"
+ created_unix: 1524304355
+
+-
+ id: 28
+ repo_id: 24
+ type: 2
+ config: "{}"
+ created_unix: 1524304355
+
+-
+ id: 29
+ repo_id: 16
+ type: 1
+ config: "{}"
+ created_unix: 1524304355
+
+-
+ id: 30
+ repo_id: 23
+ type: 1
+ config: "{}"
+ created_unix: 1524304355
+
+-
+ id: 31
+ repo_id: 27
+ type: 1
+ config: "{}"
+ created_unix: 1524304355
+
+-
+ id: 32
+ repo_id: 28
+ type: 1
+ config: "{}"
+ created_unix: 1524304355

models/fixtures/repository.yml

@@ -391,7 +391,7 @@
  is_mirror: false
 
 -
- id: 32
+ id: 32 # org public repo
  owner_id: 3
  lower_name: repo21
  name: repo21

models/fixtures/team.yml

@@ -51,3 +51,30 @@
  authorize: 4 # owner
  num_repos: 2
  num_members: 1
+
+-
+ id: 7
+ org_id: 3
+ lower_name: test_team
+ name: test_team
+ authorize: 2 # write
+ num_repos: 1
+ num_members: 1
+
+-
+ id: 8
+ org_id: 17
+ lower_name: test_team
+ name: test_team
+ authorize: 2 # write
+ num_repos: 1
+ num_members: 1
+
+-
+ id: 9
+ org_id: 17
+ lower_name: review_team
+ name: review_team
+ authorize: 1 # read
+ num_repos: 1
+ num_members: 1

models/fixtures/team_repo.yml

@@ -45,3 +45,21 @@
  org_id: 3
  team_id: 1
  repo_id: 32
+
+-
+ id: 9
+ org_id: 3
+ team_id: 7
+ repo_id: 32
+
+-
+ id: 10
+ org_id: 17
+ team_id: 8
+ repo_id: 24
+
+-
+ id: 11
+ org_id: 17
+ team_id: 9
+ repo_id: 24

models/fixtures/team_unit.yml

@@ -207,3 +207,18 @@
  id: 42
  team_id: 6
  type: 7
+
+-
+ id: 43
+ team_id: 7
+ type: 2 # issues
+
+-
+ id: 44
+ team_id: 8
+ type: 2 # issues
+
+-
+ id: 45
+ team_id: 9
+ type: 1 # code

models/fixtures/team_user.yml

@@ -44,4 +44,22 @@
  id: 8
  org_id: 19
  team_id: 6
+ uid: 20
+
+-
+ id: 9
+ org_id: 3
+ team_id: 7
+ uid: 15
+
+-
+ id: 10
+ org_id: 17
+ team_id: 8
+ uid: 2
+
+-
+ id: 11
+ org_id: 17
+ team_id: 9
  uid: 20