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

Panic with GitLab project repository auth #1113

Merged
merged 3 commits into from
Mar 25, 2021
Merged

Panic with GitLab project repository auth #1113

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a3dcd8
panic with GitLab project repository auth
piersharding Mar 18, 2021
7581c9f
Add GitLab test for group no access
piersharding Mar 24, 2021
32597f1
Merge remote-tracking branch 'origin'
piersharding Mar 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion providers/gitlab.go
View file
Original file line number Diff line number Diff line change
@@ -329,9 +329,14 @@ func (p *GitLabProvider) addProjectsToSession(ctx context.Context, s *sessions.S
if perms == nil {
// use group project access as fallback
perms = projectInfo.Permissions.GroupAccess
// group project access is not set for this user then we give up
if perms == nil {
logger.Errorf("Warning: user %q has no project level access to %s", s.Email, project.Name)
continue
}
}

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

@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.

s.Groups = append(s.Groups, fmt.Sprintf("project:%s", project.Name))
} else {
logger.Errorf("Warning: user %q does not have the minimum required access level for project %q", s.Email, project.Name)
24 changes: 24 additions & 0 deletions providers/gitlab_test.go
View file
Original file line number Diff line number Diff line change
@@ -41,6 +41,7 @@ func testGitLabBackend() *httptest.Server {
"groups": ["foo", "bar"]
}
`

projectInfo := `
{
"name": "MyProject",
@@ -56,6 +57,18 @@ func testGitLabBackend() *httptest.Server {
}
`

noAccessProjectInfo := `
{
"name": "NoAccessProject",
"archived": false,
"path_with_namespace": "no_access_group/no_access_project",
"permissions": {
"project_access": null,
"group_access": null,
}
}
`

personalProjectInfo := `
{
"name": "MyPersonalProject",
@@ -105,6 +118,13 @@ func testGitLabBackend() *httptest.Server {
} else {
w.WriteHeader(401)
}
case "/api/v4/projects/no_access_group/no_access_project":
if r.Header["Authorization"][0] == authHeader {
w.WriteHeader(200)
w.Write([]byte(noAccessProjectInfo))
} else {
w.WriteHeader(401)
}
case "/api/v4/projects/my_group/my_archived_project":
if r.Header["Authorization"][0] == authHeader {
w.WriteHeader(200)
@@ -219,6 +239,10 @@ var _ = Describe("Gitlab Provider Tests", func() {
expectedValue: nil,
projects: []string{"my_group/my_project=40"},
}),
Entry("project membership invalid on group project, no access at all", entitiesTableInput{
expectedValue: nil,
projects: []string{"no_access_group/no_access_project=30"},
}),
Entry("project membership valid on personnal project", entitiesTableInput{
expectedValue: []string{"project:my_profile/my_personal_project"},
projects: []string{"my_profile/my_personal_project"},