Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Add min_access_level to GitLab groups query#46480

Merged
pjlast merged 1 commit intomainfrom
pjlast/gitlab-groups-fix
Jan 16, 2023
Merged

Add min_access_level to GitLab groups query#46480
pjlast merged 1 commit intomainfrom
pjlast/gitlab-groups-fix

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

}

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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants