Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add min_access_level to GitLab groups query #46480

Merged
merged 1 commit into from Jan 16, 2023
Merged

Conversation

pjlast
Copy link
Contributor

@pjlast pjlast commented Jan 16, 2023

When checking if a user belongs to a GitLab group to restrict signup, we hit the /groups endpoint and check whether any of the returned groups are in that endpoint.

However, this endpoint returns all visible groups. Just because a group is visible to a user does not imply the user is a member of that group.

We need to add min_access_level=10 to the query, so that the user has to, at minimum, be a guest on the group.

Test plan

Manual verification

@github-actions
Copy link
Contributor

Problem: the label i-acknowledge-this-goes-into-the-release is absent.
👉 What to do: we're in the next Sourcegraph release code freeze period. If you are 100% sure your changes should get released or provide no risk to the release, add the label your PR with i-acknowledge-this-goes-into-the-release.

@sourcegraph-bot
Copy link
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff e06b1cc...1a5b237.

Notify File(s)
@eseliger internal/extsvc/gitlab/groups.go

@@ -19,7 +19,7 @@ func (c *Client) ListGroups(ctx context.Context, page int) (groups []*Group, has
return MockListGroups(ctx, page)
}

url := fmt.Sprintf("groups?per_page=100&page=%d", page)
url := fmt.Sprintf("groups?per_page=100&page=%d&min_access_level=10", page)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the user is in more than 100 groups? We have no pagination?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is page=%d in there.

Just about all of our clients depend on the caller to paginate through the list

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂️ Ugh, you're right. Sorry!

@pjlast pjlast merged commit 174e312 into main Jan 16, 2023
@pjlast pjlast deleted the pjlast/gitlab-groups-fix branch January 16, 2023 08:33
DaedalusG added a commit that referenced this pull request Jan 16, 2023
pjlast pushed a commit that referenced this pull request Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants