feat: enhance OAuth providers with organization and workspace support #69
Conversation
- Add GitHub organization and team-based authorization - Add Google Workspace domain-based authorization - Consolidate authentication flow by combining user retrieval and authorization - Add comprehensive test coverage for OAuth providers - Add utilities for better error handling - Improve session management with proper cookie settings BREAKING CHANGE: Authorization interface changed from separate GetUserID/Authorization calls to combined Authorization method
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Remove dead code that was not being used anywhere in the codebase.
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the OAuth authentication system by adding organization and workspace support for GitHub and Google providers respectively, while consolidating the Provider interface to streamline the authentication flow.
- Adds GitHub organization/team-based access control and Google Workspace domain restrictions
- Consolidates Provider interface by merging
GetUserIDandAuthorizationmethods into a singleAuthorizationmethod - Improves session management with proper cookie settings and simplified session keys
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/utils/must.go | Adds utility function for error handling |
| pkg/mcp-proxy/main.go | Updates provider constructors and session configuration |
| pkg/idp/idp_test.go | Updates test to use simplified session key |
| pkg/auth/oidc_test.go | Adds comprehensive test coverage for OIDC provider |
| pkg/auth/oidc.go | Implements consolidated Authorization method |
| pkg/auth/mock.go | Updates mock implementation for new interface |
| pkg/auth/main_test.go | Adds test setup configuration |
| pkg/auth/interface.go | Consolidates interface methods |
| pkg/auth/google_test.go | Adds test coverage for Google provider |
| pkg/auth/google.go | Adds workspace support and consolidated authorization |
| pkg/auth/github_test.go | Adds test coverage for GitHub provider |
| pkg/auth/github.go | Adds organization/team support and consolidated authorization |
| pkg/auth/auth_test.go | Updates tests for new interface |
| pkg/auth/auth.go | Simplifies authentication flow and session management |
| main.go | Adds command-line flags for new organization/workspace parameters |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| func Must[T any](v T, err error) T { | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| return v | ||
| } |
There was a problem hiding this comment.
The Must function lacks documentation explaining its purpose and when it should be used. Add a comment describing that this function should only be used when the error is guaranteed not to occur in normal operation, as it will panic on any error.
pkg/auth/github.go
Outdated
| resp, err := client.Get(utils.Must(url.JoinPath(p.endpoint, "/user"))) | ||
| if err != nil { | ||
| return "", err | ||
| return false, "", err | ||
| } |
There was a problem hiding this comment.
Using Must with url.JoinPath is inappropriate here since URL construction can fail with invalid inputs. The error should be handled properly instead of potentially panicking.
pkg/auth/github.go
Outdated
| resp, err = client.Get(utils.Must(url.JoinPath(p.endpoint, "/user/orgs"))) | ||
| if err != nil { | ||
| return false, "", err | ||
| } |
There was a problem hiding this comment.
Using Must with url.JoinPath is inappropriate here since URL construction can fail with invalid inputs. The error should be handled properly instead of potentially panicking.
| resp, err = client.Get(utils.Must(url.JoinPath(p.endpoint, "/user/orgs"))) | |
| if err != nil { | |
| return false, "", err | |
| } | |
| orgsURL, err := url.JoinPath(p.endpoint, "/user/orgs") | |
| if err != nil { | |
| return false, "", err | |
| } | |
| resp, err = client.Get(orgsURL) | |
| if err != nil { | |
| return false, "", err | |
| } |
pkg/auth/github.go
Outdated
| resp, err = client.Get(utils.Must(url.JoinPath(p.endpoint, "/user/teams"))) | ||
| if err != nil { | ||
| return false, "", err | ||
| } |
There was a problem hiding this comment.
Using Must with url.JoinPath is inappropriate here since URL construction can fail with invalid inputs. The error should be handled properly instead of potentially panicking.
| resp, err = client.Get(utils.Must(url.JoinPath(p.endpoint, "/user/teams"))) | |
| if err != nil { | |
| return false, "", err | |
| } | |
| teamsURL, err := url.JoinPath(p.endpoint, "/user/teams") | |
| if err != nil { | |
| return false, "", err | |
| } | |
| resp, err = client.Get(teamsURL) | |
| if err != nil { | |
| return false, "", err | |
| } |
pkg/auth/github.go
Outdated
| return false, "", errors.New("failed to get user info from GitHub API: " + resp.Status) | ||
| } | ||
| defer resp.Body.Close() | ||
| var orgInfo []struct { | ||
| Login string `json:"login"` | ||
| } | ||
| if err := json.NewDecoder(resp.Body).Decode(&orgInfo); err != nil { | ||
| return false, "", err | ||
| } |
There was a problem hiding this comment.
The defer resp.Body.Close() statement will only close the last response body in the loop. Previous response bodies from the organizations API call are not being closed, causing potential resource leaks.
| return false, "", errors.New("failed to get user info from GitHub API: " + resp.Status) | |
| } | |
| defer resp.Body.Close() | |
| var orgInfo []struct { | |
| Login string `json:"login"` | |
| } | |
| if err := json.NewDecoder(resp.Body).Decode(&orgInfo); err != nil { | |
| return false, "", err | |
| } | |
| resp.Body.Close() | |
| return false, "", errors.New("failed to get user info from GitHub API: " + resp.Status) | |
| } | |
| var orgInfo []struct { | |
| Login string `json:"login"` | |
| } | |
| if err := json.NewDecoder(resp.Body).Decode(&orgInfo); err != nil { | |
| resp.Body.Close() | |
| return false, "", err | |
| } | |
| resp.Body.Close() |
Renamed resp variables to resp1, resp2, resp3 to avoid variable shadowing and improve code readability in the GitHub OAuth authorization flow.
Summary
This PR enhances the OAuth authentication system with support for organization-based and workspace-based authorization:
Type of Change
Related Issues
#33
Breaking Changes
BREAKING CHANGE: The Provider interface has changed. The
GetUserIDandAuthorizationmethods have been consolidated into a singleAuthorization(ctx, token) (bool, string, error)method that returns authorization status, user identifier, and any error in one call.