Skip to content

Commit eabbddc

Browse files
lunnylunny
authored
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
1 parent 0222623 commit eabbddc

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

80 files changed

+1351
-765
lines changed

‎cmd/serv.go

+2-12
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ func runServ(c *cli.Context) error {
193193
keyID int64
194194
user *models.User
195195
)
196-
if requestedMode == models.AccessModeWrite || repo.IsPrivate {
196+
if requestedMode == models.AccessModeWrite || repo.IsPrivate || setting.Service.RequireSignInView {
197197
keys := strings.Split(c.Args()[0], "-")
198198
if len(keys) != 2 {
199199
fail("Key ID format error", "Invalid key argument: %s", c.Args()[0])
@@ -236,7 +236,7 @@ func runServ(c *cli.Context) error {
236236
user.Name, repoPath)
237237
}
238238

239-
mode, err := private.AccessLevel(user.ID, repo.ID)
239+
mode, err := private.CheckUnitUser(user.ID, repo.ID, user.IsAdmin, unitType)
240240
if err != nil {
241241
fail("Internal error", "Failed to check access: %v", err)
242242
} else if *mode < requestedMode {
@@ -249,16 +249,6 @@ func runServ(c *cli.Context) error {
249249
user.Name, requestedMode, repoPath)
250250
}
251251

252-
check, err := private.CheckUnitUser(user.ID, repo.ID, user.IsAdmin, unitType)
253-
if err != nil {
254-
fail("You do not have allowed for this action", "Failed to access internal api: [user.Name: %s, repoPath: %s]", user.Name, repoPath)
255-
}
256-
if !check {
257-
fail("You do not have allowed for this action",
258-
"User %s does not have allowed access to repository %s 's code",
259-
user.Name, repoPath)
260-
}
261-
262252
os.Setenv(models.EnvPusherName, user.Name)
263253
os.Setenv(models.EnvPusherID, fmt.Sprintf("%d", user.ID))
264254
}

‎integrations/api_repo_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ func TestAPISearchRepo(t *testing.T) {
164164
assert.Len(t, body.Data, expected.count)
165165
for _, repo := range body.Data {
166166
r := getRepo(t, repo.ID)
167-
hasAccess, err := models.HasAccess(userID, r, models.AccessModeRead)
167+
hasAccess, err := models.HasAccess(userID, r)
168168
assert.NoError(t, err)
169169
assert.True(t, hasAccess)
170170

‎models/access.go

-16
Original file line numberDiff line numberDiff line change
@@ -80,22 +80,6 @@ func accessLevel(e Engine, userID int64, repo *Repository) (AccessMode, error) {
8080
return a.Mode, nil
8181
}
8282

83-
// AccessLevel returns the Access a user has to a repository. Will return NoneAccess if the
84-
// user does not have access.
85-
func AccessLevel(userID int64, repo *Repository) (AccessMode, error) {
86-
return accessLevel(x, userID, repo)
87-
}
88-
89-
func hasAccess(e Engine, userID int64, repo *Repository, testMode AccessMode) (bool, error) {
90-
mode, err := accessLevel(e, userID, repo)
91-
return testMode <= mode, err
92-
}
93-
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)
97-
}
98-
9983
type repoAccess struct {
10084
Access `xorm:"extends"`
10185
Repository `xorm:"extends"`

‎models/access_test.go

+18-23
Original file line numberDiff line numberDiff line change
@@ -20,28 +20,28 @@ var accessModes = []AccessMode{
2020
func TestAccessLevel(t *testing.T) {
2121
assert.NoError(t, PrepareTestDatabase())
2222

23-
user1 := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
24-
user2 := AssertExistsAndLoadBean(t, &User{ID: 5}).(*User)
23+
user2 := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
24+
user5 := AssertExistsAndLoadBean(t, &User{ID: 5}).(*User)
2525
// A public repository owned by User 2
2626
repo1 := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
2727
assert.False(t, repo1.IsPrivate)
2828
// A private repository owned by Org 3
29-
repo2 := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository)
30-
assert.True(t, repo2.IsPrivate)
29+
repo3 := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository)
30+
assert.True(t, repo3.IsPrivate)
3131

32-
level, err := AccessLevel(user1.ID, repo1)
32+
level, err := AccessLevel(user2, repo1)
3333
assert.NoError(t, err)
3434
assert.Equal(t, AccessModeOwner, level)
3535

36-
level, err = AccessLevel(user1.ID, repo2)
36+
level, err = AccessLevel(user2, repo3)
3737
assert.NoError(t, err)
38-
assert.Equal(t, AccessModeWrite, level)
38+
assert.Equal(t, AccessModeOwner, level)
3939

40-
level, err = AccessLevel(user2.ID, repo1)
40+
level, err = AccessLevel(user5, repo1)
4141
assert.NoError(t, err)
4242
assert.Equal(t, AccessModeRead, level)
4343

44-
level, err = AccessLevel(user2.ID, repo2)
44+
level, err = AccessLevel(user5, repo3)
4545
assert.NoError(t, err)
4646
assert.Equal(t, AccessModeNone, level)
4747
}
@@ -58,23 +58,18 @@ func TestHasAccess(t *testing.T) {
5858
repo2 := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository)
5959
assert.True(t, repo2.IsPrivate)
6060

61-
for _, accessMode := range accessModes {
62-
has, err := HasAccess(user1.ID, repo1, accessMode)
63-
assert.NoError(t, err)
64-
assert.True(t, has)
61+
has, err := HasAccess(user1.ID, repo1)
62+
assert.NoError(t, err)
63+
assert.True(t, has)
6564

66-
has, err = HasAccess(user1.ID, repo2, accessMode)
67-
assert.NoError(t, err)
68-
assert.Equal(t, accessMode <= AccessModeWrite, has)
65+
has, err = HasAccess(user1.ID, repo2)
66+
assert.NoError(t, err)
6967

70-
has, err = HasAccess(user2.ID, repo1, accessMode)
71-
assert.NoError(t, err)
72-
assert.Equal(t, accessMode <= AccessModeRead, has)
68+
has, err = HasAccess(user2.ID, repo1)
69+
assert.NoError(t, err)
7370

74-
has, err = HasAccess(user2.ID, repo2, accessMode)
75-
assert.NoError(t, err)
76-
assert.Equal(t, accessMode <= AccessModeNone, has)
77-
}
71+
has, err = HasAccess(user2.ID, repo2)
72+
assert.NoError(t, err)
7873
}
7974

8075
func TestUser_GetRepositoryAccesses(t *testing.T) {

‎models/branches.go

+9-3
Original file line numberDiff line numberDiff line change
@@ -243,10 +243,16 @@ func updateUserWhitelist(repo *Repository, currentWhitelist, newWhitelist []int6
243243

244244
whitelist = make([]int64, 0, len(newWhitelist))
245245
for _, userID := range newWhitelist {
246-
has, err := hasAccess(x, userID, repo, AccessModeWrite)
246+
user, err := GetUserByID(userID)
247247
if err != nil {
248-
return nil, fmt.Errorf("HasAccess [user_id: %d, repo_id: %d]: %v", userID, repo.ID, err)
249-
} else if !has {
248+
return nil, fmt.Errorf("GetUserByID [user_id: %d, repo_id: %d]: %v", userID, repo.ID, err)
249+
}
250+
perm, err := GetUserRepoPermission(repo, user)
251+
if err != nil {
252+
return nil, fmt.Errorf("GetUserRepoPermission [user_id: %d, repo_id: %d]: %v", userID, repo.ID, err)
253+
}
254+
255+
if !perm.CanWrite(UnitTypeCode) {
250256
continue // Drop invalid user ID
251257
}
252258

‎models/fixtures/repo_unit.yml

+113-1
Original file line numberDiff line numberDiff line change
@@ -108,4 +108,116 @@
108108
repo_id: 33
109109
type: 5
110110
config: "{}"
111-
created_unix: 1535593231
111+
created_unix: 1535593231
112+
113+
-
114+
id: 17
115+
repo_id: 4
116+
type: 4
117+
config: "{}"
118+
created_unix: 946684810
119+
120+
-
121+
id: 18
122+
repo_id: 4
123+
type: 5
124+
config: "{}"
125+
created_unix: 946684810
126+
127+
-
128+
id: 19
129+
repo_id: 4
130+
type: 1
131+
config: "{}"
132+
created_unix: 946684810
133+
134+
-
135+
id: 20
136+
repo_id: 4
137+
type: 2
138+
config: "{\"EnableTimetracker\":true,\"AllowOnlyContributorsToTrackTime\":true}"
139+
created_unix: 946684810
140+
141+
-
142+
id: 21
143+
repo_id: 4
144+
type: 3
145+
config: "{\"IgnoreWhitespaceConflicts\":false,\"AllowMerge\":true,\"AllowRebase\":true,\"AllowSquash\":true}"
146+
created_unix: 946684810
147+
148+
-
149+
id: 22
150+
repo_id: 2
151+
type: 4
152+
config: "{}"
153+
created_unix: 946684810
154+
155+
-
156+
id: 23
157+
repo_id: 2
158+
type: 5
159+
config: "{}"
160+
created_unix: 946684810
161+
162+
-
163+
id: 24
164+
repo_id: 2
165+
type: 1
166+
config: "{}"
167+
created_unix: 946684810
168+
169+
-
170+
id: 25
171+
repo_id: 32
172+
type: 1
173+
config: "{}"
174+
created_unix: 1524304355
175+
176+
-
177+
id: 26
178+
repo_id: 32
179+
type: 2
180+
config: "{}"
181+
created_unix: 1524304355
182+
183+
-
184+
id: 27
185+
repo_id: 24
186+
type: 1
187+
config: "{}"
188+
created_unix: 1524304355
189+
190+
-
191+
id: 28
192+
repo_id: 24
193+
type: 2
194+
config: "{}"
195+
created_unix: 1524304355
196+
197+
-
198+
id: 29
199+
repo_id: 16
200+
type: 1
201+
config: "{}"
202+
created_unix: 1524304355
203+
204+
-
205+
id: 30
206+
repo_id: 23
207+
type: 1
208+
config: "{}"
209+
created_unix: 1524304355
210+
211+
-
212+
id: 31
213+
repo_id: 27
214+
type: 1
215+
config: "{}"
216+
created_unix: 1524304355
217+
218+
-
219+
id: 32
220+
repo_id: 28
221+
type: 1
222+
config: "{}"
223+
created_unix: 1524304355

‎models/fixtures/repository.yml

+1-1
Original file line numberDiff line numberDiff line change
@@ -391,7 +391,7 @@
391391
is_mirror: false
392392

393393
-
394-
id: 32
394+
id: 32 # org public repo
395395
owner_id: 3
396396
lower_name: repo21
397397
name: repo21

‎models/fixtures/team.yml

+27
Original file line numberDiff line numberDiff line change
@@ -51,3 +51,30 @@
5151
authorize: 4 # owner
5252
num_repos: 2
5353
num_members: 1
54+
55+
-
56+
id: 7
57+
org_id: 3
58+
lower_name: test_team
59+
name: test_team
60+
authorize: 2 # write
61+
num_repos: 1
62+
num_members: 1
63+
64+
-
65+
id: 8
66+
org_id: 17
67+
lower_name: test_team
68+
name: test_team
69+
authorize: 2 # write
70+
num_repos: 1
71+
num_members: 1
72+
73+
-
74+
id: 9
75+
org_id: 17
76+
lower_name: review_team
77+
name: review_team
78+
authorize: 1 # read
79+
num_repos: 1
80+
num_members: 1

‎models/fixtures/team_repo.yml

+18
Original file line numberDiff line numberDiff line change
@@ -45,3 +45,21 @@
4545
org_id: 3
4646
team_id: 1
4747
repo_id: 32
48+
49+
-
50+
id: 9
51+
org_id: 3
52+
team_id: 7
53+
repo_id: 32
54+
55+
-
56+
id: 10
57+
org_id: 17
58+
team_id: 8
59+
repo_id: 24
60+
61+
-
62+
id: 11
63+
org_id: 17
64+
team_id: 9
65+
repo_id: 24

‎models/fixtures/team_unit.yml

+15
Original file line numberDiff line numberDiff line change
@@ -207,3 +207,18 @@
207207
id: 42
208208
team_id: 6
209209
type: 7
210+
211+
-
212+
id: 43
213+
team_id: 7
214+
type: 2 # issues
215+
216+
-
217+
id: 44
218+
team_id: 8
219+
type: 2 # issues
220+
221+
-
222+
id: 45
223+
team_id: 9
224+
type: 1 # code

‎models/fixtures/team_user.yml

+18
Original file line numberDiff line numberDiff line change
@@ -44,4 +44,22 @@
4444
id: 8
4545
org_id: 19
4646
team_id: 6
47+
uid: 20
48+
49+
-
50+
id: 9
51+
org_id: 3
52+
team_id: 7
53+
uid: 15
54+
55+
-
56+
id: 10
57+
org_id: 17
58+
team_id: 8
59+
uid: 2
60+
61+
-
62+
id: 11
63+
org_id: 17
64+
team_id: 9
4765
uid: 20

0 commit comments

Comments
 (0)