From de93f283628067018976d63fe1e6feee39b0d043 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 25 Jan 2024 12:44:53 -0600 Subject: [PATCH 1/6] fix: github changing emails should update coder emails on login Github Oauth login matches on github user id, not their email --- coderd/coderdtest/oidctest/idp.go | 22 +++++++- coderd/userauth.go | 13 ++++- coderd/userauth_test.go | 83 +++++++++++++++++++++++++++++++ 3 files changed, 116 insertions(+), 2 deletions(-) diff --git a/coderd/coderdtest/oidctest/idp.go b/coderd/coderdtest/oidctest/idp.go index 044db86ce0dc6..4caa9948b0729 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.Log(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..001ab92059cc0 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -604,7 +604,18 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { return } - user, link, err := findLinkedUser(ctx, api.Database, githubLinkedID(ghUser), verifiedEmail.GetEmail()) + // We intentionally omit the email here. Users should be found through the + // github uuid. If the id is nil, we should never link on the uuid 0. + // This would be a big problem, and should never happen. + if ghUser.GetID() == 0 { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Github user ID is missing.", + }) + return + } + + ghid := githubLinkedID(ghUser) + user, link, err := findLinkedUser(ctx, api.Database, ghid) if err != nil { logger.Error(ctx, "oauth2: unable to find linked user", slog.F("gh_user", ghUser.Name), slog.Error(err)) httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index fe6ded1e901b1..d0254a7cd4ea4 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" ) @@ -591,6 +593,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 From 8077aa69f2e57e3c3aef767ea768ecbd6d2ad48c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 25 Jan 2024 12:51:00 -0600 Subject: [PATCH 2/6] fix was not actually a fix, revert The test written still passes, will keep looking --- coderd/userauth.go | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 001ab92059cc0..e6819c3ffb721 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -604,18 +604,7 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { return } - // We intentionally omit the email here. Users should be found through the - // github uuid. If the id is nil, we should never link on the uuid 0. - // This would be a big problem, and should never happen. - if ghUser.GetID() == 0 { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Github user ID is missing.", - }) - return - } - - ghid := githubLinkedID(ghUser) - user, link, err := findLinkedUser(ctx, api.Database, ghid) + 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)) httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ From bb2bdf17139c1be5a13d4b2592c6cf9972231104 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 25 Jan 2024 13:05:08 -0600 Subject: [PATCH 3/6] fix: ensure all linked accounts use a github id --- coderd/userauth.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/coderd/userauth.go b/coderd/userauth.go index e6819c3ffb721..90d5e469adc5d 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 returned an ID of 0, 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)) From 36b31c368e264d5ccacd9a3576278098b51648b5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 26 Jan 2024 11:57:12 -0600 Subject: [PATCH 4/6] fix unit testS --- coderd/userauth_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index d0254a7cd4ea4..01a806feabd21 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -207,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 }, @@ -392,6 +393,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 }, @@ -444,6 +446,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 }, @@ -496,6 +499,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 }, @@ -534,6 +538,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 }, @@ -576,6 +581,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 }, From 0033d9b665a7fc550840fa03df452f2862d9321d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 26 Jan 2024 12:25:39 -0600 Subject: [PATCH 5/6] github uids to all tests --- coderd/userauth_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 01a806feabd21..e502b7b4cc780 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -267,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{{ @@ -298,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{{ From 5821971f9cc19a2aa9283a5745b89a358381bd70 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 29 Jan 2024 08:53:26 -0600 Subject: [PATCH 6/6] Fix language --- coderd/coderdtest/oidctest/idp.go | 2 +- coderd/userauth.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/coderdtest/oidctest/idp.go b/coderd/coderdtest/oidctest/idp.go index 4caa9948b0729..cc0fe97434ee1 100644 --- a/coderd/coderdtest/oidctest/idp.go +++ b/coderd/coderdtest/oidctest/idp.go @@ -382,7 +382,7 @@ func (f *FakeIDP) Login(t testing.TB, client *codersdk.Client, idTokenClaims jwt if resp.StatusCode != http.StatusOK { data, err := httputil.DumpResponse(resp, true) if err == nil { - t.Log(string(data)) + t.Logf("Attempt Login response payload\n%s", string(data)) } } require.Equal(t, http.StatusOK, resp.StatusCode, "client failed to login") diff --git a/coderd/userauth.go b/coderd/userauth.go index 90d5e469adc5d..37a0402fa5713 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -611,7 +611,7 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { // 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 returned an ID of 0, this should never happen. Please report this error.", + 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"