Skip to content

Commit ffe7645

Browse files
jdomeracki-coderf0sselCoder Agents
authored
fix(coderd)!: restrict OIDC email fallback to first-time account linking (#25712) (#26176)
Backport of #25712 to `release/2.34`. Cherry-picked with `git cherry-pick -x` (`53d287a139`); the commit body references the original PR. _Generated by Coder Agents on behalf of @jdomeracki-coder._ Co-authored-by: Garrett Delfosse <garrett@coder.com> Co-authored-by: Coder Agents <agents@coder.com>
1 parent bd5666a commit ffe7645

9 files changed

Lines changed: 454 additions & 5 deletions

File tree

‎coderd/database/dbauthz/dbauthz.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7481,6 +7481,13 @@ func (q *querier) UpdateUserLink(ctx context.Context, arg database.UpdateUserLin
74817481
return fetchAndQuery(q.log, q.auth, policy.ActionUpdatePersonal, fetch, q.db.UpdateUserLink)(ctx, arg)
74827482
}
74837483

7484+
func (q *querier) UpdateUserLinkedID(ctx context.Context, arg database.UpdateUserLinkedIDParams) (database.UserLink, error) {
7485+
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceUserObject(arg.UserID)); err != nil {
7486+
return database.UserLink{}, err
7487+
}
7488+
return q.db.UpdateUserLinkedID(ctx, arg)
7489+
}
7490+
74847491
func (q *querier) UpdateUserLoginType(ctx context.Context, arg database.UpdateUserLoginTypeParams) (database.User, error) {
74857492
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceSystem); err != nil {
74867493
return database.User{}, err

‎coderd/database/dbauthz/dbauthz_test.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3150,6 +3150,12 @@ func (s *MethodTestSuite) TestUser() {
31503150
dbm.EXPECT().UpdateUserLink(gomock.Any(), arg).Return(link, nil).AnyTimes()
31513151
check.Args(arg).Asserts(link, policy.ActionUpdatePersonal).Returns(link)
31523152
}))
3153+
s.Run("UpdateUserLinkedID", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
3154+
link := testutil.Fake(s.T(), faker, database.UserLink{})
3155+
arg := database.UpdateUserLinkedIDParams{LinkedID: link.LinkedID, UserID: link.UserID, LoginType: link.LoginType}
3156+
dbm.EXPECT().UpdateUserLinkedID(gomock.Any(), arg).Return(link, nil).AnyTimes()
3157+
check.Args(arg).Asserts(link, policy.ActionUpdate).Returns(link)
3158+
}))
31533159
s.Run("UpdateUserRoles", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
31543160
u := testutil.Fake(s.T(), faker, database.User{RBACRoles: []string{codersdk.RoleTemplateAdmin}})
31553161
o := u

‎coderd/database/dbmetrics/querymetrics.go‎

Lines changed: 8 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/dbmock/dbmock.go‎

Lines changed: 15 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/querier.go‎

Lines changed: 4 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries.sql.go‎

Lines changed: 35 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/user_links.sql‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,17 @@ SET
5050
WHERE
5151
user_id = $7 AND login_type = $8 RETURNING *;
5252

53+
-- name: UpdateUserLinkedID :one
54+
-- Backfills linked_id for legacy user_links that were created before
55+
-- linked_id tracking was added. Only updates when linked_id is empty
56+
-- to avoid overwriting a valid binding.
57+
UPDATE
58+
user_links
59+
SET
60+
linked_id = @linked_id
61+
WHERE
62+
user_id = @user_id AND login_type = @login_type AND linked_id = '' RETURNING *;
63+
5364
-- name: OIDCClaimFields :many
5465
-- OIDCClaimFields returns a list of distinct keys in the the merged_claims fields.
5566
-- This query is used to generate the list of available sync fields for idp sync settings.

‎coderd/userauth.go‎

Lines changed: 69 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1036,7 +1036,16 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
10361036
})
10371037
return
10381038
}
1039-
user, link, err := findLinkedUser(ctx, api.Database, githubLinkedID(ghUser), verifiedEmail.GetEmail())
1039+
user, link, err := findLinkedUser(ctx, api.Database, githubLinkedID(ghUser), database.LoginTypeGithub, verifiedEmail.GetEmail())
1040+
if errors.Is(err, errLinkedIDAlreadyBound) {
1041+
logger.Warn(ctx, "oauth2: blocked login, account already linked to different identity",
1042+
slog.F("email", verifiedEmail.GetEmail()),
1043+
)
1044+
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
1045+
Message: "This account is already linked to a different identity provider subject.",
1046+
})
1047+
return
1048+
}
10401049
if err != nil {
10411050
logger.Error(ctx, "oauth2: unable to find linked user", slog.F("gh_user", ghUser.Name), slog.Error(err))
10421051
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
@@ -1436,7 +1445,22 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
14361445
}
14371446
ctx = slog.With(ctx, slog.F("email", email), slog.F("username", username), slog.F("name", name))
14381447

1439-
user, link, err := findLinkedUser(ctx, api.Database, oidcLinkedID(idToken), email)
1448+
user, link, err := findLinkedUser(ctx, api.Database, oidcLinkedID(idToken), database.LoginTypeOIDC, email)
1449+
if errors.Is(err, errLinkedIDAlreadyBound) {
1450+
logger.Warn(ctx, "oauth2: blocked login, account already linked to different identity",
1451+
slog.F("email", email),
1452+
)
1453+
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
1454+
Status: http.StatusForbidden,
1455+
HideStatus: true,
1456+
Title: "Account already linked",
1457+
Description: "This account is already linked to a different identity provider subject. Contact your administrator.",
1458+
Actions: []site.Action{
1459+
{URL: "/login", Text: "Back to login"},
1460+
},
1461+
})
1462+
return
1463+
}
14401464
if err != nil {
14411465
logger.Error(ctx, "oauth2: unable to find linked user", slog.F("email", email), slog.Error(err))
14421466
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
@@ -1870,6 +1894,31 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
18701894
if err != nil {
18711895
return xerrors.Errorf("update user link: %w", err)
18721896
}
1897+
1898+
// Defense-in-depth: if a concurrent transaction backfilled
1899+
// linked_id between findLinkedUser and this point, reject the
1900+
// login with a 403 instead of letting it bubble up as a 500.
1901+
if link.LinkedID != "" && link.LinkedID != params.LinkedID {
1902+
return &idpsync.HTTPError{
1903+
Code: http.StatusForbidden,
1904+
Msg: "Account already linked",
1905+
Detail: "This account is already linked to a different identity provider subject. Contact your administrator.",
1906+
RenderStaticPage: true,
1907+
}
1908+
}
1909+
1910+
// Backfill linked_id for legacy links.
1911+
if link.LinkedID == "" && params.LinkedID != "" {
1912+
//nolint:gocritic // System needs to update the user link.
1913+
link, err = tx.UpdateUserLinkedID(dbauthz.AsSystemRestricted(ctx), database.UpdateUserLinkedIDParams{
1914+
LinkedID: params.LinkedID,
1915+
UserID: user.ID,
1916+
LoginType: params.LoginType,
1917+
})
1918+
if err != nil {
1919+
return xerrors.Errorf("backfill user linked id: %w", err)
1920+
}
1921+
}
18731922
}
18741923

18751924
err = api.IDPSync.SyncOrganizations(ctx, tx, user, params.OrganizationSync)
@@ -2090,9 +2139,17 @@ func oidcLinkedID(tok *oidc.IDToken) string {
20902139
return strings.Join([]string{tok.Issuer, tok.Subject}, "||")
20912140
}
20922141

2142+
// errLinkedIDAlreadyBound is returned by findLinkedUser when the user
2143+
// found by email already has a user_link with a different linked_id.
2144+
var errLinkedIDAlreadyBound = xerrors.New("user account is already linked to a different identity provider subject")
2145+
20932146
// findLinkedUser tries to find a user by their unique OAuth-linked ID.
2094-
// If it doesn't not find it, it returns the user by their email.
2095-
func findLinkedUser(ctx context.Context, db database.Store, linkedID string, emails ...string) (database.User, database.UserLink, error) {
2147+
// If it does not find a match, it falls back to email-based lookup.
2148+
// The email fallback is restricted to first-time account linking and
2149+
// legacy links (empty linked_id) only. If the user found by email
2150+
// already has a link with a different linked_id, errLinkedIDAlreadyBound
2151+
// is returned to prevent account takeover via IdP email reuse.
2152+
func findLinkedUser(ctx context.Context, db database.Store, linkedID string, loginType database.LoginType, emails ...string) (database.User, database.UserLink, error) {
20962153
var (
20972154
user database.User
20982155
link database.UserLink
@@ -2137,12 +2194,19 @@ func findLinkedUser(ctx context.Context, db database.Store, linkedID string, ema
21372194
// possible that a user_link exists without a populated 'linked_id'.
21382195
link, err = db.GetUserLinkByUserIDLoginType(ctx, database.GetUserLinkByUserIDLoginTypeParams{
21392196
UserID: user.ID,
2140-
LoginType: user.LoginType,
2197+
LoginType: loginType,
21412198
})
21422199
if err != nil && !errors.Is(err, sql.ErrNoRows) {
21432200
return database.User{}, database.UserLink{}, xerrors.Errorf("get user link by user id and login type: %w", err)
21442201
}
21452202

2203+
// Block email fallback when an existing link has a different linked_id.
2204+
// Prevents account takeover via IdP email reuse; first-time and legacy
2205+
// (empty linked_id) links pass through.
2206+
if err == nil && link.LinkedID != "" && link.LinkedID != linkedID {
2207+
return database.User{}, database.UserLink{}, errLinkedIDAlreadyBound
2208+
}
2209+
21462210
return user, link, nil
21472211
}
21482212

0 commit comments

Comments
 (0)