Finish initial github login flow

Also: - fix github teams query: fix bug and sort/unique the results - add IDP display name to github downstream subject - fix error types returned by LoginFromCallback - add trace logs to github API results - update e2e test - implement placeholder version of refresh for github
2026-09-05 07:37:12 +00:00 · 2024-05-22 21:21:45 -05:00
parent ba2d122308
commit 8923704f3c
14 changed files with 453 additions and 275 deletions
@@ -99,34 +99,23 @@ func (p *Provider) GetAuthorizationURL() string {
 }

 func (p *Provider) ExchangeAuthcode(ctx context.Context, authcode string, redirectURI string) (string, error) {
-	// TODO: write tests for this
-	panic("write some tests for this sketch of the implementation, maybe by running a test server in the unit tests")
-	//nolint:govet // this code is intentionally unreachable until we resolve the todos
 	tok, err := p.c.OAuth2Config.Exchange(
 		coreosoidc.ClientContext(ctx, p.c.HttpClient),
 		authcode,
 		oauth2.SetAuthURLParam("redirect_uri", redirectURI),
 	)
 	if err != nil {
-		return "", err
+		return "", fmt.Errorf("error exchanging authorization code using GitHub API: %w", err)
 	}
 	return tok.AccessToken, 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.
+// GetUser will use the provided configuration to make HTTPS calls to the GitHub API to get the identity of the
+// authenticated user and to discover their org and team memberships.
+// 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) {
+func (p *Provider) GetUser(ctx context.Context, accessToken string, idpDisplayName string) (*upstreamprovider.GitHubUser, error) {
 	githubClient, err := p.buildGitHubClient(p.c.HttpClient, p.c.APIBaseURL, accessToken)
 	if err != nil {
 		return nil, err
@@ -139,7 +128,7 @@ func (p *Provider) GetUser(ctx context.Context, accessToken string) (*upstreampr
 		return nil, err
 	}

-	githubUser.DownstreamSubject = downstreamsubject.GitHub(p.c.APIBaseURL, "TODO_IDP_DISPLAY_NAME", userInfo.Login, userInfo.ID)
+	githubUser.DownstreamSubject = downstreamsubject.GitHub(p.c.APIBaseURL, idpDisplayName, userInfo.Login, userInfo.ID)

 	switch p.c.UsernameAttribute {
 	case supervisoridpv1alpha1.GitHubUsernameLoginAndID:
@@ -148,6 +137,8 @@ func (p *Provider) GetUser(ctx context.Context, accessToken string) (*upstreampr
 		githubUser.Username = userInfo.Login
 	case supervisoridpv1alpha1.GitHubUsernameID:
 		githubUser.Username = userInfo.ID
+	default:
+		return nil, fmt.Errorf("bad configuration: unknown GitHub username attribute: %s", p.c.UsernameAttribute)
 	}

 	orgMembership, err := githubClient.GetOrgMembership(ctx)
@@ -174,6 +165,8 @@ func (p *Provider) GetUser(ctx context.Context, accessToken string) (*upstreampr
 			downstreamGroup = fmt.Sprintf("%s/%s", team.Org, team.Name)
 		case supervisoridpv1alpha1.GitHubUseTeamSlugForGroupName:
 			downstreamGroup = fmt.Sprintf("%s/%s", team.Org, team.Slug)
+		default:
+			return nil, fmt.Errorf("bad configuration: unknown GitHub group name attribute: %s", p.c.GroupNameAttribute)
 		}

 		githubUser.Groups = append(githubUser.Groups, downstreamGroup)
@@ -5,9 +5,12 @@ package upstreamgithub

 import (
 	"context"
+	"crypto/tls"
 	"errors"
+	"fmt"
 	"net/http"
 	"testing"
+	"time"

 	"github.com/stretchr/testify/require"
 	"go.uber.org/mock/gomock"
@@ -15,11 +18,13 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/rand"
 	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/client-go/util/cert"

 	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"
+	"go.pinniped.dev/internal/testutil/tlsserver"
 )

 func TestGitHubProvider(t *testing.T) {
@@ -82,7 +87,112 @@ func TestGitHubProvider(t *testing.T) {
 	}, subject.GetConfig().HttpClient)
 }

+func TestExchangeAuthcode(t *testing.T) {
+	const fakeGitHubAccessToken = "gho_16C7e42F292c6912E7710c838347Ae178B4a" //nolint:gosec // this is not a credential
+
+	tests := []struct {
+		name              string
+		tokenEndpointPath string
+		wantErr           string
+	}{
+		{
+			name:              "happy path",
+			tokenEndpointPath: "/token",
+		},
+		{
+			name:              "when the GitHub token endpoint returns an error",
+			tokenEndpointPath: "/token-error",
+			wantErr:           "error exchanging authorization code using GitHub API: oauth2: cannot fetch token: 401 Unauthorized\nResponse: some github error",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
+			testServer, testServerCA := tlsserver.TestServerIPv4(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				// See documentation at https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/authorizing-oauth-apps
+				// GitHub docs say to use a POST.
+				require.Equal(t, http.MethodPost, r.Method)
+
+				// The OAuth client library happens to choose to send these headers. Asserting here for our own understanding.
+				require.Len(t, r.Header, 4)
+				require.Equal(t, "application/x-www-form-urlencoded", r.Header.Get("Content-Type"))
+				require.Equal(t, "gzip", r.Header.Get("Accept-Encoding"))
+				require.NotEmpty(t, r.Header.Get("User-Agent"))
+				require.NotEmpty(t, r.Header.Get("Content-Length"))
+
+				// Get the params.
+				err := r.ParseForm()
+				require.NoError(t, err)
+				params := r.PostForm
+				require.Len(t, params, 5)
+				// These four params are documented by GitHub.
+				require.Equal(t, "fake-client-id", params.Get("client_id"))
+				require.Equal(t, "fake-client-secret", params.Get("client_secret"))
+				require.Equal(t, "https://fake-redirect-url", params.Get("redirect_uri"))
+				require.Equal(t, "fake-authcode", params.Get("code"))
+				// This param is not documented by GitHub, but is standard OAuth2. GitHub should respect or ignore it.
+				require.Equal(t, "authorization_code", params.Get("grant_type"))
+
+				// The GitHub docs say that it will return a URL encoded form by default, so I assume it would set this header.
+				w.Header().Set("content-type", "application/x-www-form-urlencoded")
+
+				switch r.URL.Path {
+				case "/token":
+					// Example response from GitHub docs.
+					responseBody := "access_token=" + fakeGitHubAccessToken + "&scope=repo%2Cgist&token_type=bearer"
+					w.WriteHeader(http.StatusOK)
+					_, err = w.Write([]byte(responseBody))
+					require.NoError(t, err)
+				case "/token-error":
+					responseBody := "some github error"
+					w.WriteHeader(http.StatusUnauthorized)
+					_, err = w.Write([]byte(responseBody))
+					require.NoError(t, err)
+				default:
+					t.Fatalf("tried to call provider at unexpected endpoint: %s", r.URL.Path)
+				}
+			}), nil)
+			testServerPool, err := cert.NewPoolFromBytes(testServerCA)
+			require.NoError(t, err)
+
+			subject := New(ProviderConfig{
+				OAuth2Config: &oauth2.Config{
+					ClientID:     "fake-client-id",
+					ClientSecret: "fake-client-secret",
+					Scopes:       []string{"scope1", "scope2"},
+					Endpoint: oauth2.Endpoint{
+						AuthURL:   "https://fake-auth-url",
+						TokenURL:  testServer.URL + test.tokenEndpointPath,
+						AuthStyle: oauth2.AuthStyleInParams,
+					},
+				},
+				HttpClient: &http.Client{
+					Timeout: 10 * time.Second,
+					Transport: &http.Transport{TLSClientConfig: &tls.Config{
+						MinVersion: tls.VersionTLS12,
+						RootCAs:    testServerPool,
+					}},
+				},
+			})
+
+			accessToken, err := subject.ExchangeAuthcode(context.Background(), "fake-authcode", "https://fake-redirect-url")
+			if test.wantErr != "" {
+				require.EqualError(t, err, test.wantErr)
+				require.Empty(t, accessToken)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, fakeGitHubAccessToken, accessToken)
+			}
+		})
+	}
+}
+
 func TestGetUser(t *testing.T) {
+	const idpDisplayName = "idp display name 😀"
+	const encodedIDPDisplayName = "idp+display+name+%F0%9F%98%80"
+
 	ctrl := gomock.NewController(t)
 	t.Cleanup(ctrl.Finish)

@@ -117,7 +227,7 @@ func TestGetUser(t *testing.T) {
 			},
 			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",
+				DownstreamSubject: fmt.Sprintf("https://some-url?idpName=%s&login=some-github-login&id=some-github-id", encodedIDPDisplayName),
 			},
 		},
 		{
@@ -137,7 +247,7 @@ func TestGetUser(t *testing.T) {
 			},
 			wantUser: &upstreamprovider.GitHubUser{
 				Username:          "some-github-login",
-				DownstreamSubject: "https://some-url?idpName=TODO_IDP_DISPLAY_NAME&login=some-github-login&id=some-github-id",
+				DownstreamSubject: fmt.Sprintf("https://some-url?idpName=%s&login=some-github-login&id=some-github-id", encodedIDPDisplayName),
 			},
 		},
 		{
@@ -157,7 +267,7 @@ func TestGetUser(t *testing.T) {
 			},
 			wantUser: &upstreamprovider.GitHubUser{
 				Username:          "some-github-id",
-				DownstreamSubject: "https://some-url?idpName=TODO_IDP_DISPLAY_NAME&login=some-github-login&id=some-github-id",
+				DownstreamSubject: fmt.Sprintf("https://some-url?idpName=%s&login=some-github-login&id=some-github-id", encodedIDPDisplayName),
 			},
 		},
 		{
@@ -178,7 +288,7 @@ func TestGetUser(t *testing.T) {
 			},
 			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",
+				DownstreamSubject: fmt.Sprintf("https://some-url?idpName=%s&login=some-github-login&id=some-github-id", encodedIDPDisplayName),
 			},
 		},
 		{
@@ -213,7 +323,7 @@ func TestGetUser(t *testing.T) {
 					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{
+				mockGitHubInterface.EXPECT().GetTeamMembership(someContext, sets.New[string]("allowed-org1", "allowed-org2")).Return([]githubclient.TeamInfo{
 					{
 						Name: "org1-team1-name",
 						Slug: "org1-team1-slug",
@@ -234,7 +344,7 @@ func TestGetUser(t *testing.T) {
 			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",
+				DownstreamSubject: fmt.Sprintf("https://some-url?idpName=%s&login=some-github-login&id=some-github-id", encodedIDPDisplayName),
 			},
 		},
 		{
@@ -252,7 +362,7 @@ func TestGetUser(t *testing.T) {
 					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{
+				mockGitHubInterface.EXPECT().GetTeamMembership(someContext, sets.New[string]("allowed-org1", "allowed-org2")).Return([]githubclient.TeamInfo{
 					{
 						Name: "org1-team1-name",
 						Slug: "org1-team1-slug",
@@ -273,7 +383,7 @@ func TestGetUser(t *testing.T) {
 			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",
+				DownstreamSubject: fmt.Sprintf("https://some-url?idpName=%s&login=some-github-login&id=some-github-id", encodedIDPDisplayName),
 			},
 		},
 		{
@@ -299,8 +409,9 @@ func TestGetUser(t *testing.T) {
 		{
 			name: "returns errors from githubClient.GetOrgMembership()",
 			providerConfig: ProviderConfig{
-				APIBaseURL: "https://some-url",
-				HttpClient: someHttpClient,
+				APIBaseURL:        "https://some-url",
+				HttpClient:        someHttpClient,
+				UsernameAttribute: supervisoridpv1alpha1.GitHubUsernameLoginAndID,
 			},
 			buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) {
 				mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{}, nil)
@@ -311,8 +422,9 @@ func TestGetUser(t *testing.T) {
 		{
 			name: "returns errors from githubClient.GetTeamMembership()",
 			providerConfig: ProviderConfig{
-				APIBaseURL: "https://some-url",
-				HttpClient: someHttpClient,
+				APIBaseURL:        "https://some-url",
+				HttpClient:        someHttpClient,
+				UsernameAttribute: supervisoridpv1alpha1.GitHubUsernameLoginAndID,
 			},
 			buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) {
 				mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{}, nil)
@@ -321,6 +433,45 @@ func TestGetUser(t *testing.T) {
 			},
 			wantErr: "error from githubClient.GetTeamMembership",
 		},
+		{
+			name: "bad configuration: UsernameAttribute",
+			providerConfig: ProviderConfig{
+				APIBaseURL:        "https://some-url",
+				HttpClient:        someHttpClient,
+				UsernameAttribute: "this-is-not-legal-value-from-the-enum",
+			},
+			buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) {
+				mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{
+					Login: "some-github-login",
+					ID:    "some-github-id",
+				}, nil)
+			},
+			wantErr: "bad configuration: unknown GitHub username attribute: this-is-not-legal-value-from-the-enum",
+		},
+		{
+			name: "bad configuration: GroupNameAttribute",
+			providerConfig: ProviderConfig{
+				APIBaseURL:         "https://some-url",
+				HttpClient:         someHttpClient,
+				UsernameAttribute:  supervisoridpv1alpha1.GitHubUsernameLoginAndID,
+				GroupNameAttribute: "this-is-not-legal-value-from-the-enum",
+			},
+			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([]githubclient.TeamInfo{
+					{
+						Name: "org1-team1-name",
+						Slug: "org1-team1-slug",
+						Org:  "org1-name",
+					},
+				}, nil)
+			},
+			wantErr: "bad configuration: unknown GitHub group name attribute: this-is-not-legal-value-from-the-enum",
+		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
@@ -341,7 +492,7 @@ func TestGetUser(t *testing.T) {
 				return mockGitHubInterface, test.buildGitHubClientError
 			}

-			actualUser, actualErr := p.GetUser(context.Background(), accessToken)
+			actualUser, actualErr := p.GetUser(context.Background(), accessToken, idpDisplayName)
 			if test.wantErr != "" {
 				require.EqualError(t, actualErr, test.wantErr)
 				require.Nil(t, actualUser)