Restrict permission check on repositories and fix some problems (#5314)

* fix units permission problems * fix some bugs and merge LoadUnits to repoAssignment * refactor permission struct and add some copyright heads * remove unused codes * fix routes units check * improve permission check * add unit tests for permission * fix typo * fix tests * fix some routes * fix api permission check * improve permission check * fix some permission check * fix tests * fix tests * improve some permission check * fix some permission check * refactor AccessLevel * fix bug * fix tests * fix tests * fix tests * fix AccessLevel * rename CanAccess * fix tests * fix comment * fix bug * add missing unit for test repos * fix bug * rename some functions * fix routes check
go-gitea · Nov 28, 2018 · eabbddc · eabbddc
1 parent 0222623
commit eabbddc
Show file tree

Hide file tree

Showing 80 changed files with 1,351 additions and 765 deletions.
diff --git a/cmd/serv.go b/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))
 		}

diff --git a/integrations/api_repo_test.go b/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)
 

diff --git a/models/access.go b/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"`

diff --git a/models/access_test.go b/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) {

diff --git a/models/branches.go b/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
 		}
 

diff --git a/models/fixtures/repo_unit.yml b/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
diff --git a/models/fixtures/repository.yml b/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

diff --git a/models/fixtures/team.yml b/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
diff --git a/models/fixtures/team_repo.yml b/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
diff --git a/models/fixtures/team_unit.yml b/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
diff --git a/models/fixtures/team_user.yml b/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