hide issues from org private repos w/o team assignment #4034

daviian · 2018-05-24T09:35:16Z

targets #4029

hides private organization repository issues from users that have no access to them.

jonasfranz · 2018-05-24T10:52:33Z

models/access_test.go

-	repo1 := AssertExistsAndLoadBean(t, &Repository{OwnerID: 2, IsPrivate: false}).(*Repository)
-	repo2 := AssertExistsAndLoadBean(t, &Repository{OwnerID: 3, IsPrivate: true}).(*Repository)
+	// A public repository owned by User 2
+	repo1 := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)


Why did you remove the IsPrivate: false part?

[13:16] lunny: JonasFranz because bool has default value false, Don’t know it means false or null
[13:16] lunny: Sess.Get has the same behavior
[13:16] JonasFranz: Okay, so we need to check it afterwards?

Since it is not supported to use bools in xorm, I propose to add an check if IsPrivate is set right:

assert.False(t, repo1.IsPrivate)

or

assert.True(t, repo1.IsPrivate)

I changed it such that it really loads the expected repository. The resulting repository is the same, just an assurance.
That's applicable to the other changes in access_test.go too.

But it would be good to check that the repository is really private/public a comment does not ensure that.

I can add it of course, just think that it's nothing that adds value since its predefined by the fixtures anyway.

jonasfranz · 2018-05-24T10:52:42Z

models/access_test.go

+	// A public repository owned by User 2
+	repo1 := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
+	// A private repository owned by Org 3
+	repo2 := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository)


Why did you remove the IsPrivate: true part?

jonasfranz · 2018-05-24T10:52:50Z

models/access_test.go

+	// A public repository owned by User 2
+	repo1 := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
+	// A private repository owned by Org 3
+	repo2 := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository)


Why did you remove the IsPrivate: true part?

jonasfranz · 2018-05-24T10:52:54Z

models/access_test.go

-	repo1 := AssertExistsAndLoadBean(t, &Repository{OwnerID: 2, IsPrivate: false}).(*Repository)
-	repo2 := AssertExistsAndLoadBean(t, &Repository{OwnerID: 3, IsPrivate: true}).(*Repository)
+	// A public repository owned by User 2
+	repo1 := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)


Why did you remove the IsPrivate: false part?

jonasfranz · 2018-05-24T10:54:39Z

models/user.go

@@ -557,7 +557,9 @@ func (u *User) GetOrgRepositoryIDs() ([]int64, error) {
 	var ids []int64
 	return ids, x.Table("repository").
 		Cols("repository.id").
-		Join("INNER", "team_user", "repository.owner_id = team_user.org_id AND team_user.uid = ?", u.ID).
+		Join("INNER", "team_user", "repository.owner_id = team_user.org_id").
+		Join("INNER", "team_repo", "(team_user.team_id = team_repo.team_id AND repository.id = team_repo.repo_id) OR repository.is_private = 0").


Check is_private = 0 first

jonasfranz · 2018-05-24T10:55:39Z

models/user_test.go

+	assert.NoError(t, err)
+	// User 5's team has no access to any repo
+	var emptyRepos []int64
+	assert.Equal(t, accessibleRepos, emptyRepos)


Please use assert.Len instead

jonasfranz · 2018-05-24T10:57:23Z

Drone ci is failing.

jonasfranz · 2018-05-24T10:59:38Z

models/user_test.go

+	accessibleRepos, err := user2.GetOrgRepositoryIDs()
+	assert.NoError(t, err)
+	// User 2's team has access to private repos 3, 5, repo 32 is a public repo of the organization
+	assert.Equal(t, accessibleRepos, []int64{3, 5, 32})


The second parameter should be the expected value and the third the actual value. (See https://godoc.org/github.com/stretchr/testify/assert#Equal)

jonasfranz · 2018-05-24T10:59:45Z

models/user_test.go

+	accessibleRepos, err = user4.GetOrgRepositoryIDs()
+	assert.NoError(t, err)
+	// User 4's team has access to private repo 3, repo 32 is a public repo of the organization
+	assert.Equal(t, accessibleRepos, []int64{3, 32})


The second parameter should be the expected value and the third the actual value. (See https://godoc.org/github.com/stretchr/testify/assert#Equal)

codecov-io · 2018-05-24T11:53:58Z

Codecov Report

Merging #4034 into master will increase coverage by 0.08%.
The diff coverage is 29.24%.

@@            Coverage Diff             @@
##           master    #4034      +/-   ##
==========================================
+ Coverage      20%   20.09%   +0.08%     
==========================================
  Files         153      153              
  Lines       30620    30660      +40     
==========================================
+ Hits         6127     6160      +33     
+ Misses      23565    23558       -7     
- Partials      928      942      +14

Impacted Files	Coverage Δ
models/repo.go	`18.4% <0%> (+0.61%)`	⬆️
models/models.go	`33.33% <100%> (+0.28%)`	⬆️
models/user.go	`22.33% <33.33%> (+0.73%)`	⬆️
routers/user/home.go	`24.92% <40%> (ø)`	⬆️
models/notification.go	`66.84% <42.85%> (-0.95%)`	⬇️
models/org_team.go	`47.22% <5%> (-3.01%)`	⬇️
models/repo_watch.go	`61.62% <50%> (-1.89%)`	⬇️
models/org.go	`68.18% <69.23%> (-0.04%)`	⬇️
modules/process/manager.go	`69.56% <0%> (-4.35%)`	⬇️
models/topic.go	`54.12% <0%> (-0.58%)`	⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46d19c4...f5a3465. Read the comment docs.

jonasfranz · 2018-05-24T12:03:26Z

models/user.go

@@ -557,7 +557,9 @@ func (u *User) GetOrgRepositoryIDs() ([]int64, error) {
 	var ids []int64
 	return ids, x.Table("repository").
 		Cols("repository.id").
-		Join("INNER", "team_user", "repository.owner_id = team_user.org_id AND team_user.uid = ?", u.ID).
+		Join("INNER", "team_user", "repository.owner_id = team_user.org_id").
+		Join("INNER", "team_repo", "(team_user.team_id = team_repo.team_id AND repository.id = team_repo.repo_id) OR repository.is_private != ?", true).


Please change it to:

repository.is_private = 0 OR (team_user.team_id = team_repo.team_id AND repository.id = team_repo.repo_id)

Problem with this is that it's not necessarily 0 if it is not private, because it could also be null
Also integration tests with pgsql fail on my pc

jonasfranz · 2018-05-24T16:33:17Z

models/user.go

@@ -558,7 +558,7 @@ func (u *User) GetOrgRepositoryIDs() ([]int64, error) {
 	return ids, x.Table("repository").
 		Cols("repository.id").
 		Join("INNER", "team_user", "repository.owner_id = team_user.org_id").
-		Join("INNER", "team_repo", "(team_user.team_id = team_repo.team_id AND repository.id = team_repo.repo_id) OR repository.is_private != ?", true).


Please remove , true since it is not used anymore

Actually it is used. It's not possible to write != 0 because it doesn't work on all databases. You will see that pgsql fails if we use that.

jonasfranz

This may be out of scope for this PR but if I create a team with permissions for Wiki only for example, the issues of the repository are shown in the issue list of the team members. But they shouldn't see them.

lunny · 2018-05-29T06:04:56Z

LGTM

lunny · 2018-05-29T08:25:18Z

As @JonasFranzDEV said, team unit permissions are also need to be checked.

daviian · 2018-06-02T17:10:28Z

@lunny @JonasFranzDEV Do you want me to make a more or less dirty fix by iterating over the selected repo ID's and check the unittype or do you favor a more solid solution by moving out the UnitType json into a new team_unit table?

lafriks · 2018-06-02T17:46:23Z

I would prefer team_unit table but than it can not be backported

daviian · 2018-06-02T18:28:28Z

@lafriks I could backport the quick'n'dirty solution anyway ;-)

lafriks · 2018-06-02T22:36:13Z

@daviian it is not so critical bug to be worth it imho

daviian · 2018-06-03T18:55:54Z

Can you mark this PR as WIP? There is one thing missing I've not implemented yet.

TODO

Migrate team unit types into new team_unit table
Filter Issues by team access and unit type
Don't notify users about certain actions that lack the required unit type.
~~Remove link to issue in actions on dashboard if user is not allowed to access~~

daviian · 2018-06-05T06:56:26Z

@lafriks There's one open issue though. What happens to existing actions on the dashboard that are linked to unallowed issues because of a change in the units afterwards?

jonasfranz

conflicted but otherwise LGTM

lafriks · 2018-06-20T10:16:13Z

@daviian please rebase and resolve conflicts

axifive · 2018-06-20T15:59:24Z

@daviian, Please use v69 migration name for conflict resolving. Because I already resolved #4258 with v68

daviian · 2018-06-20T20:24:03Z

@axifive Sry, didn't read your comment before rebasing.

axifive · 2018-06-21T15:54:04Z

@daviian, just wanted to warn against double rebase

lunny added the type/bug label May 24, 2018

lunny added this to the 1.5.0 milestone May 24, 2018

lunny added backport/v1.3 and removed backport/v1.3 labels May 24, 2018

jonasfranz suggested changes May 24, 2018

View reviewed changes

bkcsoft added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 24, 2018

jonasfranz suggested changes May 24, 2018

View reviewed changes

jonasfranz approved these changes May 24, 2018

View reviewed changes

bkcsoft added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 24, 2018

jonasfranz approved these changes May 25, 2018

View reviewed changes

jonasfranz suggested changes May 25, 2018

View reviewed changes

bkcsoft added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 29, 2018

daviian force-pushed the bugfix/unallowed-issue-listing branch from 156fd84 to 3603861 Compare June 3, 2018 18:52

techknowlogick added the pr/wip This PR is not ready for review label Jun 3, 2018

lunny approved these changes Jun 19, 2018

View reviewed changes

jonasfranz approved these changes Jun 20, 2018

View reviewed changes

daviian force-pushed the bugfix/unallowed-issue-listing branch from e46702f to fc89ab6 Compare June 20, 2018 20:22

daviian added 16 commits June 21, 2018 12:01

hide issues from org private repos w/o team assignment

6f46ee3

add comments to altered tests

0e3c279

fix test

0b4402b

fix null value on bool field

2d77d18

add suggested changes

17b946e

further improvements

8ea95f6

move team unittypes to new team_unit table

a122731

limit GetUserRepositoryIDs by accessible unittypes

cab5121

check unit types on action and notification creation

a1fdf48

adjust test to take new permission checks into account

7b990f7

load repo before actually using it

4ea560c

fix some things

abdaa54

undo mistakenly moved code

4e21fc7

add unique indexes

498357b

remove team-units if team gets deleted

d7b0a49

remove team-units if org gets deleted

f5a3465

daviian force-pushed the bugfix/unallowed-issue-listing branch from e69f332 to f5a3465 Compare June 21, 2018 10:02

sapk approved these changes Jun 21, 2018

View reviewed changes

techknowlogick merged commit 0b3ea42 into go-gitea:master Jun 21, 2018

techknowlogick mentioned this pull request Jun 21, 2018

Team members able to see issues for private repositories not part of team #4029

Closed

7 tasks

daviian deleted the bugfix/unallowed-issue-listing branch June 21, 2018 20:00

nuno-silva mentioned this pull request Feb 5, 2019

Users in owner team get 404 when accessing organisation repos #4867

Closed

7 tasks

go-gitea locked and limited conversation to collaborators Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hide issues from org private repos w/o team assignment #4034

hide issues from org private repos w/o team assignment #4034

daviian commented May 24, 2018

jonasfranz May 24, 2018

jonasfranz May 24, 2018 •

edited

Loading

daviian May 24, 2018

jonasfranz May 24, 2018

daviian May 24, 2018

jonasfranz May 24, 2018

jonasfranz May 24, 2018

jonasfranz May 24, 2018

jonasfranz May 24, 2018

jonasfranz May 24, 2018

jonasfranz commented May 24, 2018

jonasfranz May 24, 2018

jonasfranz May 24, 2018

codecov-io commented May 24, 2018 •

edited

Loading

jonasfranz May 24, 2018

daviian May 24, 2018

jonasfranz May 24, 2018

daviian May 24, 2018

jonasfranz left a comment •

edited

Loading

lunny commented May 29, 2018

lunny commented May 29, 2018

daviian commented Jun 2, 2018

lafriks commented Jun 2, 2018

daviian commented Jun 2, 2018 •

edited

Loading

lafriks commented Jun 2, 2018

daviian commented Jun 3, 2018 •

edited

Loading

daviian commented Jun 5, 2018

jonasfranz left a comment

lafriks commented Jun 20, 2018

axifive commented Jun 20, 2018 •

edited

Loading

daviian commented Jun 20, 2018

axifive commented Jun 21, 2018

hide issues from org private repos w/o team assignment #4034

hide issues from org private repos w/o team assignment #4034

Conversation

daviian commented May 24, 2018

Choose a reason for hiding this comment

jonasfranz May 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasfranz commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 24, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasfranz left a comment • edited Loading

Choose a reason for hiding this comment

lunny commented May 29, 2018

lunny commented May 29, 2018

daviian commented Jun 2, 2018

lafriks commented Jun 2, 2018

daviian commented Jun 2, 2018 • edited Loading

lafriks commented Jun 2, 2018

daviian commented Jun 3, 2018 • edited Loading

daviian commented Jun 5, 2018

jonasfranz left a comment

Choose a reason for hiding this comment

lafriks commented Jun 20, 2018

axifive commented Jun 20, 2018 • edited Loading

daviian commented Jun 20, 2018

axifive commented Jun 21, 2018

jonasfranz May 24, 2018 •

edited

Loading

codecov-io commented May 24, 2018 •

edited

Loading

jonasfranz left a comment •

edited

Loading

daviian commented Jun 2, 2018 •

edited

Loading

daviian commented Jun 3, 2018 •

edited

Loading

axifive commented Jun 20, 2018 •

edited

Loading