coder · Emyrk · Jan 29, 2024 · Jan 25, 2024 · Jan 25, 2024 · Jan 25, 2024
diff --git a/coderd/coderdtest/oidctest/idp.go b/coderd/coderdtest/oidctest/idp.go
@@ -16,6 +16,7 @@ import (
 	"net/http"
 	"net/http/cookiejar"
 	"net/http/httptest"
+	"net/http/httputil"
 	"net/url"
 	"strconv"
 	"strings"
@@ -67,6 +68,9 @@ type FakeIDP struct {
 	handler   http.Handler
 	cfg       *oauth2.Config
 
+	// callbackPath allows changing where the callback path to coderd is expected.
+	// This only affects using the Login helper functions.
+	callbackPath string
 	// clientID to be used by coderd
 	clientID     string
 	clientSecret string
@@ -161,6 +165,12 @@ func WithDefaultExpire(d time.Duration) func(*FakeIDP) {
 	}
 }
 
+func WithCallbackPath(path string) func(*FakeIDP) {
+	return func(f *FakeIDP) {
+		f.callbackPath = path
+	}
+}
+
 func WithStaticCredentials(id, secret string) func(*FakeIDP) {
 	return func(f *FakeIDP) {
 		if id != "" {
@@ -369,6 +379,12 @@ func (f *FakeIDP) Login(t testing.TB, client *codersdk.Client, idTokenClaims jwt
 	t.Helper()
 
 	client, resp := f.AttemptLogin(t, client, idTokenClaims, opts...)
+	if resp.StatusCode != http.StatusOK {
+		data, err := httputil.DumpResponse(resp, true)
+		if err == nil {
+			t.Logf("Attempt Login response payload\n%s", string(data))
+		}
+	}
 	require.Equal(t, http.StatusOK, resp.StatusCode, "client failed to login")
 	return client, resp
 }
@@ -398,7 +414,11 @@ func (f *FakeIDP) AttemptLogin(t testing.TB, client *codersdk.Client, idTokenCla
 func (f *FakeIDP) LoginWithClient(t testing.TB, client *codersdk.Client, idTokenClaims jwt.MapClaims, opts ...func(r *http.Request)) (*codersdk.Client, *http.Response) {
 	t.Helper()
 
-	coderOauthURL, err := client.URL.Parse("/api/v2/users/oidc/callback")
+	path := "/api/v2/users/oidc/callback"
+	if f.callbackPath != "" {
+		path = f.callbackPath
+	}
+	coderOauthURL, err := client.URL.Parse(path)
 	require.NoError(t, err)
 	f.SetRedirect(t, coderOauthURL.String())
 

diff --git a/coderd/userauth.go b/coderd/userauth.go
@@ -604,6 +604,25 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// If we have a nil GitHub ID, that is a big problem. That would mean we link
+	// this user and all other users with this bug to the same uuid.
+	// We should instead throw an error. This should never occur in production.
+	//
+	// Verified that the lowest ID on GitHub is "1", so 0 should never occur.
+	if ghUser.GetID() == 0 {
+		httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
+			Message: "The GitHub user ID is missing, this should never happen. Please report this error.",
+			// If this happens, the User could either be:
+			//  - Empty, in which case all these fields would also be empty.
+			//  - Not a user, in which case the "Type" would be something other than "User"
+			Detail: fmt.Sprintf("Other user fields: name=%q, email=%q, type=%q",
+				ghUser.GetName(),
+				ghUser.GetEmail(),
+				ghUser.GetType(),
+			),
+		})
+		return
+	}
 	user, link, err := findLinkedUser(ctx, api.Database, githubLinkedID(ghUser), verifiedEmail.GetEmail())
 	if err != nil {
 		logger.Error(ctx, "oauth2: unable to find linked user", slog.F("gh_user", ghUser.Name), slog.Error(err))

diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go
@@ -14,6 +14,7 @@ import (
 	"github.com/golang-jwt/jwt/v4"
 	"github.com/google/go-github/v43/github"
 	"github.com/google/uuid"
+	"github.com/prometheus/client_golang/prometheus"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/xerrors"
 
@@ -25,6 +26,7 @@ import (
 	"github.com/coder/coder/v2/coderd/database"
 	"github.com/coder/coder/v2/coderd/database/dbgen"
 	"github.com/coder/coder/v2/coderd/database/dbtestutil"
+	"github.com/coder/coder/v2/coderd/promoauth"
 	"github.com/coder/coder/v2/codersdk"
 	"github.com/coder/coder/v2/testutil"
 )
@@ -205,6 +207,7 @@ func TestUserOAuth2Github(t *testing.T) {
 				},
 				AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
 					return &github.User{
+						ID:    github.Int64(100),
 						Login: github.String("kyle"),
 					}, nil
 				},
@@ -264,7 +267,9 @@ func TestUserOAuth2Github(t *testing.T) {
 					}}, nil
 				},
 				AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
-					return &github.User{}, nil
+					return &github.User{
+						ID: github.Int64(100),
+					}, nil
 				},
 				ListEmails: func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error) {
 					return []*github.UserEmail{{
@@ -295,7 +300,9 @@ func TestUserOAuth2Github(t *testing.T) {
 					}}, nil
 				},
 				AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
-					return &github.User{}, nil
+					return &github.User{
+						ID: github.Int64(100),
+					}, nil
 				},
 				ListEmails: func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error) {
 					return []*github.UserEmail{{
@@ -390,6 +397,7 @@ func TestUserOAuth2Github(t *testing.T) {
 				},
 				AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
 					return &github.User{
+						ID:    github.Int64(100),
 						Login: github.String("kyle"),
 					}, nil
 				},
@@ -442,6 +450,7 @@ func TestUserOAuth2Github(t *testing.T) {
 				},
 				AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
 					return &github.User{
+						ID:    github.Int64(100),
 						Login: github.String("mathias"),
 					}, nil
 				},
@@ -494,6 +503,7 @@ func TestUserOAuth2Github(t *testing.T) {
 				},
 				AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
 					return &github.User{
+						ID:    github.Int64(100),
 						Login: github.String("mathias"),
 					}, nil
 				},
@@ -532,6 +542,7 @@ func TestUserOAuth2Github(t *testing.T) {
 				},
 				AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
 					return &github.User{
+						ID:    github.Int64(100),
 						Login: github.String("mathias"),
 					}, nil
 				},
@@ -574,6 +585,7 @@ func TestUserOAuth2Github(t *testing.T) {
 				},
 				AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
 					return &github.User{
+						ID:    github.Int64(100),
 						Login: github.String("kyle"),
 					}, nil
 				},
@@ -591,6 +603,87 @@ func TestUserOAuth2Github(t *testing.T) {
 
 		require.Equal(t, http.StatusUnauthorized, resp.StatusCode)
 	})
+	t.Run("ChangedEmail", func(t *testing.T) {
+		t.Parallel()
+
+		fake := oidctest.NewFakeIDP(t,
+			oidctest.WithServing(),
+			oidctest.WithCallbackPath("/api/v2/users/oauth2/github/callback"),
+		)
+		const ghID = int64(7777)
+		auditor := audit.NewMock()
+		coderEmail := &github.UserEmail{
+			Email:    github.String("alice@coder.com"),
+			Verified: github.Bool(true),
+			Primary:  github.Bool(true),
+		}
+		gmailEmail := &github.UserEmail{
+			Email:    github.String("alice@gmail.com"),
+			Verified: github.Bool(true),
+			Primary:  github.Bool(false),
+		}
+		emails := []*github.UserEmail{
+			gmailEmail,
+			coderEmail,
+		}
+
+		client := coderdtest.New(t, &coderdtest.Options{
+			Auditor: auditor,
+			GithubOAuth2Config: &coderd.GithubOAuth2Config{
+				AllowSignups:  true,
+				AllowEveryone: true,
+				OAuth2Config:  promoauth.NewFactory(prometheus.NewRegistry()).NewGithub("test-github", fake.OIDCConfig(t, []string{})),
+				ListOrganizationMemberships: func(ctx context.Context, client *http.Client) ([]*github.Membership, error) {
+					return []*github.Membership{}, nil
+				},
+				TeamMembership: func(ctx context.Context, client *http.Client, org, team, username string) (*github.Membership, error) {
+					return nil, xerrors.New("no teams")
+				},
+				AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
+					return &github.User{
+						Login: github.String("alice"),
+						ID:    github.Int64(ghID),
+					}, nil
+				},
+				ListEmails: func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error) {
+					return emails, nil
+				},
+			},
+		})
+
+		ctx := testutil.Context(t, testutil.WaitMedium)
+		// This should register the user
+		client, _ = fake.Login(t, client, jwt.MapClaims{})
+		user, err := client.User(ctx, "me")
+		require.NoError(t, err)
+		userID := user.ID
+		require.Equal(t, user.Email, *coderEmail.Email)
+
+		// Now the user is registered, let's change their primary email.
+		coderEmail.Primary = github.Bool(false)
+		gmailEmail.Primary = github.Bool(true)
+
+		client, _ = fake.Login(t, client, jwt.MapClaims{})
+		user, err = client.User(ctx, "me")
+		require.NoError(t, err)
+		require.Equal(t, user.ID, userID)
+		require.Equal(t, user.Email, *gmailEmail.Email)
+
+		// Entirely change emails.
+		newEmail := "alice@newdomain.com"
+		emails = []*github.UserEmail{
+			{
+				Email:    github.String("alice@newdomain.com"),
+				Primary:  github.Bool(true),
+				Verified: github.Bool(true),
+			},
+		}
+		client, _ = fake.Login(t, client, jwt.MapClaims{})
+		user, err = client.User(ctx, "me")
+		require.NoError(t, err)
+		require.Equal(t, user.ID, userID)
+		require.Equal(t, user.Email, newEmail)
+	})
 }
 
 // nolint:bodyclose