[gitlab] Fix GitLab internal project visibility by pjlast · Pull Request #54426 · sourcegraph/sourcegraph-public-snapshot

pjlast · 2023-06-29T11:52:02Z

Original Slack thread here

Before this PR

We use a min_access_level=20 check on GitLab project queries, because access_level 10 is GUEST, and users with GUEST access cannot see repo contents, so we don't want to give access to those repos on Sourcegraph
HOWEVER, for Internal repos on GitLab, users can see the contents of the repo depending on how the repo is configured, regardless of their access level. So this puts us in a conundrum. If we have min_access_level=20, users can't see repos they should be able to see, but if we remove it, users can see repos they shouldn't be able to see

After this PR

Adds a feature flag named "gitLabProjectVisibilityExperimental". When set to true:

@indradhanush and I diffed some responses from the GitLab API, depending on whether or not you are part of the GitLab group or not, whether the project is visible or not, etc. etc. and there is a common field:
default_branch is only visible in the project API response if the user is able to see the contents of the project. If they cannot see the contents of the project, they cannot see the default branch of the project.
So by removing min_access_level=20 and checking for the presence of default_branch on the returned project list, we can determine whether or not the user should be able to see the repo contents or not

Test plan

VCR tests should be updated

sourcegraph-bot · 2023-06-29T11:54:11Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 0b1f669...204d404.

Notify	File(s)
@eseliger	internal/extsvc/gitlab/projects.go
@indradhanush	internal/repos/gitlab_test.go
@sashaostrikov	internal/extsvc/gitlab/projects.go internal/repos/gitlab_test.go
@unknwon	internal/authz/providers/gitlab/BUILD.bazel internal/authz/providers/gitlab/BUILD.bazel internal/authz/providers/gitlab/oauth_test.go internal/authz/providers/gitlab/oauth_test.go internal/authz/providers/gitlab/sudo.go internal/authz/providers/gitlab/sudo.go internal/authz/providers/gitlab/sudo_test.go internal/authz/providers/gitlab/sudo_test.go

indradhanush

LGTM as we paired on it. 😆

sashaostrikov

LGTM! Let's add a comment directly in code?

sashaostrikov · 2023-06-29T12:20:20Z


 			for _, p := range projects {
-				projectIDs = append(projectIDs, extsvc.RepoID(strconv.Itoa(p.ID)))
+				if p.DefaultBranch != "" {


I'd add a nice comment that you had in Slack/PR here as well

+500. This feels like a tripwire for future us.

I'd also love to see a test for this change.

+1024 really nice trick but even Cody won't be able to explain this 😂

BolajiOlajide

+1 to @sashaostrikov's comment.

Asides that, this looks good to me.

sashaostrikov · 2023-07-05T12:30:40Z

 	return p.Visibility == "private" || p.Visibility == "internal"
 }

+// ContentsVisible reports whether or not the repository contents of this project is visible to the user.


…t_branch, instead of using group access level

@indradhanush

Original Slack thread [here](https://sourcegraph.slack.com/archives/C05EMJM2SLR/p1687969384430849) ## Before this PR We use a `min_access_level=20` check on GitLab project queries, because `access_level` 10 is GUEST, and users with GUEST access cannot see repo contents, so we don't want to give access to those repos on Sourcegraph HOWEVER, for Internal repos on GitLab, users can see the contents of the repo depending on how the repo is configured, regardless of their access level. So this puts us in a conundrum. If we have `min_access_level=20`, users can't see repos they should be able to see, but if we remove it, users can see repos they shouldn't be able to see ## After this PR Adds a feature flag named "gitLabProjectVisibilityExperimental". When set to true: @indradhanush and I diffed some responses from the GitLab API, depending on whether or not you are part of the GitLab group or not, whether the project is visible or not, etc. etc. and there is a common field: `default_branch` is only visible in the project API response if the user is able to see the contents of the project. If they cannot see the contents of the project, they cannot see the default branch of the project. So by removing `min_access_level=20` and checking for the presence of `default_branch` on the returned project list, we can determine whether or not the user should be able to see the repo contents or not ## Test plan VCR tests should be updated  (cherry picked from commit 8cb61f8)

@indradhanush

Original Slack thread [here](https://sourcegraph.slack.com/archives/C05EMJM2SLR/p1687969384430849) ## Before this PR We use a `min_access_level=20` check on GitLab project queries, because `access_level` 10 is GUEST, and users with GUEST access cannot see repo contents, so we don't want to give access to those repos on Sourcegraph HOWEVER, for Internal repos on GitLab, users can see the contents of the repo depending on how the repo is configured, regardless of their access level. So this puts us in a conundrum. If we have `min_access_level=20`, users can't see repos they should be able to see, but if we remove it, users can see repos they shouldn't be able to see ## After this PR Adds a feature flag named "gitLabProjectVisibilityExperimental". When set to true: @indradhanush and I diffed some responses from the GitLab API, depending on whether or not you are part of the GitLab group or not, whether the project is visible or not, etc. etc. and there is a common field: `default_branch` is only visible in the project API response if the user is able to see the contents of the project. If they cannot see the contents of the project, they cannot see the default branch of the project. So by removing `min_access_level=20` and checking for the presence of `default_branch` on the returned project list, we can determine whether or not the user should be able to see the repo contents or not ## Test plan VCR tests should be updated

@indradhanush

Original Slack thread [here](https://sourcegraph.slack.com/archives/C05EMJM2SLR/p1687969384430849) ## Before this PR We use a `min_access_level=20` check on GitLab project queries, because `access_level` 10 is GUEST, and users with GUEST access cannot see repo contents, so we don't want to give access to those repos on Sourcegraph HOWEVER, for Internal repos on GitLab, users can see the contents of the repo depending on how the repo is configured, regardless of their access level. So this puts us in a conundrum. If we have `min_access_level=20`, users can't see repos they should be able to see, but if we remove it, users can see repos they shouldn't be able to see ## After this PR Adds a feature flag named "gitLabProjectVisibilityExperimental". When set to true: @indradhanush and I diffed some responses from the GitLab API, depending on whether or not you are part of the GitLab group or not, whether the project is visible or not, etc. etc. and there is a common field: `default_branch` is only visible in the project API response if the user is able to see the contents of the project. If they cannot see the contents of the project, they cannot see the default branch of the project. So by removing `min_access_level=20` and checking for the presence of `default_branch` on the returned project list, we can determine whether or not the user should be able to see the repo contents or not ## Test plan VCR tests should be updated  <br> Backport 8cb61f8 from #54426 Co-authored-by: Petri-Johan Last <petri.last@sourcegraph.com>

@indradhanush

Original Slack thread [here](https://sourcegraph.slack.com/archives/C05EMJM2SLR/p1687969384430849) ## Before this PR We use a `min_access_level=20` check on GitLab project queries, because `access_level` 10 is GUEST, and users with GUEST access cannot see repo contents, so we don't want to give access to those repos on Sourcegraph HOWEVER, for Internal repos on GitLab, users can see the contents of the repo depending on how the repo is configured, regardless of their access level. So this puts us in a conundrum. If we have `min_access_level=20`, users can't see repos they should be able to see, but if we remove it, users can see repos they shouldn't be able to see ## After this PR Adds a feature flag named "gitLabProjectVisibilityExperimental". When set to true: @indradhanush and I diffed some responses from the GitLab API, depending on whether or not you are part of the GitLab group or not, whether the project is visible or not, etc. etc. and there is a common field: `default_branch` is only visible in the project API response if the user is able to see the contents of the project. If they cannot see the contents of the project, they cannot see the default branch of the project. So by removing `min_access_level=20` and checking for the presence of `default_branch` on the returned project list, we can determine whether or not the user should be able to see the repo contents or not ## Test plan VCR tests should be updated

cla-bot Bot added the cla-signed label Jun 29, 2023

github-actions Bot added the team/iam label Jun 29, 2023

pjlast requested a review from a team June 29, 2023 11:59

indradhanush added the team/source Tickets under the purview of Source - the one Source to graph it all label Jun 29, 2023

indradhanush approved these changes Jun 29, 2023

View reviewed changes

sashaostrikov approved these changes Jun 29, 2023

View reviewed changes

BolajiOlajide approved these changes Jun 29, 2023

View reviewed changes

sashaostrikov approved these changes Jul 5, 2023

View reviewed changes

pjlast added 3 commits July 27, 2023 09:08

Determine GitLab project visibility by checking if we can view defaul…

8963e27

…t_branch, instead of using group access level

Update changelog

c7355c2

Explain default branch checking

6da60d3

pjlast force-pushed the pjlast/gitlab-internal-repo-perms-fix branch from eb14c81 to 6da60d3 Compare July 27, 2023 07:09

pjlast added 5 commits July 27, 2023 09:24

Move changelog entry

8fe4ea3

VCR test update

4c684e3

Adjust perms tests

37dd7cf

More test updates

692eac7

Internal repos test fix

9cc2e1b

kopancek approved these changes Jul 27, 2023

View reviewed changes

pjlast added 5 commits July 27, 2023 13:34

Add feature flag check

a1258a0

bazel configure

322e363

Add feature flag and tests

3780f2e

bazel configure

bd34e0b

Update changelog

204d404

pjlast added the backport 5.1 label Jul 27, 2023

pjlast merged commit 8cb61f8 into main Jul 27, 2023

pjlast deleted the pjlast/gitlab-internal-repo-perms-fix branch July 27, 2023 13:13

github-actions Bot mentioned this pull request Jul 27, 2023

[Backport 5.1] [gitlab] Fix GitLab internal project visibility #55359

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[gitlab] Fix GitLab internal project visibility#54426

[gitlab] Fix GitLab internal project visibility#54426
pjlast merged 13 commits into
mainfrom
pjlast/gitlab-internal-repo-perms-fix

pjlast commented Jun 29, 2023 •

edited

Loading

Uh oh!

sourcegraph-bot commented Jun 29, 2023 •

edited

Loading

Uh oh!

indradhanush left a comment

Uh oh!

sashaostrikov left a comment

Uh oh!

sashaostrikov Jun 29, 2023

Uh oh!

mrnugget Jun 29, 2023

Uh oh!

unknwon Jul 5, 2023

Uh oh!

BolajiOlajide left a comment

Uh oh!

sashaostrikov Jul 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Conversation

pjlast commented Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before this PR

After this PR

Test plan

Uh oh!

sourcegraph-bot commented Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

indradhanush left a comment

Choose a reason for hiding this comment

Uh oh!

sashaostrikov left a comment

Choose a reason for hiding this comment

Uh oh!

sashaostrikov Jun 29, 2023

Choose a reason for hiding this comment

Uh oh!

mrnugget Jun 29, 2023

Choose a reason for hiding this comment

Uh oh!

unknwon Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

BolajiOlajide left a comment

Choose a reason for hiding this comment

Uh oh!

sashaostrikov Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

pjlast commented Jun 29, 2023 •

edited

Loading

sourcegraph-bot commented Jun 29, 2023 •

edited

Loading