From ff9959fe7e49ae13d7e8923a99d4fd52f28d100f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 25 Jan 2024 11:33:47 -0600 Subject: [PATCH 1/3] chore: instrument additional githubapi calls This only affects github as a login source, not external auth. --- cli/server.go | 46 +++++++++++++++++++++----------------- coderd/promoauth/oauth2.go | 14 ++++++++++-- 2 files changed, 37 insertions(+), 23 deletions(-) diff --git a/cli/server.go b/cli/server.go index c862769e58b67..4631df82dfc44 100644 --- a/cli/server.go +++ b/cli/server.go @@ -1773,12 +1773,6 @@ func configureGithubOAuth2(instrument *promoauth.Factory, accessURL *url.URL, cl Slug: parts[1], }) } - createClient := func(client *http.Client) (*github.Client, error) { - if enterpriseBaseURL != "" { - return github.NewEnterpriseClient(enterpriseBaseURL, "", client) - } - return github.NewClient(client), nil - } endpoint := xgithub.Endpoint if enterpriseBaseURL != "" { @@ -1800,24 +1794,34 @@ func configureGithubOAuth2(instrument *promoauth.Factory, accessURL *url.URL, cl } } + instrumentedOauth := instrument.NewGithub("github-login", &oauth2.Config{ + ClientID: clientID, + ClientSecret: clientSecret, + Endpoint: endpoint, + RedirectURL: redirectURL.String(), + Scopes: []string{ + "read:user", + "read:org", + "user:email", + }, + }) + + createClient := func(client *http.Client, source promoauth.Oauth2Source) (*github.Client, error) { + client = instrumentedOauth.InstrumentHTTPClient(client, source) + if enterpriseBaseURL != "" { + return github.NewEnterpriseClient(enterpriseBaseURL, "", client) + } + return github.NewClient(client), nil + } + return &coderd.GithubOAuth2Config{ - OAuth2Config: instrument.NewGithub("github-login", &oauth2.Config{ - ClientID: clientID, - ClientSecret: clientSecret, - Endpoint: endpoint, - RedirectURL: redirectURL.String(), - Scopes: []string{ - "read:user", - "read:org", - "user:email", - }, - }), + OAuth2Config: instrumentedOauth, AllowSignups: allowSignups, AllowEveryone: allowEveryone, AllowOrganizations: allowOrgs, AllowTeams: allowTeams, AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) { - api, err := createClient(client) + api, err := createClient(client, promoauth.SourceGitAPIAuthUser) if err != nil { return nil, err } @@ -1825,7 +1829,7 @@ func configureGithubOAuth2(instrument *promoauth.Factory, accessURL *url.URL, cl return user, err }, ListEmails: func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error) { - api, err := createClient(client) + api, err := createClient(client, promoauth.SourceGitAPIListEmails) if err != nil { return nil, err } @@ -1833,7 +1837,7 @@ func configureGithubOAuth2(instrument *promoauth.Factory, accessURL *url.URL, cl return emails, err }, ListOrganizationMemberships: func(ctx context.Context, client *http.Client) ([]*github.Membership, error) { - api, err := createClient(client) + api, err := createClient(client, promoauth.SourceGitAPIOrgMemberships) if err != nil { return nil, err } @@ -1846,7 +1850,7 @@ func configureGithubOAuth2(instrument *promoauth.Factory, accessURL *url.URL, cl return memberships, err }, TeamMembership: func(ctx context.Context, client *http.Client, org, teamSlug, username string) (*github.Membership, error) { - api, err := createClient(client) + api, err := createClient(client, promoauth.SourceGitAPITeamMemberships) if err != nil { return nil, err } diff --git a/coderd/promoauth/oauth2.go b/coderd/promoauth/oauth2.go index 258694563581c..30e5269cd319e 100644 --- a/coderd/promoauth/oauth2.go +++ b/coderd/promoauth/oauth2.go @@ -19,6 +19,11 @@ const ( SourceTokenSource Oauth2Source = "TokenSource" SourceAppInstallations Oauth2Source = "AppInstallations" SourceAuthorizeDevice Oauth2Source = "AuthorizeDevice" + + SourceGitAPIAuthUser Oauth2Source = "GitAPIAuthUser" + SourceGitAPIListEmails Oauth2Source = "GitAPIListEmails" + SourceGitAPIOrgMemberships Oauth2Source = "GitAPIOrgMemberships" + SourceGitAPITeamMemberships Oauth2Source = "GitAPITeamMemberships" ) // OAuth2Config exposes a subset of *oauth2.Config functions for easier testing. @@ -209,6 +214,12 @@ func (c *Config) TokenSource(ctx context.Context, token *oauth2.Token) oauth2.To return c.underlying.TokenSource(c.wrapClient(ctx, SourceTokenSource), token) } +func (c *Config) InstrumentHTTPClient(hc *http.Client, source Oauth2Source) *http.Client { + // The new tripper will instrument every request made by the oauth2 client. + hc.Transport = newInstrumentedTripper(c, source, hc.Transport) + return hc +} + // wrapClient is the only way we can accurately instrument the oauth2 client. // This is because method calls to the 'OAuth2Config' interface are not 1:1 with // network requests. @@ -229,8 +240,7 @@ func (c *Config) oauthHTTPClient(ctx context.Context, source Oauth2Source) *http cli = hc } - // The new tripper will instrument every request made by the oauth2 client. - cli.Transport = newInstrumentedTripper(c, source, cli.Transport) + cli = c.InstrumentHTTPClient(cli, source) return cli } From 67c272a8f1ec2658fc35946cd4ecdfc3b30b5d00 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 25 Jan 2024 12:44:53 -0600 Subject: [PATCH 2/3] 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 23ef0e7fa48bdb4dfcf04f9c65b0d1e6d1d7fcc6 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 25 Jan 2024 12:46:15 -0600 Subject: [PATCH 3/3] Revert "fix: github changing emails should update coder emails on login" This reverts commit 67c272a8f1ec2658fc35946cd4ecdfc3b30b5d00. This commit was intended for another branch --- coderd/coderdtest/oidctest/idp.go | 22 +------- coderd/userauth.go | 13 +---- coderd/userauth_test.go | 83 ------------------------------- 3 files changed, 2 insertions(+), 116 deletions(-) diff --git a/coderd/coderdtest/oidctest/idp.go b/coderd/coderdtest/oidctest/idp.go index 4caa9948b0729..044db86ce0dc6 100644 --- a/coderd/coderdtest/oidctest/idp.go +++ b/coderd/coderdtest/oidctest/idp.go @@ -16,7 +16,6 @@ import ( "net/http" "net/http/cookiejar" "net/http/httptest" - "net/http/httputil" "net/url" "strconv" "strings" @@ -68,9 +67,6 @@ 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 @@ -165,12 +161,6 @@ 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 != "" { @@ -379,12 +369,6 @@ 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 } @@ -414,11 +398,7 @@ 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() - path := "/api/v2/users/oidc/callback" - if f.callbackPath != "" { - path = f.callbackPath - } - coderOauthURL, err := client.URL.Parse(path) + coderOauthURL, err := client.URL.Parse("/api/v2/users/oidc/callback") require.NoError(t, err) f.SetRedirect(t, coderOauthURL.String()) 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{ diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index d0254a7cd4ea4..fe6ded1e901b1 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -14,7 +14,6 @@ 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" @@ -26,7 +25,6 @@ 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" ) @@ -593,87 +591,6 @@ 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