upstreamgitub.go now uses githubclient to determine username and groups

2026-01-03 11:45:45 +00:00 · 2024-05-20 09:46:57 -05:00
parent 8719c7a2db
commit 938bea9910
6 changed files with 507 additions and 23 deletions
--- a/internal/upstreamgithub/upstreamgithub.go
+++ b/internal/upstreamgithub/upstreamgithub.go
@@ -6,14 +6,19 @@ package upstreamgithub

 import (
 	"context"
+	"errors"
+	"fmt"
 	"net/http"

 	coreosoidc "github.com/coreos/go-oidc/v3/oidc"
 	"golang.org/x/oauth2"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/sets"

-	"go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1"
+	supervisoridpv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1"
+	"go.pinniped.dev/internal/federationdomain/downstreamsubject"
 	"go.pinniped.dev/internal/federationdomain/upstreamprovider"
+	"go.pinniped.dev/internal/githubclient"
 )

 // ProviderConfig holds the active configuration of an upstream GitHub provider.
@@ -26,8 +31,8 @@ type ProviderConfig struct {
 	// or https://HOSTNAME/api/v3/ for Enterprise Server.
 	APIBaseURL string

-	UsernameAttribute  v1alpha1.GitHubUsernameAttribute
-	GroupNameAttribute v1alpha1.GitHubGroupNameAttribute
+	UsernameAttribute  supervisoridpv1alpha1.GitHubUsernameAttribute
+	GroupNameAttribute supervisoridpv1alpha1.GitHubGroupNameAttribute

 	// AllowedOrganizations, when empty, means to allow users from all orgs.
 	AllowedOrganizations []string
@@ -46,7 +51,8 @@ type ProviderConfig struct {
 }

 type Provider struct {
-	c ProviderConfig
+	c                 ProviderConfig
+	buildGitHubClient func(httpClient *http.Client, apiBaseURL, token string) (githubclient.GitHubInterface, error)
 }

 var _ upstreamprovider.UpstreamGithubIdentityProviderI = &Provider{}
@@ -54,7 +60,10 @@ var _ upstreamprovider.UpstreamGithubIdentityProviderI = &Provider{}
 // New creates a Provider. The config is not a pointer to ensure that a copy of the config is created,
 // making the resulting Provider use an effectively read-only configuration.
 func New(config ProviderConfig) *Provider {
-	return &Provider{c: config}
+	return &Provider{
+		c:                 config,
+		buildGitHubClient: githubclient.NewGitHubClient,
+	}
 }

 func (p *Provider) GetName() string {
@@ -73,11 +82,11 @@ func (p *Provider) GetScopes() []string {
 	return p.c.OAuth2Config.Scopes
 }

-func (p *Provider) GetUsernameAttribute() v1alpha1.GitHubUsernameAttribute {
+func (p *Provider) GetUsernameAttribute() supervisoridpv1alpha1.GitHubUsernameAttribute {
 	return p.c.UsernameAttribute
 }

-func (p *Provider) GetGroupNameAttribute() v1alpha1.GitHubGroupNameAttribute {
+func (p *Provider) GetGroupNameAttribute() supervisoridpv1alpha1.GitHubGroupNameAttribute {
 	return p.c.GroupNameAttribute
 }

@@ -104,19 +113,73 @@ func (p *Provider) ExchangeAuthcode(ctx context.Context, authcode string, redire
 	return tok.AccessToken, nil
 }

-func (p *Provider) GetUser(_ctx context.Context, _accessToken string) (*upstreamprovider.GitHubUser, error) {
-	// TODO Implement this to make several https calls to github to learn about the user, using a lower-level githubclient package.
-	//   Pass the ctx, accessToken, p.c.HttpClient, and p.c.APIBaseURL to the lower-level package's functions.
-	// TODO: Reject the auth if the user does not belong to any of p.c.AllowedOrganizations (unless p.c.AllowedOrganizations is empty).
-	// TODO: Make use of p.c.UsernameAttribute and p.c.GroupNameAttribute when deciding the username and group names.
-	// TODO: Determine the downstream subject by first writing a helper in downstream_subject.go and then calling it here.
-	panic("implement me")
-	//nolint:govet // this code is intentionally unreachable until we resolve the todos
-	return &upstreamprovider.GitHubUser{
-		Username:          "TODO",
-		Groups:            []string{"org/TODO"},
-		DownstreamSubject: "TODO",
-	}, nil
+// GetUser will use the provided configuration to make HTTPS calls to the GitHub API to find out who the logged-in user is,
+// what organizations they belong to, and what teams they belong to.
+// If the user's information meets the AllowedOrganization criteria specified on the GitHubIdentityProvider, they will be
+// allowed to log in.
+// Note that errors from the githubclient package already have helpful error prefixes, so there is no need for additional prefixes here.
+// TODO: populate the IDP display name
+// TODO: What should we do if the group or team name is outside of the enum? The controller would reject this.
+// TODO: should we use the APIBaseURL or some other URL in the downstreamSubject
+//
+// Examples:
+//
+//	"github.com" or "https://github.com" or "https://api.github.com"?
+//	"enterprise.tld" or "https://enterprise.tld" or "https://enterprise.tld/api/v3"?
+func (p *Provider) GetUser(ctx context.Context, accessToken string) (*upstreamprovider.GitHubUser, error) {
+	githubClient, err := p.buildGitHubClient(p.c.HttpClient, p.c.APIBaseURL, accessToken)
+	if err != nil {
+		return nil, err
+	}
+
+	githubUser := upstreamprovider.GitHubUser{}
+
+	userInfo, err := githubClient.GetUserInfo(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	githubUser.DownstreamSubject = downstreamsubject.GitHub(p.c.APIBaseURL, "TODO_IDP_DISPLAY_NAME", userInfo.Login, userInfo.ID)
+
+	switch p.c.UsernameAttribute {
+	case supervisoridpv1alpha1.GitHubUsernameLoginAndID:
+		githubUser.Username = fmt.Sprintf("%s:%s", userInfo.Login, userInfo.ID)
+	case supervisoridpv1alpha1.GitHubUsernameLogin:
+		githubUser.Username = userInfo.Login
+	case supervisoridpv1alpha1.GitHubUsernameID:
+		githubUser.Username = userInfo.ID
+	}
+
+	orgMembership, err := githubClient.GetOrgMembership(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	allowedOrgs := sets.New[string](p.c.AllowedOrganizations...)
+
+	if allowedOrgs.Len() > 0 && allowedOrgs.Intersection(orgMembership).Len() < 1 {
+		return nil, errors.New("user is not allowed to log in due to organization membership policy")
+	}
+
+	teamMembership, err := githubClient.GetTeamMembership(ctx, allowedOrgs)
+	if err != nil {
+		return nil, err
+	}
+
+	for _, team := range teamMembership {
+		downstreamGroup := ""
+
+		switch p.c.GroupNameAttribute {
+		case supervisoridpv1alpha1.GitHubUseTeamNameForGroupName:
+			downstreamGroup = fmt.Sprintf("%s/%s", team.Org, team.Name)
+		case supervisoridpv1alpha1.GitHubUseTeamSlugForGroupName:
+			downstreamGroup = fmt.Sprintf("%s/%s", team.Org, team.Slug)
+		}
+
+		githubUser.Groups = append(githubUser.Groups, downstreamGroup)
+	}
+
+	return &githubUser, nil
 }

 // GetConfig returns the config. This is not part of the UpstreamGithubIdentityProviderI interface and is just for testing.
--- a/internal/upstreamgithub/upstreamgithub_test.go
+++ b/internal/upstreamgithub/upstreamgithub_test.go
@@ -4,14 +4,22 @@
 package upstreamgithub

 import (
+	"context"
+	"errors"
 	"net/http"
 	"testing"

 	"github.com/stretchr/testify/require"
+	"go.uber.org/mock/gomock"
 	"golang.org/x/oauth2"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/rand"
+	"k8s.io/apimachinery/pkg/util/sets"

-	"go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1"
+	supervisoridpv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1"
+	"go.pinniped.dev/internal/federationdomain/upstreamprovider"
+	"go.pinniped.dev/internal/githubclient"
+	"go.pinniped.dev/internal/mocks/mockgithubclient"
 )

 func TestGitHubProvider(t *testing.T) {
@@ -65,11 +73,282 @@ func TestGitHubProvider(t *testing.T) {
 	require.Equal(t, types.UID("resource-uid-12345"), subject.GetResourceUID())
 	require.Equal(t, "fake-client-id", subject.GetClientID())
 	require.Equal(t, "fake-client-id", subject.GetClientID())
-	require.Equal(t, v1alpha1.GitHubUsernameAttribute("fake-username-attribute"), subject.GetUsernameAttribute())
-	require.Equal(t, v1alpha1.GitHubGroupNameAttribute("fake-group-name-attribute"), subject.GetGroupNameAttribute())
+	require.Equal(t, supervisoridpv1alpha1.GitHubUsernameAttribute("fake-username-attribute"), subject.GetUsernameAttribute())
+	require.Equal(t, supervisoridpv1alpha1.GitHubGroupNameAttribute("fake-group-name-attribute"), subject.GetGroupNameAttribute())
 	require.Equal(t, []string{"fake-org", "fake-org2"}, subject.GetAllowedOrganizations())
 	require.Equal(t, "https://fake-authorization-url", subject.GetAuthorizationURL())
 	require.Equal(t, &http.Client{
 		Timeout: 1234509,
 	}, subject.GetConfig().HttpClient)
 }
+
+func TestGetUser(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	t.Cleanup(ctrl.Finish)
+
+	someContext := context.Background()
+
+	someHttpClient := &http.Client{
+		Timeout: 1234509,
+	}
+
+	tests := []struct {
+		name                   string
+		providerConfig         ProviderConfig
+		buildGitHubClientError error
+		buildMockResponses     func(hubInterface *mockgithubclient.MockGitHubInterface)
+		wantUser               *upstreamprovider.GitHubUser
+		wantErr                string
+	}{
+		{
+			name: "happy path with username=login:id",
+			providerConfig: ProviderConfig{
+				APIBaseURL:        "https://some-url",
+				HttpClient:        someHttpClient,
+				UsernameAttribute: supervisoridpv1alpha1.GitHubUsernameLoginAndID,
+			},
+			buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) {
+				mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{
+					Login: "some-github-login",
+					ID:    "some-github-id",
+				}, nil)
+				mockGitHubInterface.EXPECT().GetOrgMembership(someContext).Return(nil, nil)
+				mockGitHubInterface.EXPECT().GetTeamMembership(someContext, sets.New[string]()).Return(nil, nil)
+			},
+			wantUser: &upstreamprovider.GitHubUser{
+				Username:          "some-github-login:some-github-id",
+				DownstreamSubject: "https://some-url?idpName=TODO_IDP_DISPLAY_NAME&login=some-github-login&id=some-github-id",
+			},
+		},
+		{
+			name: "happy path with username=login",
+			providerConfig: ProviderConfig{
+				APIBaseURL:        "https://some-url",
+				HttpClient:        someHttpClient,
+				UsernameAttribute: supervisoridpv1alpha1.GitHubUsernameLogin,
+			},
+			buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) {
+				mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{
+					Login: "some-github-login",
+					ID:    "some-github-id",
+				}, nil)
+				mockGitHubInterface.EXPECT().GetOrgMembership(someContext).Return(nil, nil)
+				mockGitHubInterface.EXPECT().GetTeamMembership(someContext, sets.New[string]()).Return(nil, nil)
+			},
+			wantUser: &upstreamprovider.GitHubUser{
+				Username:          "some-github-login",
+				DownstreamSubject: "https://some-url?idpName=TODO_IDP_DISPLAY_NAME&login=some-github-login&id=some-github-id",
+			},
+		},
+		{
+			name: "happy path with username=id",
+			providerConfig: ProviderConfig{
+				APIBaseURL:        "https://some-url",
+				HttpClient:        someHttpClient,
+				UsernameAttribute: supervisoridpv1alpha1.GitHubUsernameID,
+			},
+			buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) {
+				mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{
+					Login: "some-github-login",
+					ID:    "some-github-id",
+				}, nil)
+				mockGitHubInterface.EXPECT().GetOrgMembership(someContext).Return(nil, nil)
+				mockGitHubInterface.EXPECT().GetTeamMembership(someContext, sets.New[string]()).Return(nil, nil)
+			},
+			wantUser: &upstreamprovider.GitHubUser{
+				Username:          "some-github-id",
+				DownstreamSubject: "https://some-url?idpName=TODO_IDP_DISPLAY_NAME&login=some-github-login&id=some-github-id",
+			},
+		},
+		{
+			name: "happy path with user in allowed organizations",
+			providerConfig: ProviderConfig{
+				APIBaseURL:           "https://some-url",
+				HttpClient:           someHttpClient,
+				UsernameAttribute:    supervisoridpv1alpha1.GitHubUsernameLoginAndID,
+				AllowedOrganizations: []string{"allowed-org1", "allowed-org2"},
+			},
+			buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) {
+				mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{
+					Login: "some-github-login",
+					ID:    "some-github-id",
+				}, nil)
+				mockGitHubInterface.EXPECT().GetOrgMembership(someContext).Return(sets.New[string]("allowed-org2"), nil)
+				mockGitHubInterface.EXPECT().GetTeamMembership(someContext, sets.New[string]("allowed-org1", "allowed-org2")).Return(nil, nil)
+			},
+			wantUser: &upstreamprovider.GitHubUser{
+				Username:          "some-github-login:some-github-id",
+				DownstreamSubject: "https://some-url?idpName=TODO_IDP_DISPLAY_NAME&login=some-github-login&id=some-github-id",
+			},
+		},
+		{
+			name: "returns error when the user does not belong to the allowed organizations",
+			providerConfig: ProviderConfig{
+				APIBaseURL:           "https://some-url",
+				HttpClient:           someHttpClient,
+				UsernameAttribute:    supervisoridpv1alpha1.GitHubUsernameID,
+				AllowedOrganizations: []string{"allowed-org"},
+			},
+			buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) {
+				mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{
+					Login: "some-github-login",
+					ID:    "some-github-id",
+				}, nil)
+				mockGitHubInterface.EXPECT().GetOrgMembership(someContext).Return(sets.New[string]("disallowed-org"), nil)
+			},
+			wantErr: "user is not allowed to log in due to organization membership policy",
+		},
+		{
+			name: "happy path with groups=name",
+			providerConfig: ProviderConfig{
+				APIBaseURL:           "https://some-url",
+				HttpClient:           someHttpClient,
+				UsernameAttribute:    supervisoridpv1alpha1.GitHubUsernameLoginAndID,
+				AllowedOrganizations: []string{"allowed-org1", "allowed-org2"},
+				GroupNameAttribute:   supervisoridpv1alpha1.GitHubUseTeamNameForGroupName,
+			},
+			buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) {
+				mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{
+					Login: "some-github-login",
+					ID:    "some-github-id",
+				}, nil)
+				mockGitHubInterface.EXPECT().GetOrgMembership(someContext).Return(sets.New[string]("allowed-org2"), nil)
+				mockGitHubInterface.EXPECT().GetTeamMembership(someContext, sets.New[string]("allowed-org1", "allowed-org2")).Return([]*githubclient.TeamInfo{
+					{
+						Name: "org1-team1-name",
+						Slug: "org1-team1-slug",
+						Org:  "org1-name",
+					},
+					{
+						Name: "org1-team2-name",
+						Slug: "org1-team2-slug",
+						Org:  "org1-name",
+					},
+					{
+						Name: "org2-team1-name",
+						Slug: "org2-team1-slug",
+						Org:  "org2-name",
+					},
+				}, nil)
+			},
+			wantUser: &upstreamprovider.GitHubUser{
+				Username:          "some-github-login:some-github-id",
+				Groups:            []string{"org1-name/org1-team1-name", "org1-name/org1-team2-name", "org2-name/org2-team1-name"},
+				DownstreamSubject: "https://some-url?idpName=TODO_IDP_DISPLAY_NAME&login=some-github-login&id=some-github-id",
+			},
+		},
+		{
+			name: "happy path with groups=slug",
+			providerConfig: ProviderConfig{
+				APIBaseURL:           "https://some-url",
+				HttpClient:           someHttpClient,
+				UsernameAttribute:    supervisoridpv1alpha1.GitHubUsernameLoginAndID,
+				AllowedOrganizations: []string{"allowed-org1", "allowed-org2"},
+				GroupNameAttribute:   supervisoridpv1alpha1.GitHubUseTeamSlugForGroupName,
+			},
+			buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) {
+				mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{
+					Login: "some-github-login",
+					ID:    "some-github-id",
+				}, nil)
+				mockGitHubInterface.EXPECT().GetOrgMembership(someContext).Return(sets.New[string]("allowed-org2"), nil)
+				mockGitHubInterface.EXPECT().GetTeamMembership(someContext, sets.New[string]("allowed-org1", "allowed-org2")).Return([]*githubclient.TeamInfo{
+					{
+						Name: "org1-team1-name",
+						Slug: "org1-team1-slug",
+						Org:  "org1-name",
+					},
+					{
+						Name: "org1-team2-name",
+						Slug: "org1-team2-slug",
+						Org:  "org1-name",
+					},
+					{
+						Name: "org2-team1-name",
+						Slug: "org2-team1-slug",
+						Org:  "org2-name",
+					},
+				}, nil)
+			},
+			wantUser: &upstreamprovider.GitHubUser{
+				Username:          "some-github-login:some-github-id",
+				Groups:            []string{"org1-name/org1-team1-slug", "org1-name/org1-team2-slug", "org2-name/org2-team1-slug"},
+				DownstreamSubject: "https://some-url?idpName=TODO_IDP_DISPLAY_NAME&login=some-github-login&id=some-github-id",
+			},
+		},
+		{
+			name: "returns errors from buildGitHubClient()",
+			providerConfig: ProviderConfig{
+				APIBaseURL: "https://some-url",
+				HttpClient: someHttpClient,
+			},
+			buildGitHubClientError: errors.New("error from building a github client"),
+			wantErr:                "error from building a github client",
+		},
+		{
+			name: "returns errors from githubClient.GetUserInfo()",
+			providerConfig: ProviderConfig{
+				APIBaseURL: "https://some-url",
+				HttpClient: someHttpClient,
+			},
+			buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) {
+				mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(nil, errors.New("error from githubClient.GetUserInfo"))
+			},
+			wantErr: "error from githubClient.GetUserInfo",
+		},
+		{
+			name: "returns errors from githubClient.GetOrgMembership()",
+			providerConfig: ProviderConfig{
+				APIBaseURL: "https://some-url",
+				HttpClient: someHttpClient,
+			},
+			buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) {
+				mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{}, nil)
+				mockGitHubInterface.EXPECT().GetOrgMembership(someContext).Return(nil, errors.New("error from githubClient.GetOrgMembership"))
+			},
+			wantErr: "error from githubClient.GetOrgMembership",
+		},
+		{
+			name: "returns errors from githubClient.GetTeamMembership()",
+			providerConfig: ProviderConfig{
+				APIBaseURL: "https://some-url",
+				HttpClient: someHttpClient,
+			},
+			buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) {
+				mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{}, nil)
+				mockGitHubInterface.EXPECT().GetOrgMembership(someContext).Return(nil, nil)
+				mockGitHubInterface.EXPECT().GetTeamMembership(someContext, gomock.Any()).Return(nil, errors.New("error from githubClient.GetTeamMembership"))
+			},
+			wantErr: "error from githubClient.GetTeamMembership",
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
+			accessToken := "some-opaque-github-access-token" + rand.String(8)
+			mockGitHubInterface := mockgithubclient.NewMockGitHubInterface(ctrl)
+			if test.buildMockResponses != nil {
+				test.buildMockResponses(mockGitHubInterface)
+			}
+
+			p := New(test.providerConfig)
+			p.buildGitHubClient = func(httpClient *http.Client, apiBaseURL, token string) (githubclient.GitHubInterface, error) {
+				require.Equal(t, test.providerConfig.HttpClient, httpClient)
+				require.Equal(t, test.providerConfig.APIBaseURL, apiBaseURL)
+				require.Equal(t, accessToken, token)
+
+				return mockGitHubInterface, test.buildGitHubClientError
+			}
+
+			actualUser, actualErr := p.GetUser(context.Background(), accessToken)
+			if test.wantErr != "" {
+				require.EqualError(t, actualErr, test.wantErr)
+				require.Nil(t, actualUser)
+				return
+			}
+			require.NoError(t, actualErr)
+			require.Equal(t, test.wantUser, actualUser)
+		})
+	}
+}