Skip to content

Commit 61fa6e0

Browse files
f0sselCoder Agents
f0ssel
and
Coder Agents
committed
fix(coderd): address review feedback for OIDC email fallback
- Authorize UpdateUserLinkedID with ActionUpdate on the user object, matching the sibling InsertUserLink, instead of ActionUpdatePersonal. - Add a GitHub OAuth test for the errLinkedIDAlreadyBound email-fallback block (previously only the OIDC path was covered). - Add an OIDC issuer-URL-change test documenting the deliberate breaking behavior for existing links recorded under a previous issuer. Co-authored-by: Coder Agents <agents@coder.com>
1 parent edd54fb commit 61fa6e0

3 files changed

Lines changed: 122 additions & 8 deletions

File tree

‎coderd/database/dbauthz/dbauthz.go‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7477,13 +7477,10 @@ func (q *querier) UpdateUserLink(ctx context.Context, arg database.UpdateUserLin
74777477
}
74787478

74797479
func (q *querier) UpdateUserLinkedID(ctx context.Context, arg database.UpdateUserLinkedIDParams) (database.UserLink, error) {
7480-
fetch := func(ctx context.Context, arg database.UpdateUserLinkedIDParams) (database.UserLink, error) {
7481-
return q.db.GetUserLinkByUserIDLoginType(ctx, database.GetUserLinkByUserIDLoginTypeParams{
7482-
UserID: arg.UserID,
7483-
LoginType: arg.LoginType,
7484-
})
7480+
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceUserObject(arg.UserID)); err != nil {
7481+
return database.UserLink{}, err
74857482
}
7486-
return fetchAndQuery(q.log, q.auth, policy.ActionUpdatePersonal, fetch, q.db.UpdateUserLinkedID)(ctx, arg)
7483+
return q.db.UpdateUserLinkedID(ctx, arg)
74877484
}
74887485

74897486
func (q *querier) UpdateUserLoginType(ctx context.Context, arg database.UpdateUserLoginTypeParams) (database.User, error) {

‎coderd/database/dbauthz/dbauthz_test.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3153,9 +3153,8 @@ func (s *MethodTestSuite) TestUser() {
31533153
s.Run("UpdateUserLinkedID", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
31543154
link := testutil.Fake(s.T(), faker, database.UserLink{})
31553155
arg := database.UpdateUserLinkedIDParams{LinkedID: link.LinkedID, UserID: link.UserID, LoginType: link.LoginType}
3156-
dbm.EXPECT().GetUserLinkByUserIDLoginType(gomock.Any(), database.GetUserLinkByUserIDLoginTypeParams{UserID: link.UserID, LoginType: link.LoginType}).Return(link, nil).AnyTimes()
31573156
dbm.EXPECT().UpdateUserLinkedID(gomock.Any(), arg).Return(link, nil).AnyTimes()
3158-
check.Args(arg).Asserts(link, policy.ActionUpdatePersonal).Returns(link)
3157+
check.Args(arg).Asserts(link, policy.ActionUpdate).Returns(link)
31593158
}))
31603159
s.Run("UpdateUserRoles", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
31613160
u := testutil.Fake(s.T(), faker, database.User{RBACRoles: []string{codersdk.RoleTemplateAdmin}})

‎coderd/userauth_test.go‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -386,6 +386,67 @@ func TestUserOAuth2Github(t *testing.T) {
386386

387387
require.Equal(t, http.StatusForbidden, resp.StatusCode)
388388
})
389+
t.Run("EmailFallbackBlockedByExistingLink", func(t *testing.T) {
390+
t.Parallel()
391+
392+
// A victim already has a GitHub link bound to a specific GitHub user
393+
// ID. An attacker authenticates with a different GitHub user ID but
394+
// the victim's verified email. The email fallback must not hand the
395+
// attacker the victim's account, even with signups enabled.
396+
owner, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{
397+
GithubOAuth2Config: &coderd.GithubOAuth2Config{
398+
OAuth2Config: &testutil.OAuth2Config{},
399+
AllowSignups: true,
400+
AllowEveryone: true,
401+
ListOrganizationMemberships: func(_ context.Context, _ *http.Client) ([]*github.Membership, error) {
402+
return []*github.Membership{}, nil
403+
},
404+
TeamMembership: func(_ context.Context, _ *http.Client, _, _, _ string) (*github.Membership, error) {
405+
return nil, xerrors.New("no teams")
406+
},
407+
AuthenticatedUser: func(_ context.Context, _ *http.Client) (*github.User, error) {
408+
// Attacker's GitHub ID differs from the victim's link.
409+
return &github.User{
410+
ID: github.Int64(200),
411+
Login: github.String("attacker"),
412+
Name: github.String("Attacker"),
413+
}, nil
414+
},
415+
ListEmails: func(_ context.Context, _ *http.Client) ([]*github.UserEmail, error) {
416+
return []*github.UserEmail{{
417+
Email: github.String("victim@coder.com"),
418+
Verified: github.Bool(true),
419+
Primary: github.Bool(true),
420+
}}, nil
421+
},
422+
},
423+
})
424+
425+
// Seed the victim with an existing GitHub link (a different linked_id).
426+
victim := dbgen.User(t, db, database.User{
427+
Email: "victim@coder.com",
428+
LoginType: database.LoginTypeGithub,
429+
})
430+
const victimLinkedID = "100"
431+
dbgen.UserLink(t, db, database.UserLink{
432+
UserID: victim.ID,
433+
LoginType: database.LoginTypeGithub,
434+
LinkedID: victimLinkedID,
435+
})
436+
437+
resp := oauth2Callback(t, owner)
438+
require.Equal(t, http.StatusForbidden, resp.StatusCode,
439+
"attacker with a different GitHub ID must not authenticate as the victim")
440+
441+
// The victim's link must be untouched.
442+
victimLink, err := db.GetUserLinkByUserIDLoginType(dbauthz.AsSystemRestricted(context.Background()), database.GetUserLinkByUserIDLoginTypeParams{
443+
UserID: victim.ID,
444+
LoginType: database.LoginTypeGithub,
445+
})
446+
require.NoError(t, err)
447+
require.Equal(t, victimLinkedID, victimLink.LinkedID,
448+
"victim's linked_id must remain unchanged")
449+
})
389450
t.Run("Signup", func(t *testing.T) {
390451
t.Parallel()
391452
auditor := audit.NewMock()
@@ -1805,6 +1866,63 @@ func TestUserOIDC(t *testing.T) {
18051866
"linked_id should be backfilled with the correct value after login")
18061867
})
18071868

1869+
// Tests that changing the OIDC issuer URL blocks an existing user whose
1870+
// linked_id was recorded under the old issuer. This is a deliberate
1871+
// breaking change: before this fix the email fallback silently rescued
1872+
// such users. Now the login is rejected because the existing link's
1873+
// linked_id (old issuer) differs from the newly computed one (new issuer).
1874+
t.Run("OIDCEmailFallbackBlockedByIssuerChange", func(t *testing.T) {
1875+
t.Parallel()
1876+
ctx := testutil.Context(t, testutil.WaitShort)
1877+
1878+
fake := oidctest.NewFakeIDP(t,
1879+
oidctest.WithRefresh(func(_ string) error {
1880+
return xerrors.New("refreshing token should never occur")
1881+
}),
1882+
oidctest.WithServing(),
1883+
)
1884+
cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) {
1885+
cfg.AllowSignups = true
1886+
})
1887+
1888+
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug)
1889+
owner, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{
1890+
OIDCConfig: cfg,
1891+
Logger: &logger,
1892+
})
1893+
1894+
// Seed a user whose link was created under a different (old) issuer
1895+
// but with the same subject the IdP presents on login.
1896+
user := dbgen.User(t, db, database.User{
1897+
LoginType: database.LoginTypeOIDC,
1898+
})
1899+
const sub = "stable-subject"
1900+
oldLinkedID := "https://old-issuer.example.com||" + sub
1901+
dbgen.UserLink(t, db, database.UserLink{
1902+
UserID: user.ID,
1903+
LoginType: database.LoginTypeOIDC,
1904+
LinkedID: oldLinkedID,
1905+
})
1906+
1907+
// Login presents the same subject but the current issuer, so the
1908+
// computed linked_id differs from the stored one and is blocked.
1909+
_, resp := fake.AttemptLogin(t, owner, jwt.MapClaims{
1910+
"email": user.Email,
1911+
"sub": sub,
1912+
})
1913+
require.Equal(t, http.StatusForbidden, resp.StatusCode,
1914+
"issuer change must block the email fallback for an existing link")
1915+
1916+
// The stored link must remain unchanged.
1917+
link, err := db.GetUserLinkByUserIDLoginType(dbauthz.AsSystemRestricted(ctx), database.GetUserLinkByUserIDLoginTypeParams{
1918+
UserID: user.ID,
1919+
LoginType: database.LoginTypeOIDC,
1920+
})
1921+
require.NoError(t, err)
1922+
require.Equal(t, oldLinkedID, link.LinkedID,
1923+
"linked_id must not be modified when the login is blocked")
1924+
})
1925+
18081926
t.Run("OIDCConvert", func(t *testing.T) {
18091927
t.Parallel()
18101928

0 commit comments

Comments
 (0)