diff --git a/coderd/coderdtest/oidctest/idp.go b/coderd/coderdtest/oidctest/idp.go index 044db86ce0dc6..cc0fe97434ee1 100644 --- 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 index e6819c3ffb721..37a0402fa5713 100644 --- 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 index fe6ded1e901b1..e502b7b4cc780 100644 --- 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