Skip to content

Commit 04a2326

Browse files
EmyrkEmyrk
authored
chore: ensure github uids are unique (#11826)
1 parent d66e6e7 commit 04a2326

File tree

3 files changed

+135
-3
lines changed

3 files changed

+135
-3
lines changed

‎coderd/coderdtest/oidctest/idp.go

Lines changed: 21 additions & 1 deletion
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.Logf("Attempt Login response payload\n%s", 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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -604,6 +604,25 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
604604
return
605605
}
606606

607+
// If we have a nil GitHub ID, that is a big problem. That would mean we link
608+
// this user and all other users with this bug to the same uuid.
609+
// We should instead throw an error. This should never occur in production.
610+
//
611+
// Verified that the lowest ID on GitHub is "1", so 0 should never occur.
612+
if ghUser.GetID() == 0 {
613+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
614+
Message: "The GitHub user ID is missing, this should never happen. Please report this error.",
615+
// If this happens, the User could either be:
616+
// - Empty, in which case all these fields would also be empty.
617+
// - Not a user, in which case the "Type" would be something other than "User"
618+
Detail: fmt.Sprintf("Other user fields: name=%q, email=%q, type=%q",
619+
ghUser.GetName(),
620+
ghUser.GetEmail(),
621+
ghUser.GetType(),
622+
),
623+
})
624+
return
625+
}
607626
user, link, err := findLinkedUser(ctx, api.Database, githubLinkedID(ghUser), verifiedEmail.GetEmail())
608627
if err != nil {
609628
logger.Error(ctx, "oauth2: unable to find linked user", slog.F("gh_user", ghUser.Name), slog.Error(err))

‎coderd/userauth_test.go

Lines changed: 95 additions & 2 deletions
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
)
@@ -205,6 +207,7 @@ func TestUserOAuth2Github(t *testing.T) {
205207
},
206208
AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
207209
return &github.User{
210+
ID: github.Int64(100),
208211
Login: github.String("kyle"),
209212
}, nil
210213
},
@@ -264,7 +267,9 @@ func TestUserOAuth2Github(t *testing.T) {
264267
}}, nil
265268
},
266269
AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
267-
return &github.User{}, nil
270+
return &github.User{
271+
ID: github.Int64(100),
272+
}, nil
268273
},
269274
ListEmails: func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error) {
270275
return []*github.UserEmail{{
@@ -295,7 +300,9 @@ func TestUserOAuth2Github(t *testing.T) {
295300
}}, nil
296301
},
297302
AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
298-
return &github.User{}, nil
303+
return &github.User{
304+
ID: github.Int64(100),
305+
}, nil
299306
},
300307
ListEmails: func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error) {
301308
return []*github.UserEmail{{
@@ -390,6 +397,7 @@ func TestUserOAuth2Github(t *testing.T) {
390397
},
391398
AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
392399
return &github.User{
400+
ID: github.Int64(100),
393401
Login: github.String("kyle"),
394402
}, nil
395403
},
@@ -442,6 +450,7 @@ func TestUserOAuth2Github(t *testing.T) {
442450
},
443451
AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
444452
return &github.User{
453+
ID: github.Int64(100),
445454
Login: github.String("mathias"),
446455
}, nil
447456
},
@@ -494,6 +503,7 @@ func TestUserOAuth2Github(t *testing.T) {
494503
},
495504
AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
496505
return &github.User{
506+
ID: github.Int64(100),
497507
Login: github.String("mathias"),
498508
}, nil
499509
},
@@ -532,6 +542,7 @@ func TestUserOAuth2Github(t *testing.T) {
532542
},
533543
AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
534544
return &github.User{
545+
ID: github.Int64(100),
535546
Login: github.String("mathias"),
536547
}, nil
537548
},
@@ -574,6 +585,7 @@ func TestUserOAuth2Github(t *testing.T) {
574585
},
575586
AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
576587
return &github.User{
588+
ID: github.Int64(100),
577589
Login: github.String("kyle"),
578590
}, nil
579591
},
@@ -591,6 +603,87 @@ func TestUserOAuth2Github(t *testing.T) {
591603

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

596689
// nolint:bodyclose

0 commit comments

Comments
 (0)