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.

Sign up for GitHub

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

Jump to bottom

Panic with GitLab project repository auth #1113

Merged
merged 3 commits into from
Mar 25, 2021
Merged

Panic with GitLab project repository auth #1113

merged 3 commits into from
Mar 25, 2021

Conversation

piersharding
Copy link
Contributor

  • /api/v4/projects/:id can return nil permissions

Signed-off-by: Piers Harding piers@ompka.net

Description

Check project API payload for nil in GroupAccess value.

fixes #1111

Motivation and Context

This PR addresses #1111 - where the GitLab projects API (https://docs.gitlab.com/ee/api/projects.html) does not necessarily return a GroupAccess value. The resulting permissions are now checked for nil to mitigate this.

How Has This Been Tested?

The change has been tested with setting --gitlab-project=ska-telescope/ska-tango-images=30, and running against the patched image registry.gitlab.com/piersharding/oauth2-proxy:latest .

Checklist:

(not too sure about these settings)

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

@piersharding piersharding requested a review from a team as a code owner March 19, 2021 14:40
@piersharding piersharding mentioned this pull request Mar 19, 2021
Closed
NickMeves
NickMeves reviewed Mar 19, 2021
View reviewed changes
providers/gitlab.go
@@ -331,7 +331,7 @@ func (p *GitLabProvider) addProjectsToSession(ctx context.Context, s *sessions.S
perms = projectInfo.Permissions.GroupAccess
}

if perms.AccessLevel >= project.AccessLevel {
if perms != nil && perms.AccessLevel >= project.AccessLevel {
Copy link
Member

@NickMeves NickMeves Mar 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This mixed with the above if perms == nil fallback handling is slightly confusing. Can we check for nil on line 331 on projectInfo.Permissions.GroupAccess and log + continue if it is?

Can you also add a scenario for this case to the test table here:

oauth2-proxy/providers/gitlab_test.go

Line 189 in 4d9de06

Context("when filtering on gitlab entities (groups and projects)", func() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks - I have reformatted it as suggested.

@piersharding
panic with GitLab project repository auth
7a3dcd8 
* /api/v4/projects/:id can return nil permissions

Signed-off-by: Piers Harding <piers@ompka.net>
@papey
Copy link
Contributor

papey commented Mar 23, 2021 •
edited
Loading

@piersharding can you also add a test case for this ? Thanks !

NickMeves
NickMeves approved these changes Mar 25, 2021
View reviewed changes
Copy link
Member

@NickMeves NickMeves left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@NickMeves
Copy link
Member

We are releasing in a couple of hours, we'll take care of adding a CHANGELOG entry in the release PR.

@NickMeves NickMeves merged commit 73d9f38 into oauth2-proxy:master Mar 25, 2021
@piersharding
Copy link
Contributor Author

Thanks - sorry, I was learning the process as I was going along.
Cheers,
Piers.

@NickMeves
Copy link
Member

No worries! Thanks for the finding the issue & submitting the fix!

@NickMeves NickMeves mentioned this pull request Mar 25, 2021
Merged
3 tasks
samirachoadi pushed a commit to samirachoadi/oauth2-proxy that referenced this pull request Jun 13, 2021
@piersharding @samirachoadi
Panic with GitLab project repository auth (oauth2-proxy#1113)
2d8788f 
* panic with GitLab project repository auth

* /api/v4/projects/:id can return nil permissions

Signed-off-by: Piers Harding <piers@ompka.net>

* Add GitLab test for group no access

Signed-off-by: Piers Harding <piers@ompka.net>
k-jell pushed a commit to liquidinvestigations/oauth2-proxy that referenced this pull request Apr 6, 2022
@piersharding @k-jell
Panic with GitLab project repository auth (oauth2-proxy#1113)
cd40f3b 
* panic with GitLab project repository auth

* /api/v4/projects/:id can return nil permissions

Signed-off-by: Piers Harding <piers@ompka.net>

* Add GitLab test for group no access

Signed-off-by: Piers Harding <piers@ompka.net>
Jing-ze pushed a commit to Jing-ze/oauth2-proxy that referenced this pull request Nov 19, 2024
@cr7258 @johnlanni @CH3CHO
doc: update cluster-key-rate-limit doc (oauth2-proxy#1113)
4e881fd 
Co-authored-by: 澄潭 <zty98751@alibaba-inc.com>
Co-authored-by: Kent Dong <ch3cho@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NickMeves NickMeves NickMeves approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Panic with GitLab project auth when user has no access
3 participants
@piersharding @papey @NickMeves