Skip to content

Commit 67c272a

Browse files
EmyrkEmyrk
committed
fix: github changing emails should update coder emails on login
Github Oauth login matches on github user id, not their email
1 parent ff9959f commit 67c272a

File tree

3 files changed

+116
-2
lines changed

3 files changed

+116
-2
lines changed

‎coderd/coderdtest/oidctest/idp.go

+21-1
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"net/http"
1717
"net/http/cookiejar"
1818
"net/http/httptest"
19+
"net/http/httputil"
1920
"net/url"
2021
"strconv"
2122
"strings"
@@ -67,6 +68,9 @@ type FakeIDP struct {
6768
handler http.Handler
6869
cfg *oauth2.Config
6970

71+
// callbackPath allows changing where the callback path to coderd is expected.
72+
// This only affects using the Login helper functions.
73+
callbackPath string
7074
// clientID to be used by coderd
7175
clientID string
7276
clientSecret string
@@ -161,6 +165,12 @@ func WithDefaultExpire(d time.Duration) func(*FakeIDP) {
161165
}
162166
}
163167

168+
func WithCallbackPath(path string) func(*FakeIDP) {
169+
return func(f *FakeIDP) {
170+
f.callbackPath = path
171+
}
172+
}
173+
164174
func WithStaticCredentials(id, secret string) func(*FakeIDP) {
165175
return func(f *FakeIDP) {
166176
if id != "" {
@@ -369,6 +379,12 @@ func (f *FakeIDP) Login(t testing.TB, client *codersdk.Client, idTokenClaims jwt
369379
t.Helper()
370380

371381
client, resp := f.AttemptLogin(t, client, idTokenClaims, opts...)
382+
if resp.StatusCode != http.StatusOK {
383+
data, err := httputil.DumpResponse(resp, true)
384+
if err == nil {
385+
t.Log(string(data))
386+
}
387+
}
372388
require.Equal(t, http.StatusOK, resp.StatusCode, "client failed to login")
373389
return client, resp
374390
}
@@ -398,7 +414,11 @@ func (f *FakeIDP) AttemptLogin(t testing.TB, client *codersdk.Client, idTokenCla
398414
func (f *FakeIDP) LoginWithClient(t testing.TB, client *codersdk.Client, idTokenClaims jwt.MapClaims, opts ...func(r *http.Request)) (*codersdk.Client, *http.Response) {
399415
t.Helper()
400416

401-
coderOauthURL, err := client.URL.Parse("/api/v2/users/oidc/callback")
417+
path := "/api/v2/users/oidc/callback"
418+
if f.callbackPath != "" {
419+
path = f.callbackPath
420+
}
421+
coderOauthURL, err := client.URL.Parse(path)
402422
require.NoError(t, err)
403423
f.SetRedirect(t, coderOauthURL.String())
404424

‎coderd/userauth.go

+12-1
Original file line numberDiff line numberDiff line change
@@ -604,7 +604,18 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
604604
return
605605
}
606606

607-
user, link, err := findLinkedUser(ctx, api.Database, githubLinkedID(ghUser), verifiedEmail.GetEmail())
607+
// We intentionally omit the email here. Users should be found through the
608+
// github uuid. If the id is nil, we should never link on the uuid 0.
609+
// This would be a big problem, and should never happen.
610+
if ghUser.GetID() == 0 {
611+
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
612+
Message: "Github user ID is missing.",
613+
})
614+
return
615+
}
616+
617+
ghid := githubLinkedID(ghUser)
618+
user, link, err := findLinkedUser(ctx, api.Database, ghid)
608619
if err != nil {
609620
logger.Error(ctx, "oauth2: unable to find linked user", slog.F("gh_user", ghUser.Name), slog.Error(err))
610621
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{

‎coderd/userauth_test.go

+83
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"github.com/golang-jwt/jwt/v4"
1515
"github.com/google/go-github/v43/github"
1616
"github.com/google/uuid"
17+
"github.com/prometheus/client_golang/prometheus"
1718
"github.com/stretchr/testify/require"
1819
"golang.org/x/xerrors"
1920

@@ -25,6 +26,7 @@ import (
2526
"github.com/coder/coder/v2/coderd/database"
2627
"github.com/coder/coder/v2/coderd/database/dbgen"
2728
"github.com/coder/coder/v2/coderd/database/dbtestutil"
29+
"github.com/coder/coder/v2/coderd/promoauth"
2830
"github.com/coder/coder/v2/codersdk"
2931
"github.com/coder/coder/v2/testutil"
3032
)
@@ -591,6 +593,87 @@ func TestUserOAuth2Github(t *testing.T) {
591593

592594
require.Equal(t, http.StatusUnauthorized, resp.StatusCode)
593595
})
596+
t.Run("ChangedEmail", func(t *testing.T) {
597+
t.Parallel()
598+
599+
fake := oidctest.NewFakeIDP(t,
600+
oidctest.WithServing(),
601+
oidctest.WithCallbackPath("/api/v2/users/oauth2/github/callback"),
602+
)
603+
const ghID = int64(7777)
604+
auditor := audit.NewMock()
605+
coderEmail := &github.UserEmail{
606+
Email: github.String("alice@coder.com"),
607+
Verified: github.Bool(true),
608+
Primary: github.Bool(true),
609+
}
610+
gmailEmail := &github.UserEmail{
611+
Email: github.String("alice@gmail.com"),
612+
Verified: github.Bool(true),
613+
Primary: github.Bool(false),
614+
}
615+
emails := []*github.UserEmail{
616+
gmailEmail,
617+
coderEmail,
618+
}
619+
620+
client := coderdtest.New(t, &coderdtest.Options{
621+
Auditor: auditor,
622+
GithubOAuth2Config: &coderd.GithubOAuth2Config{
623+
AllowSignups: true,
624+
AllowEveryone: true,
625+
OAuth2Config: promoauth.NewFactory(prometheus.NewRegistry()).NewGithub("test-github", fake.OIDCConfig(t, []string{})),
626+
ListOrganizationMemberships: func(ctx context.Context, client *http.Client) ([]*github.Membership, error) {
627+
return []*github.Membership{}, nil
628+
},
629+
TeamMembership: func(ctx context.Context, client *http.Client, org, team, username string) (*github.Membership, error) {
630+
return nil, xerrors.New("no teams")
631+
},
632+
AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
633+
return &github.User{
634+
Login: github.String("alice"),
635+
ID: github.Int64(ghID),
636+
}, nil
637+
},
638+
ListEmails: func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error) {
639+
return emails, nil
640+
},
641+
},
642+
})
643+
644+
ctx := testutil.Context(t, testutil.WaitMedium)
645+
// This should register the user
646+
client, _ = fake.Login(t, client, jwt.MapClaims{})
647+
user, err := client.User(ctx, "me")
648+
require.NoError(t, err)
649+
userID := user.ID
650+
require.Equal(t, user.Email, *coderEmail.Email)
651+
652+
// Now the user is registered, let's change their primary email.
653+
coderEmail.Primary = github.Bool(false)
654+
gmailEmail.Primary = github.Bool(true)
655+
656+
client, _ = fake.Login(t, client, jwt.MapClaims{})
657+
user, err = client.User(ctx, "me")
658+
require.NoError(t, err)
659+
require.Equal(t, user.ID, userID)
660+
require.Equal(t, user.Email, *gmailEmail.Email)
661+
662+
// Entirely change emails.
663+
newEmail := "alice@newdomain.com"
664+
emails = []*github.UserEmail{
665+
{
666+
Email: github.String("alice@newdomain.com"),
667+
Primary: github.Bool(true),
668+
Verified: github.Bool(true),
669+
},
670+
}
671+
client, _ = fake.Login(t, client, jwt.MapClaims{})
672+
user, err = client.User(ctx, "me")
673+
require.NoError(t, err)
674+
require.Equal(t, user.ID, userID)
675+
require.Equal(t, user.Email, newEmail)
676+
})
594677
}
595678

596679
// nolint:bodyclose

0 commit comments

Comments
 (0)