From ba42c1787b45b29a3346ea2c7e57aba232369764 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 13 Jun 2023 15:00:19 -0500 Subject: [PATCH 01/58] WIP: working on ability to merge oidc with password auth account --- .../AccountPage/AccountPage.tsx | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx b/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx index bc2cdc793fb2a..898f5bddb525b 100644 --- a/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx +++ b/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx @@ -4,13 +4,31 @@ import { AccountForm } from "../../../components/SettingsAccountForm/SettingsAcc import { useAuth } from "components/AuthProvider/AuthProvider" import { useMe } from "hooks/useMe" import { usePermissions } from "hooks/usePermissions" +import { SignInForm } from "components/SignInForm/SignInForm" +import { retrieveRedirect } from "utils/redirect" +import { useQuery } from "@tanstack/react-query" +import { getAuthMethods } from "api/api" export const AccountPage: FC = () => { + const queryKey = ["get-auth-methods"] + const { + data: authMethodsData, + error: authMethodsError, + isLoading: authMethodsLoading, + isFetched: authMethodsFetched, + } = useQuery({ + queryKey, + queryFn: getAuthMethods, + }) + const [authState, authSend] = useAuth() const me = useMe() const permissions = usePermissions() const { updateProfileError } = authState.context const canEditUsers = permissions && permissions.updateUsers + // Extra + const redirectTo = retrieveRedirect(location.search) + console.log(authState.context.data) return (
@@ -29,6 +47,17 @@ export const AccountPage: FC = () => { }) }} /> + + { + console.log(credentials) + return + }} + >
) } From a74695249090fd0a57e7acc127c304901cf2968b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 14 Jun 2023 11:48:33 -0500 Subject: [PATCH 02/58] Fix displaying error on login page --- site/src/components/SignInForm/SignInForm.tsx | 2 +- .../UserSettingsPage/AccountPage/AccountPage.tsx | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/site/src/components/SignInForm/SignInForm.tsx b/site/src/components/SignInForm/SignInForm.tsx index 4e5d552fc4371..0ebb930ae99ac 100644 --- a/site/src/components/SignInForm/SignInForm.tsx +++ b/site/src/components/SignInForm/SignInForm.tsx @@ -99,7 +99,7 @@ export const SignInForm: FC> = ({ {loginPageTranslation.t("signInTo")}{" "} {commonTranslation.t("coder")} - +
diff --git a/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx b/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx index 898f5bddb525b..9eb60591575be 100644 --- a/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx +++ b/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx @@ -8,6 +8,7 @@ import { SignInForm } from "components/SignInForm/SignInForm" import { retrieveRedirect } from "utils/redirect" import { useQuery } from "@tanstack/react-query" import { getAuthMethods } from "api/api" +import { AuthMethods } from "api/typesGenerated" export const AccountPage: FC = () => { const queryKey = ["get-auth-methods"] @@ -17,6 +18,16 @@ export const AccountPage: FC = () => { isLoading: authMethodsLoading, isFetched: authMethodsFetched, } = useQuery({ + select: (res: AuthMethods) => { + return { + ...res, + // Disable the password auth in this account section. For merging accounts, + // we only want to support oidc. + password: { + enabled: false, + }, + } + }, queryKey, queryFn: getAuthMethods, }) From bd93ef9c932a24a5d2275a434e5ffc53b1c63054 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 14 Jun 2023 13:20:25 -0500 Subject: [PATCH 03/58] add migration to add a table for tracking oidc state --- coderd/coderd.go | 1 + coderd/database/dump.sql | 17 ++++++++++++++++ .../000126_merge_oidc_account.down.sql | 0 .../000126_merge_oidc_account.up.sql | 17 ++++++++++++++++ coderd/database/models.go | 9 +++++++++ coderd/database/queries/users.sql | 20 +++++++++++++++++++ coderd/httpmw/oauth2.go | 8 ++++++++ 7 files changed, 72 insertions(+) create mode 100644 coderd/database/migrations/000126_merge_oidc_account.down.sql create mode 100644 coderd/database/migrations/000126_merge_oidc_account.up.sql diff --git a/coderd/coderd.go b/coderd/coderd.go index 64cc157c723d4..e9c6c35c8a54f 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -603,6 +603,7 @@ func New(options *Options) *API { // This value is intentionally increased during tests. r.Use(httpmw.RateLimit(options.LoginRateLimit, time.Minute)) r.Post("/login", api.postLogin) + r.Post("/upgrade-to-oidc", api.postUpgradeToOIDC) r.Route("/oauth2", func(r chi.Router) { r.Route("/github", func(r chi.Router) { r.Use(httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient, nil)) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 174e6d42aa0d9..c6647ce3379cf 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -263,6 +263,17 @@ CREATE SEQUENCE licenses_id_seq ALTER SEQUENCE licenses_id_seq OWNED BY licenses.id; +CREATE TABLE oidc_merge_state ( + state_string text NOT NULL, + created_at timestamp with time zone NOT NULL, + expires_at timestamp with time zone NOT NULL, + user_id uuid NOT NULL +); + +COMMENT ON TABLE oidc_merge_state IS 'Stores the state string for OIDC merge requests. If an OIDC state string is found in this table, it is assumed the user had a LoginType "password" and is switching to an OIDC based authentication.'; + +COMMENT ON COLUMN oidc_merge_state.expires_at IS 'The time at which the state string expires, a merge request times out if the user does not perform it quick enough.'; + CREATE TABLE organization_members ( user_id uuid NOT NULL, organization_id uuid NOT NULL, @@ -792,6 +803,9 @@ ALTER TABLE ONLY licenses ALTER TABLE ONLY licenses ADD CONSTRAINT licenses_pkey PRIMARY KEY (id); +ALTER TABLE ONLY oidc_merge_state + ADD CONSTRAINT oidc_merge_state_pkey PRIMARY KEY (state_string); + ALTER TABLE ONLY organization_members ADD CONSTRAINT organization_members_pkey PRIMARY KEY (organization_id, user_id); @@ -957,6 +971,9 @@ ALTER TABLE ONLY group_members ALTER TABLE ONLY groups ADD CONSTRAINT groups_organization_id_fkey FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE CASCADE; +ALTER TABLE ONLY oidc_merge_state + ADD CONSTRAINT oidc_merge_state_user_id_fkey FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE; + ALTER TABLE ONLY organization_members ADD CONSTRAINT organization_members_organization_id_uuid_fkey FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE CASCADE; diff --git a/coderd/database/migrations/000126_merge_oidc_account.down.sql b/coderd/database/migrations/000126_merge_oidc_account.down.sql new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/coderd/database/migrations/000126_merge_oidc_account.up.sql b/coderd/database/migrations/000126_merge_oidc_account.up.sql new file mode 100644 index 0000000000000..31699b27ed161 --- /dev/null +++ b/coderd/database/migrations/000126_merge_oidc_account.up.sql @@ -0,0 +1,17 @@ +BEGIN; + +CREATE TABLE IF NOT EXISTS oidc_merge_state ( + state_string text NOT NULL, + created_at timestamptz NOT NULL, + expires_at timestamptz NOT NULL, + user_id uuid NOT NULL + REFERENCES users (id) ON DELETE CASCADE, + PRIMARY KEY (state_string) +); + +COMMENT ON TABLE oidc_merge_state IS 'Stores the state string for OIDC merge requests. If an OIDC state string is found in this table, ' + 'it is assumed the user had a LoginType "password" and is switching to an OIDC based authentication.'; + +COMMENT ON COLUMN oidc_merge_state.expires_at IS 'The time at which the state string expires, a merge request times out if the user does not perform it quick enough.'; + +COMMIT; diff --git a/coderd/database/models.go b/coderd/database/models.go index cd571d6bba803..02f8af93395bd 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -1422,6 +1422,15 @@ type License struct { UUID uuid.UUID `db:"uuid" json:"uuid"` } +// Stores the state string for OIDC merge requests. If an OIDC state string is found in this table, it is assumed the user had a LoginType "password" and is switching to an OIDC based authentication. +type OidcMergeState struct { + StateString string `db:"state_string" json:"state_string"` + CreatedAt time.Time `db:"created_at" json:"created_at"` + // The time at which the state string expires, a merge request times out if the user does not perform it quick enough. + ExpiresAt time.Time `db:"expires_at" json:"expires_at"` + UserID uuid.UUID `db:"user_id" json:"user_id"` +} + type Organization struct { ID uuid.UUID `db:"id" json:"id"` Name string `db:"name" json:"name"` diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index 22b3c9c1d6584..bb0abbeb9b270 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -1,3 +1,23 @@ +-- name: GetUserOIDCMergeState :one +SELECT + * +FROM + oidc_merge_state +WHERE + user_id = @user_id AND + state_string = @state_string; + +-- name: InsertUserOIDCMergeState :one +INSERT INTO + oidc_merge_state ( + user_id, + state_string, + created_at, + updated_at + ) +VALUES + ($1, $2, $3, $4) RETURNING *; + -- name: GetUserByID :one SELECT * diff --git a/coderd/httpmw/oauth2.go b/coderd/httpmw/oauth2.go index be2648f474512..584ad6e45c9e4 100644 --- a/coderd/httpmw/oauth2.go +++ b/coderd/httpmw/oauth2.go @@ -90,6 +90,14 @@ func ExtractOAuth2(config OAuth2Config, client *http.Client, authURLOpts map[str state := r.URL.Query().Get("state") if code == "" { + // If this url param is provided, then a user is trying to merge + // their account with an OIDC account. Their password would have + // been required to get to this point, so we do not need to verify + // their password again. + // TODO: @emyrk should we check their api key here? + oidcMergeState := r.URL.Query().Get("oidc_merge_state") + var _ = oidcMergeState + // If the code isn't provided, we'll redirect! state, err := cryptorand.String(32) if err != nil { From 0078629d82fab343f3edd4a17d02e845476c846b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 14 Jun 2023 14:24:31 -0500 Subject: [PATCH 04/58] WIP: add full functionality, but block the upgrade --- coderd/coderd.go | 9 +- coderd/database/dbauthz/dbauthz.go | 15 ++ coderd/database/dbfake/dbfake.go | 8 + coderd/database/dbmetrics/dbmetrics.go | 28 +++ coderd/database/dump.sql | 17 +- .../000126_merge_oidc_account.up.sql | 17 -- ...sql => 000127_merge_oidc_account.down.sql} | 0 .../000127_merge_oidc_account.up.sql | 22 ++ coderd/database/models.go | 8 +- coderd/database/querier.go | 2 + coderd/database/queries.sql.go | 68 +++++++ coderd/database/queries/users.sql | 13 +- coderd/httpmw/oauth2.go | 34 ++-- coderd/userauth.go | 191 ++++++++++++++---- codersdk/users.go | 5 + .../AccountPage/AccountPage.tsx | 2 +- 16 files changed, 350 insertions(+), 89 deletions(-) delete mode 100644 coderd/database/migrations/000126_merge_oidc_account.up.sql rename coderd/database/migrations/{000126_merge_oidc_account.down.sql => 000127_merge_oidc_account.down.sql} (100%) create mode 100644 coderd/database/migrations/000127_merge_oidc_account.up.sql diff --git a/coderd/coderd.go b/coderd/coderd.go index 61ac764b91bd4..7a7ead9acb84b 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -604,7 +604,6 @@ func New(options *Options) *API { // This value is intentionally increased during tests. r.Use(httpmw.RateLimit(options.LoginRateLimit, time.Minute)) r.Post("/login", api.postLogin) - r.Post("/upgrade-to-oidc", api.postUpgradeToOIDC) r.Route("/oauth2", func(r chi.Router) { r.Route("/github", func(r chi.Router) { r.Use(httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient, nil)) @@ -615,6 +614,14 @@ func New(options *Options) *API { r.Use(httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient, oidcAuthURLParams)) r.Get("/", api.userOIDC) }) + // This is an authenticated route. It handles converting a user + // from Password based authentication to OIDC based + r.Route("/upgrade-to-oidc", func(r chi.Router) { + r.Use( + apiKeyMiddleware, + ) + r.Post("/upgrade-to-oidc", api.postUpgradeToOIDC) + }) }) r.Group(func(r chi.Router) { r.Use( diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 30b8c5d33569c..185c91e2e7a81 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1336,6 +1336,13 @@ func (q *querier) GetUserLinkByUserIDLoginType(ctx context.Context, arg database return q.db.GetUserLinkByUserIDLoginType(ctx, arg) } +func (q *querier) GetUserOauthMergeState(ctx context.Context, arg database.GetUserOauthMergeStateParams) (database.OauthMergeState, error) { + if err := q.authorizeContext(ctx, rbac.ActionRead, rbac.ResourceSystem); err != nil { + return database.OauthMergeState{}, err + } + return q.db.GetUserOauthMergeState(ctx, arg) +} + func (q *querier) GetUsers(ctx context.Context, arg database.GetUsersParams) ([]database.GetUsersRow, error) { // TODO: We should use GetUsersWithCount with a better method signature. return fetchWithPostFilter(q.auth, q.db.GetUsers)(ctx, arg) @@ -1850,6 +1857,14 @@ func (q *querier) InsertUserLink(ctx context.Context, arg database.InsertUserLin return q.db.InsertUserLink(ctx, arg) } +func (q *querier) InsertUserOauthMergeState(ctx context.Context, arg database.InsertUserOauthMergeStateParams) (database.OauthMergeState, error) { + if err := q.authorizeContext(ctx, rbac.ActionCreate, rbac.ResourceSystem); err != nil { + return database.OauthMergeState{}, err + } + + return q.db.InsertUserOauthMergeState(ctx, arg) +} + func (q *querier) InsertWorkspace(ctx context.Context, arg database.InsertWorkspaceParams) (database.Workspace, error) { obj := rbac.ResourceWorkspace.WithOwner(arg.OwnerID.String()).InOrg(arg.OrganizationID) return insert(q.log, q.auth, obj, q.db.InsertWorkspace)(ctx, arg) diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index dad08f081a4a9..f0aa04aa2fde2 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -2549,6 +2549,10 @@ func (q *fakeQuerier) GetUserLinkByUserIDLoginType(_ context.Context, params dat return database.UserLink{}, sql.ErrNoRows } +func (q *fakeQuerier) GetUserOauthMergeState(ctx context.Context, arg database.GetUserOauthMergeStateParams) (database.OauthMergeState, error) { + panic("Not implemented") +} + func (q *fakeQuerier) GetUsers(_ context.Context, params database.GetUsersParams) ([]database.GetUsersRow, error) { if err := validateDatabaseType(params); err != nil { return nil, err @@ -3926,6 +3930,10 @@ func (q *fakeQuerier) InsertUserLink(_ context.Context, args database.InsertUser return link, nil } +func (q *fakeQuerier) InsertUserOauthMergeState(ctx context.Context, arg database.InsertUserOauthMergeStateParams) (database.OauthMergeState, error) { + panic("Not implemented") +} + func (q *fakeQuerier) InsertWorkspace(_ context.Context, arg database.InsertWorkspaceParams) (database.Workspace, error) { if err := validateDatabaseType(arg); err != nil { return database.Workspace{}, err diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index 49cf5fc402c5e..94c05dde7faa6 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -108,6 +108,20 @@ func (m metricsStore) GetAuthorizedUserCount(ctx context.Context, arg database.G return count, err } +func (m metricsStore) GetUserOIDCMergeState(ctx context.Context, arg database.GetUserOIDCMergeStateParams) (database.OidcMergeState, error) { + start := time.Now() + r0, r1 := m.s.GetUserOIDCMergeState(ctx, arg) + m.queryLatencies.WithLabelValues("GetUserOIDCMergeState").Observe(time.Since(start).Seconds()) + return r0, r1 +} + +func (m metricsStore) InsertUserOIDCMergeState(ctx context.Context, arg database.InsertUserOIDCMergeStateParams) (database.OidcMergeState, error) { + start := time.Now() + r0, r1 := m.s.InsertUserOIDCMergeState(ctx, arg) + m.queryLatencies.WithLabelValues("InsertUserOIDCMergeState").Observe(time.Since(start).Seconds()) + return r0, r1 +} + func (m metricsStore) AcquireLock(ctx context.Context, pgAdvisoryXactLock int64) error { start := time.Now() err := m.s.AcquireLock(ctx, pgAdvisoryXactLock) @@ -689,6 +703,13 @@ func (m metricsStore) GetUserLinkByUserIDLoginType(ctx context.Context, arg data return link, err } +func (m metricsStore) GetUserOauthMergeState(ctx context.Context, arg database.GetUserOauthMergeStateParams) (database.OauthMergeState, error) { + start := time.Now() + r0, r1 := m.s.GetUserOauthMergeState(ctx, arg) + m.queryLatencies.WithLabelValues("GetUserOauthMergeState").Observe(time.Since(start).Seconds()) + return r0, r1 +} + func (m metricsStore) GetUsers(ctx context.Context, arg database.GetUsersParams) ([]database.GetUsersRow, error) { start := time.Now() users, err := m.s.GetUsers(ctx, arg) @@ -1123,6 +1144,13 @@ func (m metricsStore) InsertUserLink(ctx context.Context, arg database.InsertUse return link, err } +func (m metricsStore) InsertUserOauthMergeState(ctx context.Context, arg database.InsertUserOauthMergeStateParams) (database.OauthMergeState, error) { + start := time.Now() + r0, r1 := m.s.InsertUserOauthMergeState(ctx, arg) + m.queryLatencies.WithLabelValues("InsertUserOauthMergeState").Observe(time.Since(start).Seconds()) + return r0, r1 +} + func (m metricsStore) InsertWorkspace(ctx context.Context, arg database.InsertWorkspaceParams) (database.Workspace, error) { start := time.Now() workspace, err := m.s.InsertWorkspace(ctx, arg) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index f849d118353d8..9546c9024bec8 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -266,16 +266,19 @@ CREATE SEQUENCE licenses_id_seq ALTER SEQUENCE licenses_id_seq OWNED BY licenses.id; -CREATE TABLE oidc_merge_state ( +CREATE TABLE oauth_merge_state ( state_string text NOT NULL, created_at timestamp with time zone NOT NULL, expires_at timestamp with time zone NOT NULL, + oauth_id text NOT NULL, user_id uuid NOT NULL ); -COMMENT ON TABLE oidc_merge_state IS 'Stores the state string for OIDC merge requests. If an OIDC state string is found in this table, it is assumed the user had a LoginType "password" and is switching to an OIDC based authentication.'; +COMMENT ON TABLE oauth_merge_state IS 'Stores the state string for Oauth merge requests. If an Oauth state string is found in this table, it is assumed the user had a LoginType "password" and is switching to an Oauth based authentication.'; -COMMENT ON COLUMN oidc_merge_state.expires_at IS 'The time at which the state string expires, a merge request times out if the user does not perform it quick enough.'; +COMMENT ON COLUMN oauth_merge_state.expires_at IS 'The time at which the state string expires, a merge request times out if the user does not perform it quick enough.'; + +COMMENT ON COLUMN oauth_merge_state.oauth_id IS 'Identifier to know which Oauth provider the user is merging with. This prevents the user from requesting "github" and merging with a different Oauth provider'; CREATE TABLE organization_members ( user_id uuid NOT NULL, @@ -806,8 +809,8 @@ ALTER TABLE ONLY licenses ALTER TABLE ONLY licenses ADD CONSTRAINT licenses_pkey PRIMARY KEY (id); -ALTER TABLE ONLY oidc_merge_state - ADD CONSTRAINT oidc_merge_state_pkey PRIMARY KEY (state_string); +ALTER TABLE ONLY oauth_merge_state + ADD CONSTRAINT oauth_merge_state_pkey PRIMARY KEY (state_string); ALTER TABLE ONLY organization_members ADD CONSTRAINT organization_members_pkey PRIMARY KEY (organization_id, user_id); @@ -974,8 +977,8 @@ ALTER TABLE ONLY group_members ALTER TABLE ONLY groups ADD CONSTRAINT groups_organization_id_fkey FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE CASCADE; -ALTER TABLE ONLY oidc_merge_state - ADD CONSTRAINT oidc_merge_state_user_id_fkey FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE; +ALTER TABLE ONLY oauth_merge_state + ADD CONSTRAINT oauth_merge_state_user_id_fkey FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE; ALTER TABLE ONLY organization_members ADD CONSTRAINT organization_members_organization_id_uuid_fkey FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE CASCADE; diff --git a/coderd/database/migrations/000126_merge_oidc_account.up.sql b/coderd/database/migrations/000126_merge_oidc_account.up.sql deleted file mode 100644 index 31699b27ed161..0000000000000 --- a/coderd/database/migrations/000126_merge_oidc_account.up.sql +++ /dev/null @@ -1,17 +0,0 @@ -BEGIN; - -CREATE TABLE IF NOT EXISTS oidc_merge_state ( - state_string text NOT NULL, - created_at timestamptz NOT NULL, - expires_at timestamptz NOT NULL, - user_id uuid NOT NULL - REFERENCES users (id) ON DELETE CASCADE, - PRIMARY KEY (state_string) -); - -COMMENT ON TABLE oidc_merge_state IS 'Stores the state string for OIDC merge requests. If an OIDC state string is found in this table, ' - 'it is assumed the user had a LoginType "password" and is switching to an OIDC based authentication.'; - -COMMENT ON COLUMN oidc_merge_state.expires_at IS 'The time at which the state string expires, a merge request times out if the user does not perform it quick enough.'; - -COMMIT; diff --git a/coderd/database/migrations/000126_merge_oidc_account.down.sql b/coderd/database/migrations/000127_merge_oidc_account.down.sql similarity index 100% rename from coderd/database/migrations/000126_merge_oidc_account.down.sql rename to coderd/database/migrations/000127_merge_oidc_account.down.sql diff --git a/coderd/database/migrations/000127_merge_oidc_account.up.sql b/coderd/database/migrations/000127_merge_oidc_account.up.sql new file mode 100644 index 0000000000000..d4e79888bb3b1 --- /dev/null +++ b/coderd/database/migrations/000127_merge_oidc_account.up.sql @@ -0,0 +1,22 @@ +BEGIN; + +CREATE TABLE IF NOT EXISTS oauth_merge_state ( + state_string text NOT NULL, + created_at timestamptz NOT NULL, + expires_at timestamptz NOT NULL, + oauth_id text NOT NULL, + user_id uuid NOT NULL + REFERENCES users (id) ON DELETE CASCADE, + PRIMARY KEY (state_string) +); + +COMMENT ON TABLE oauth_merge_state IS 'Stores the state string for Oauth merge requests. If an Oauth state string is found in this table, ' + 'it is assumed the user had a LoginType "password" and is switching to an Oauth based authentication.'; + +COMMENT ON COLUMN oauth_merge_state.expires_at IS 'The time at which the state string expires, a merge request times out if the user does not perform it quick enough.'; + +COMMENT ON COLUMN oauth_merge_state.oauth_id IS 'Identifier to know which Oauth provider the user is merging with. ' + 'This prevents the user from requesting "github" and merging with a different Oauth provider' +; + +COMMIT; diff --git a/coderd/database/models.go b/coderd/database/models.go index f26db629b94af..837020414f22b 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -1426,13 +1426,15 @@ type License struct { UUID uuid.UUID `db:"uuid" json:"uuid"` } -// Stores the state string for OIDC merge requests. If an OIDC state string is found in this table, it is assumed the user had a LoginType "password" and is switching to an OIDC based authentication. -type OidcMergeState struct { +// Stores the state string for Oauth merge requests. If an Oauth state string is found in this table, it is assumed the user had a LoginType "password" and is switching to an Oauth based authentication. +type OauthMergeState struct { StateString string `db:"state_string" json:"state_string"` CreatedAt time.Time `db:"created_at" json:"created_at"` // The time at which the state string expires, a merge request times out if the user does not perform it quick enough. ExpiresAt time.Time `db:"expires_at" json:"expires_at"` - UserID uuid.UUID `db:"user_id" json:"user_id"` + // Identifier to know which Oauth provider the user is merging with. This prevents the user from requesting "github" and merging with a different Oauth provider + OauthID string `db:"oauth_id" json:"oauth_id"` + UserID uuid.UUID `db:"user_id" json:"user_id"` } type Organization struct { diff --git a/coderd/database/querier.go b/coderd/database/querier.go index c427ac768c79f..d335698c39ef5 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -116,6 +116,7 @@ type sqlcQuerier interface { GetUserCount(ctx context.Context) (int64, error) GetUserLinkByLinkedID(ctx context.Context, linkedID string) (UserLink, error) GetUserLinkByUserIDLoginType(ctx context.Context, arg GetUserLinkByUserIDLoginTypeParams) (UserLink, error) + GetUserOauthMergeState(ctx context.Context, arg GetUserOauthMergeStateParams) (OauthMergeState, error) // This will never return deleted users. GetUsers(ctx context.Context, arg GetUsersParams) ([]GetUsersRow, error) // This shouldn't check for deleted, because it's frequently used @@ -193,6 +194,7 @@ type sqlcQuerier interface { // InsertUserGroupsByName adds a user to all provided groups, if they exist. InsertUserGroupsByName(ctx context.Context, arg InsertUserGroupsByNameParams) error InsertUserLink(ctx context.Context, arg InsertUserLinkParams) (UserLink, error) + InsertUserOauthMergeState(ctx context.Context, arg InsertUserOauthMergeStateParams) (OauthMergeState, error) InsertWorkspace(ctx context.Context, arg InsertWorkspaceParams) (Workspace, error) InsertWorkspaceAgent(ctx context.Context, arg InsertWorkspaceAgentParams) (WorkspaceAgent, error) InsertWorkspaceAgentMetadata(ctx context.Context, arg InsertWorkspaceAgentMetadataParams) error diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 2c71a96ccc4e0..621ca59fd2b8c 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -4812,6 +4812,34 @@ func (q *sqlQuerier) GetUserCount(ctx context.Context) (int64, error) { return count, err } +const getUserOauthMergeState = `-- name: GetUserOauthMergeState :one +SELECT + state_string, created_at, expires_at, oauth_id, user_id +FROM + oauth_merge_state +WHERE + user_id = $1 AND + state_string = $2 +` + +type GetUserOauthMergeStateParams struct { + UserID uuid.UUID `db:"user_id" json:"user_id"` + StateString string `db:"state_string" json:"state_string"` +} + +func (q *sqlQuerier) GetUserOauthMergeState(ctx context.Context, arg GetUserOauthMergeStateParams) (OauthMergeState, error) { + row := q.db.QueryRowContext(ctx, getUserOauthMergeState, arg.UserID, arg.StateString) + var i OauthMergeState + err := row.Scan( + &i.StateString, + &i.CreatedAt, + &i.ExpiresAt, + &i.OauthID, + &i.UserID, + ) + return i, err +} + const getUsers = `-- name: GetUsers :many SELECT id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at, COUNT(*) OVER() AS count @@ -5041,6 +5069,46 @@ func (q *sqlQuerier) InsertUser(ctx context.Context, arg InsertUserParams) (User return i, err } +const insertUserOauthMergeState = `-- name: InsertUserOauthMergeState :one +INSERT INTO + oauth_merge_state ( + user_id, + state_string, + oauth_id, + created_at, + expires_at + ) +VALUES + ($1, $2, $3, $4, $5) RETURNING state_string, created_at, expires_at, oauth_id, user_id +` + +type InsertUserOauthMergeStateParams struct { + UserID uuid.UUID `db:"user_id" json:"user_id"` + StateString string `db:"state_string" json:"state_string"` + OauthID string `db:"oauth_id" json:"oauth_id"` + CreatedAt time.Time `db:"created_at" json:"created_at"` + ExpiresAt time.Time `db:"expires_at" json:"expires_at"` +} + +func (q *sqlQuerier) InsertUserOauthMergeState(ctx context.Context, arg InsertUserOauthMergeStateParams) (OauthMergeState, error) { + row := q.db.QueryRowContext(ctx, insertUserOauthMergeState, + arg.UserID, + arg.StateString, + arg.OauthID, + arg.CreatedAt, + arg.ExpiresAt, + ) + var i OauthMergeState + err := row.Scan( + &i.StateString, + &i.CreatedAt, + &i.ExpiresAt, + &i.OauthID, + &i.UserID, + ) + return i, err +} + const updateUserDeletedByID = `-- name: UpdateUserDeletedByID :exec UPDATE users diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index bb0abbeb9b270..e8e0457ba81e1 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -1,22 +1,23 @@ --- name: GetUserOIDCMergeState :one +-- name: GetUserOauthMergeState :one SELECT * FROM - oidc_merge_state + oauth_merge_state WHERE user_id = @user_id AND state_string = @state_string; --- name: InsertUserOIDCMergeState :one +-- name: InsertUserOauthMergeState :one INSERT INTO - oidc_merge_state ( + oauth_merge_state ( user_id, state_string, + oauth_id, created_at, - updated_at + expires_at ) VALUES - ($1, $2, $3, $4) RETURNING *; + ($1, $2, $3, $4, $5) RETURNING *; -- name: GetUserByID :one SELECT diff --git a/coderd/httpmw/oauth2.go b/coderd/httpmw/oauth2.go index 584ad6e45c9e4..47747b8cae7a9 100644 --- a/coderd/httpmw/oauth2.go +++ b/coderd/httpmw/oauth2.go @@ -16,8 +16,9 @@ import ( type oauth2StateKey struct{} type OAuth2State struct { - Token *oauth2.Token - Redirect string + Token *oauth2.Token + Redirect string + StateString string } // OAuth2Config exposes a subset of *oauth2.Config functions for easier testing. @@ -90,22 +91,26 @@ func ExtractOAuth2(config OAuth2Config, client *http.Client, authURLOpts map[str state := r.URL.Query().Get("state") if code == "" { + // If the code isn't provided, we'll redirect! + var state string // If this url param is provided, then a user is trying to merge // their account with an OIDC account. Their password would have // been required to get to this point, so we do not need to verify // their password again. // TODO: @emyrk should we check their api key here? oidcMergeState := r.URL.Query().Get("oidc_merge_state") - var _ = oidcMergeState - - // If the code isn't provided, we'll redirect! - state, err := cryptorand.String(32) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error generating state string.", - Detail: err.Error(), - }) - return + if oidcMergeState != "" { + state = oidcMergeState + } else { + var err error + state, err = cryptorand.String(32) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error generating state string.", + Detail: err.Error(), + }) + return + } } http.SetCookie(rw, &http.Cookie{ @@ -166,8 +171,9 @@ func ExtractOAuth2(config OAuth2Config, client *http.Client, authURLOpts map[str } ctx = context.WithValue(ctx, oauth2StateKey{}, OAuth2State{ - Token: oauthToken, - Redirect: redirect, + Token: oauthToken, + Redirect: redirect, + StateString: state, }) next.ServeHTTP(rw, r.WithContext(ctx)) }) diff --git a/coderd/userauth.go b/coderd/userauth.go index d9b87f1ca7b82..0c685608bd727 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -7,9 +7,13 @@ import ( "fmt" "net/http" "net/mail" + "net/url" "sort" "strconv" "strings" + "time" + + "github.com/coder/coder/cryptorand" "github.com/coreos/go-oidc/v3/oidc" "github.com/google/go-github/v43/github" @@ -30,6 +34,90 @@ import ( "github.com/coder/coder/codersdk" ) +func (api *API) postUpgradeToOIDC(rw http.ResponseWriter, r *http.Request) { + var ( + ctx = r.Context() + auditor = api.Auditor.Load() + aReq, commitAudit = audit.InitRequest[database.APIKey](rw, &audit.RequestParams{ + Audit: *auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionCreate, + }) + ) + // TODO: @emyrk This does make a new api key. Make a new auditable resource + // for oidc state. + aReq.Old = database.APIKey{} + defer commitAudit() + + var req codersdk.UpgradeToOIDCRequest + if !httpapi.Read(ctx, rw, r, &req) { + return + } + + var oauthID string + switch strings.ToLower(req.OauthProvider) { + case "oidc": + oauthID = "oidc" + case "github": + oauthID = "github" + default: + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: fmt.Sprintf("Unknown oauth provider %q. Choose from '%s' or '%s'", + req.OauthProvider, "oidc", "github"), + }) + return + } + + user, _, ok := api.loginRequest(ctx, rw, req.LoginWithPasswordRequest) + if !ok { + return + } + + // The user wants to convert their password based authentication to OIDC. + if user.LoginType != database.LoginTypePassword { + // This is checked in loginRequest, but checked again here in case that shared + // function changes its checks. Just some defensive programming. + // This login type is **required** to be password based to prevent + // users from converting other login types to OIDC. + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "User account must be have password based authentication.", + }) + return + } + + stateString, err := cryptorand.String(32) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error generating state string.", + Detail: err.Error(), + }) + return + } + + now := time.Now() + mergeState, err := api.Database.InsertUserOauthMergeState(ctx, database.InsertUserOauthMergeStateParams{ + UserID: user.ID, + StateString: stateString, + OauthID: oauthID, + CreatedAt: now, + ExpiresAt: now.Add(time.Minute * 5), + }) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal Server Error", + Detail: err.Error(), + }) + return + } + + // Redirect the user back to where they were after the account merge. + redirectURL := fmt.Sprintf("/api/v2/users/%s/callback?redirect=%s&oidc_merge_state=%s", oauthID, url.QueryEscape(r.URL.Path), mergeState.StateString) + + // Redirect the user to the normal OIDC flow with the special 'oidc_merge_state' state string. + http.Redirect(rw, r, redirectURL, http.StatusTemporaryRedirect) +} + // Authenticates the user with an email and password. // // @Summary Log in user @@ -59,34 +147,74 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { return } + user, roles, ok := api.loginRequest(ctx, rw, loginWithPassword) + if !ok { + // user failed to login + return + } + + userSubj := rbac.Subject{ + ID: user.ID.String(), + Roles: rbac.RoleNames(roles.Roles), + Groups: roles.Groups, + Scope: rbac.ScopeAll, + } + + //nolint:gocritic // Creating the API key as the user instead of as system. + cookie, key, err := api.createAPIKey(dbauthz.As(ctx, userSubj), apikey.CreateParams{ + UserID: user.ID, + LoginType: database.LoginTypePassword, + RemoteAddr: r.RemoteAddr, + DeploymentValues: api.DeploymentValues, + }) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Failed to create API key.", + Detail: err.Error(), + }) + return + } + + aReq.New = *key + + http.SetCookie(rw, cookie) + + httpapi.Write(ctx, rw, http.StatusCreated, codersdk.LoginWithPasswordResponse{ + SessionToken: cookie.Value, + }) +} + +// loginRequest will process a LoginWithPasswordRequest and return the user if +// the credentials are correct. If 'false' is returned, the authentication failed +// and the appropriate error will be written to the ResponseWriter. +func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req codersdk.LoginWithPasswordRequest) (database.User, database.GetAuthorizationUserRolesRow, bool) { //nolint:gocritic // In order to login, we need to get the user first! user, err := api.Database.GetUserByEmailOrUsername(dbauthz.AsSystemRestricted(ctx), database.GetUserByEmailOrUsernameParams{ - Email: loginWithPassword.Email, + Email: req.Email, }) if err != nil && !xerrors.Is(err, sql.ErrNoRows) { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error.", }) - return + return database.User{}, database.GetAuthorizationUserRolesRow{}, false } - aReq.UserID = user.ID - // If the user doesn't exist, it will be a default struct. - equal, err := userpassword.Compare(string(user.HashedPassword), loginWithPassword.Password) + equal, err := userpassword.Compare(string(user.HashedPassword), req.Password) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error.", }) - return + return database.User{}, database.GetAuthorizationUserRolesRow{}, false } + if !equal { // This message is the same as above to remove ease in detecting whether // users are registered or not. Attackers still could with a timing attack. httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{ Message: "Incorrect email or password.", }) - return + return database.User{}, database.GetAuthorizationUserRolesRow{}, false } // If password authentication is disabled and the user does not have the @@ -95,14 +223,14 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ Message: "Password authentication is disabled.", }) - return + return database.User{}, database.GetAuthorizationUserRolesRow{}, false } if user.LoginType != database.LoginTypePassword { httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ Message: fmt.Sprintf("Incorrect login type, attempting to use %q but user is of login type %q", database.LoginTypePassword, user.LoginType), }) - return + return database.User{}, database.GetAuthorizationUserRolesRow{}, false } //nolint:gocritic // System needs to fetch user roles in order to login user. @@ -111,7 +239,7 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error.", }) - return + return database.User{}, database.GetAuthorizationUserRolesRow{}, false } // If the user logged into a suspended account, reject the login request. @@ -119,38 +247,10 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{ Message: "Your account is suspended. Contact an admin to reactivate your account.", }) - return + return database.User{}, database.GetAuthorizationUserRolesRow{}, false } - userSubj := rbac.Subject{ - ID: user.ID.String(), - Roles: rbac.RoleNames(roles.Roles), - Groups: roles.Groups, - Scope: rbac.ScopeAll, - } - - //nolint:gocritic // Creating the API key as the user instead of as system. - cookie, key, err := api.createAPIKey(dbauthz.As(ctx, userSubj), apikey.CreateParams{ - UserID: user.ID, - LoginType: database.LoginTypePassword, - RemoteAddr: r.RemoteAddr, - DeploymentValues: api.DeploymentValues, - }) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Failed to create API key.", - Detail: err.Error(), - }) - return - } - - aReq.New = *key - - http.SetCookie(rw, cookie) - - httpapi.Write(ctx, rw, http.StatusCreated, codersdk.LoginWithPasswordResponse{ - SessionToken: cookie.Value, - }) + return user, roles, true } // Clear the user's session cookie. @@ -864,6 +964,17 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook } if user.ID != uuid.Nil && user.LoginType != params.LoginType { + mergeState, err := api.Database.GetUserOauthMergeState(dbauthz.AsSystemRestricted(ctx), database.GetUserOauthMergeStateParams{ + UserID: user.ID, + StateString: params.State.StateString, + }) + if err == nil && mergeState.ExpiresAt.Before(time.Now()) { + return httpError{ + code: http.StatusForbidden, + msg: "Hey! This upgrade would have worked if I let it", + } + } + return httpError{ code: http.StatusForbidden, msg: fmt.Sprintf("Incorrect login type, attempting to use %q but user is of login type %q", diff --git a/codersdk/users.go b/codersdk/users.go index 05838a792c370..c2c28af417304 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -93,6 +93,11 @@ type UserRoles struct { OrganizationRoles map[uuid.UUID][]string `json:"organization_roles"` } +type UpgradeToOIDCRequest struct { + OauthProvider string `json:"oauth_provider" validate:"required"` + LoginWithPasswordRequest +} + // LoginWithPasswordRequest enables callers to authenticate with email and password. type LoginWithPasswordRequest struct { Email string `json:"email" validate:"required,email" format:"email"` diff --git a/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx b/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx index 9eb60591575be..f08fd77a9111f 100644 --- a/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx +++ b/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx @@ -39,7 +39,7 @@ export const AccountPage: FC = () => { const canEditUsers = permissions && permissions.updateUsers // Extra const redirectTo = retrieveRedirect(location.search) - console.log(authState.context.data) + console.log(authState.context.data, authMethodsError) return (
From fc19a8a8e7819300ec90177d666f6b0a84a8f9af Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 15 Jun 2023 13:27:56 -0500 Subject: [PATCH 05/58] Functionality is working, need to polish and write tests. Currently disabled --- coderd/coderd.go | 2 +- coderd/database/dbauthz/dbauthz.go | 3 +- coderd/database/dbmetrics/dbmetrics.go | 14 ------ coderd/database/dbmock/dbmock.go | 30 ++++++++++++ coderd/userauth.go | 21 +++++--- codersdk/users.go | 7 +++ site/src/api/api.ts | 27 ++++++++++ site/src/api/typesGenerated.ts | 13 +++++ .../AccountPage/AccountPage.tsx | 49 +++++++++++++------ 9 files changed, 129 insertions(+), 37 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 7a7ead9acb84b..7127943db95e8 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -620,7 +620,7 @@ func New(options *Options) *API { r.Use( apiKeyMiddleware, ) - r.Post("/upgrade-to-oidc", api.postUpgradeToOIDC) + r.Post("/", api.postConvertToOauth) }) }) r.Group(func(r chi.Router) { diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 185c91e2e7a81..923c5f68e8795 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1858,7 +1858,8 @@ func (q *querier) InsertUserLink(ctx context.Context, arg database.InsertUserLin } func (q *querier) InsertUserOauthMergeState(ctx context.Context, arg database.InsertUserOauthMergeStateParams) (database.OauthMergeState, error) { - if err := q.authorizeContext(ctx, rbac.ActionCreate, rbac.ResourceSystem); err != nil { + // TODO: @emyrk this permission feels right? + if err := q.authorizeContext(ctx, rbac.ActionCreate, rbac.ResourceAPIKey.WithOwner(arg.UserID.String())); err != nil { return database.OauthMergeState{}, err } diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index 94c05dde7faa6..12266c129823a 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -108,20 +108,6 @@ func (m metricsStore) GetAuthorizedUserCount(ctx context.Context, arg database.G return count, err } -func (m metricsStore) GetUserOIDCMergeState(ctx context.Context, arg database.GetUserOIDCMergeStateParams) (database.OidcMergeState, error) { - start := time.Now() - r0, r1 := m.s.GetUserOIDCMergeState(ctx, arg) - m.queryLatencies.WithLabelValues("GetUserOIDCMergeState").Observe(time.Since(start).Seconds()) - return r0, r1 -} - -func (m metricsStore) InsertUserOIDCMergeState(ctx context.Context, arg database.InsertUserOIDCMergeStateParams) (database.OidcMergeState, error) { - start := time.Now() - r0, r1 := m.s.InsertUserOIDCMergeState(ctx, arg) - m.queryLatencies.WithLabelValues("InsertUserOIDCMergeState").Observe(time.Since(start).Seconds()) - return r0, r1 -} - func (m metricsStore) AcquireLock(ctx context.Context, pgAdvisoryXactLock int64) error { start := time.Now() err := m.s.AcquireLock(ctx, pgAdvisoryXactLock) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index a1c75353b7a96..1a3d9bf30bad3 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -1348,6 +1348,21 @@ func (mr *MockStoreMockRecorder) GetUserLinkByUserIDLoginType(arg0, arg1 interfa return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserLinkByUserIDLoginType", reflect.TypeOf((*MockStore)(nil).GetUserLinkByUserIDLoginType), arg0, arg1) } +// GetUserOauthMergeState mocks base method. +func (m *MockStore) GetUserOauthMergeState(arg0 context.Context, arg1 database.GetUserOauthMergeStateParams) (database.OauthMergeState, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetUserOauthMergeState", arg0, arg1) + ret0, _ := ret[0].(database.OauthMergeState) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetUserOauthMergeState indicates an expected call of GetUserOauthMergeState. +func (mr *MockStoreMockRecorder) GetUserOauthMergeState(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserOauthMergeState", reflect.TypeOf((*MockStore)(nil).GetUserOauthMergeState), arg0, arg1) +} + // GetUsers mocks base method. func (m *MockStore) GetUsers(arg0 context.Context, arg1 database.GetUsersParams) ([]database.GetUsersRow, error) { m.ctrl.T.Helper() @@ -2288,6 +2303,21 @@ func (mr *MockStoreMockRecorder) InsertUserLink(arg0, arg1 interface{}) *gomock. return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "InsertUserLink", reflect.TypeOf((*MockStore)(nil).InsertUserLink), arg0, arg1) } +// InsertUserOauthMergeState mocks base method. +func (m *MockStore) InsertUserOauthMergeState(arg0 context.Context, arg1 database.InsertUserOauthMergeStateParams) (database.OauthMergeState, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "InsertUserOauthMergeState", arg0, arg1) + ret0, _ := ret[0].(database.OauthMergeState) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// InsertUserOauthMergeState indicates an expected call of InsertUserOauthMergeState. +func (mr *MockStoreMockRecorder) InsertUserOauthMergeState(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "InsertUserOauthMergeState", reflect.TypeOf((*MockStore)(nil).InsertUserOauthMergeState), arg0, arg1) +} + // InsertWorkspace mocks base method. func (m *MockStore) InsertWorkspace(arg0 context.Context, arg1 database.InsertWorkspaceParams) (database.Workspace, error) { m.ctrl.T.Helper() diff --git a/coderd/userauth.go b/coderd/userauth.go index 0c685608bd727..8de9228bdf09a 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -7,7 +7,6 @@ import ( "fmt" "net/http" "net/mail" - "net/url" "sort" "strconv" "strings" @@ -34,7 +33,7 @@ import ( "github.com/coder/coder/codersdk" ) -func (api *API) postUpgradeToOIDC(rw http.ResponseWriter, r *http.Request) { +func (api *API) postConvertToOauth(rw http.ResponseWriter, r *http.Request) { var ( ctx = r.Context() auditor = api.Auditor.Load() @@ -111,11 +110,18 @@ func (api *API) postUpgradeToOIDC(rw http.ResponseWriter, r *http.Request) { return } - // Redirect the user back to where they were after the account merge. - redirectURL := fmt.Sprintf("/api/v2/users/%s/callback?redirect=%s&oidc_merge_state=%s", oauthID, url.QueryEscape(r.URL.Path), mergeState.StateString) + httpapi.Write(ctx, rw, http.StatusCreated, codersdk.OauthConversionResponse{ + StateString: mergeState.StateString, + ExpiresAt: mergeState.ExpiresAt, + OAuthID: mergeState.OauthID, + UserID: mergeState.UserID, + }) - // Redirect the user to the normal OIDC flow with the special 'oidc_merge_state' state string. - http.Redirect(rw, r, redirectURL, http.StatusTemporaryRedirect) + //// Redirect the user back to where they were after the account merge. + //redirectURL := fmt.Sprintf("/api/v2/users/%s/callback?redirect=%s&oidc_merge_state=%s", oauthID, url.QueryEscape(r.URL.Path), mergeState.StateString) + // + //// Redirect the user to the normal OIDC flow with the special 'oidc_merge_state' state string. + //http.Redirect(rw, r, redirectURL, http.StatusTemporaryRedirect) } // Authenticates the user with an email and password. @@ -968,7 +974,8 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook UserID: user.ID, StateString: params.State.StateString, }) - if err == nil && mergeState.ExpiresAt.Before(time.Now()) { + var _ = mergeState + if err == nil { return httpError{ code: http.StatusForbidden, msg: "Hey! This upgrade would have worked if I let it", diff --git a/codersdk/users.go b/codersdk/users.go index c2c28af417304..282b1d4ef3f35 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -109,6 +109,13 @@ type LoginWithPasswordResponse struct { SessionToken string `json:"session_token" validate:"required"` } +type OauthConversionResponse struct { + StateString string `json:"state_string"` + ExpiresAt time.Time `json:"expires_at"` + OAuthID string `json:"oauth_id"` + UserID uuid.UUID `json:"user_id"` +} + type CreateOrganizationRequest struct { Name string `json:"name" validate:"required,username"` } diff --git a/site/src/api/api.ts b/site/src/api/api.ts index f9404d0853d3f..5343193338ca5 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -107,6 +107,33 @@ export const login = async ( return response.data } +export const convertToOauth = async ( + email: string, + password: string, + oauth_provider: string, +): Promise => { + const payload = JSON.stringify({ + email, + password, + oauth_provider, + }) + + try { + const response = await axios.post("/api/v2/users/upgrade-to-oidc", + payload, + { + headers: { ...CONTENT_TYPE_JSON }, + }) + return response.data + } catch (error) { + if (axios.isAxiosError(error) && error.response?.status === 401) { + return undefined + } + + throw error + } +} + export const logout = async (): Promise => { await axios.post("/api/v2/users/logout") } diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 68304bd2a3eb5..ed7a8b9f4ebb5 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -544,6 +544,14 @@ export interface OIDCConfig { readonly icon_url: string } +// From codersdk/users.go +export interface OauthConversionResponse { + readonly state_string: string + readonly expires_at: string + readonly oauth_id: string + readonly user_id: string +} + // From codersdk/organizations.go export interface Organization { readonly id: string @@ -1005,6 +1013,11 @@ export interface UpdateWorkspaceTTLRequest { readonly ttl_ms?: number } +// From codersdk/users.go +export interface UpgradeToOIDCRequest extends LoginWithPasswordRequest { + readonly oauth_provider: string +} + // From codersdk/files.go export interface UploadResponse { readonly hash: string diff --git a/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx b/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx index f08fd77a9111f..5d7bb59bc11bf 100644 --- a/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx +++ b/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx @@ -7,8 +7,9 @@ import { usePermissions } from "hooks/usePermissions" import { SignInForm } from "components/SignInForm/SignInForm" import { retrieveRedirect } from "utils/redirect" import { useQuery } from "@tanstack/react-query" -import { getAuthMethods } from "api/api" +import { convertToOauth, getAuthMethods } from "api/api" import { AuthMethods } from "api/typesGenerated" +import axios from "axios" export const AccountPage: FC = () => { const queryKey = ["get-auth-methods"] @@ -18,16 +19,16 @@ export const AccountPage: FC = () => { isLoading: authMethodsLoading, isFetched: authMethodsFetched, } = useQuery({ - select: (res: AuthMethods) => { - return { - ...res, - // Disable the password auth in this account section. For merging accounts, - // we only want to support oidc. - password: { - enabled: false, - }, - } - }, + // select: (res: AuthMethods) => { + // return { + // ...res, + // // Disable the password auth in this account section. For merging accounts, + // // we only want to support oidc. + // password: { + // enabled: false, + // }, + // } + // }, queryKey, queryFn: getAuthMethods, }) @@ -64,9 +65,29 @@ export const AccountPage: FC = () => { redirectTo={redirectTo} isSigningIn={false} error={authMethodsError} - onSubmit={(credentials: { email: string; password: string }) => { - console.log(credentials) - return + onSubmit={async (credentials: { email: string; password: string }) => { + const mergeState = await convertToOauth( + credentials.email, + credentials.password, + "oidc", + ) + + window.location.href = `/api/v2/users/oidc/callback?oidc_merge_state=${ + mergeState?.state_string + }&redirect=${encodeURIComponent(redirectTo)}` + // await axios.get( + // `/api/v2/users/oidc/callback?oidc_merge_state=${ + // mergeState?.state_string + // }&redirect=${encodeURIComponent(redirectTo)}`, + // ) + + { + /* */ + } }} >
From 31a7e78831a0165d63b6eec1bd745b66262b7ed1 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 15 Jun 2023 14:16:47 -0500 Subject: [PATCH 06/58] Begin refactoring to rename and make consistent --- coderd/coderd.go | 6 +-- coderd/database/dbauthz/dbauthz.go | 32 +++++++-------- coderd/database/dump.sql | 4 +- .../000127_merge_oidc_account.up.sql | 6 +-- coderd/database/models.go | 8 ++-- coderd/database/querier.go | 2 +- coderd/database/queries.sql.go | 26 ++++++------ coderd/database/queries/users.sql | 12 +++--- coderd/userauth.go | 40 +++++++++---------- codersdk/users.go | 7 ++-- site/src/api/typesGenerated.ts | 12 +++--- 11 files changed, 75 insertions(+), 80 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 7127943db95e8..6efa66951ff4d 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -615,12 +615,12 @@ func New(options *Options) *API { r.Get("/", api.userOIDC) }) // This is an authenticated route. It handles converting a user - // from Password based authentication to OIDC based - r.Route("/upgrade-to-oidc", func(r chi.Router) { + // from Password based authentication to Oauth + r.Route("/convert-login", func(r chi.Router) { r.Use( apiKeyMiddleware, ) - r.Post("/", api.postConvertToOauth) + r.Post("/", api.postConvertLoginType) }) }) r.Group(func(r chi.Router) { diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 923c5f68e8795..1fdcbf1e61ab8 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -671,6 +671,22 @@ func authorizedTemplateVersionFromJob(ctx context.Context, q *querier, job datab } } +func (q *querier) GetUserOauthMergeState(ctx context.Context, arg database.GetUserOauthMergeStateParams) (database.OauthMergeState, error) { + if err := q.authorizeContext(ctx, rbac.ActionRead, rbac.ResourceSystem); err != nil { + return database.OauthMergeState{}, err + } + return q.db.GetUserOauthMergeState(ctx, arg) +} + +func (q *querier) InsertUserOauthMergeState(ctx context.Context, arg database.InsertUserOauthMergeStateParams) (database.OauthMergeState, error) { + // TODO: @emyrk this permission feels right? + if err := q.authorizeContext(ctx, rbac.ActionCreate, rbac.ResourceAPIKey.WithOwner(arg.UserID.String())); err != nil { + return database.OauthMergeState{}, err + } + + return q.db.InsertUserOauthMergeState(ctx, arg) +} + func (q *querier) AcquireLock(ctx context.Context, id int64) error { return q.db.AcquireLock(ctx, id) } @@ -1336,13 +1352,6 @@ func (q *querier) GetUserLinkByUserIDLoginType(ctx context.Context, arg database return q.db.GetUserLinkByUserIDLoginType(ctx, arg) } -func (q *querier) GetUserOauthMergeState(ctx context.Context, arg database.GetUserOauthMergeStateParams) (database.OauthMergeState, error) { - if err := q.authorizeContext(ctx, rbac.ActionRead, rbac.ResourceSystem); err != nil { - return database.OauthMergeState{}, err - } - return q.db.GetUserOauthMergeState(ctx, arg) -} - func (q *querier) GetUsers(ctx context.Context, arg database.GetUsersParams) ([]database.GetUsersRow, error) { // TODO: We should use GetUsersWithCount with a better method signature. return fetchWithPostFilter(q.auth, q.db.GetUsers)(ctx, arg) @@ -1857,15 +1866,6 @@ func (q *querier) InsertUserLink(ctx context.Context, arg database.InsertUserLin return q.db.InsertUserLink(ctx, arg) } -func (q *querier) InsertUserOauthMergeState(ctx context.Context, arg database.InsertUserOauthMergeStateParams) (database.OauthMergeState, error) { - // TODO: @emyrk this permission feels right? - if err := q.authorizeContext(ctx, rbac.ActionCreate, rbac.ResourceAPIKey.WithOwner(arg.UserID.String())); err != nil { - return database.OauthMergeState{}, err - } - - return q.db.InsertUserOauthMergeState(ctx, arg) -} - func (q *querier) InsertWorkspace(ctx context.Context, arg database.InsertWorkspaceParams) (database.Workspace, error) { obj := rbac.ResourceWorkspace.WithOwner(arg.OwnerID.String()).InOrg(arg.OrganizationID) return insert(q.log, q.auth, obj, q.db.InsertWorkspace)(ctx, arg) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 9546c9024bec8..c6616bbe88477 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -270,7 +270,7 @@ CREATE TABLE oauth_merge_state ( state_string text NOT NULL, created_at timestamp with time zone NOT NULL, expires_at timestamp with time zone NOT NULL, - oauth_id text NOT NULL, + to_login_type login_type NOT NULL, user_id uuid NOT NULL ); @@ -278,7 +278,7 @@ COMMENT ON TABLE oauth_merge_state IS 'Stores the state string for Oauth merge r COMMENT ON COLUMN oauth_merge_state.expires_at IS 'The time at which the state string expires, a merge request times out if the user does not perform it quick enough.'; -COMMENT ON COLUMN oauth_merge_state.oauth_id IS 'Identifier to know which Oauth provider the user is merging with. This prevents the user from requesting "github" and merging with a different Oauth provider'; +COMMENT ON COLUMN oauth_merge_state.to_login_type IS 'The login type the user is converting to. Should be github or oidc.'; CREATE TABLE organization_members ( user_id uuid NOT NULL, diff --git a/coderd/database/migrations/000127_merge_oidc_account.up.sql b/coderd/database/migrations/000127_merge_oidc_account.up.sql index d4e79888bb3b1..066850c76929e 100644 --- a/coderd/database/migrations/000127_merge_oidc_account.up.sql +++ b/coderd/database/migrations/000127_merge_oidc_account.up.sql @@ -4,7 +4,7 @@ CREATE TABLE IF NOT EXISTS oauth_merge_state ( state_string text NOT NULL, created_at timestamptz NOT NULL, expires_at timestamptz NOT NULL, - oauth_id text NOT NULL, + to_login_type login_type NOT NULL, user_id uuid NOT NULL REFERENCES users (id) ON DELETE CASCADE, PRIMARY KEY (state_string) @@ -15,8 +15,6 @@ COMMENT ON TABLE oauth_merge_state IS 'Stores the state string for Oauth merge r COMMENT ON COLUMN oauth_merge_state.expires_at IS 'The time at which the state string expires, a merge request times out if the user does not perform it quick enough.'; -COMMENT ON COLUMN oauth_merge_state.oauth_id IS 'Identifier to know which Oauth provider the user is merging with. ' - 'This prevents the user from requesting "github" and merging with a different Oauth provider' -; +COMMENT ON COLUMN oauth_merge_state.to_login_type IS 'The login type the user is converting to. Should be github or oidc.'; COMMIT; diff --git a/coderd/database/models.go b/coderd/database/models.go index 837020414f22b..d8c449fd58007 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -1,6 +1,6 @@ // Code generated by sqlc. DO NOT EDIT. // versions: -// sqlc v1.17.2 +// sqlc v1.18.0 package database @@ -1432,9 +1432,9 @@ type OauthMergeState struct { CreatedAt time.Time `db:"created_at" json:"created_at"` // The time at which the state string expires, a merge request times out if the user does not perform it quick enough. ExpiresAt time.Time `db:"expires_at" json:"expires_at"` - // Identifier to know which Oauth provider the user is merging with. This prevents the user from requesting "github" and merging with a different Oauth provider - OauthID string `db:"oauth_id" json:"oauth_id"` - UserID uuid.UUID `db:"user_id" json:"user_id"` + // The login type the user is converting to. Should be github or oidc. + ToLoginType LoginType `db:"to_login_type" json:"to_login_type"` + UserID uuid.UUID `db:"user_id" json:"user_id"` } type Organization struct { diff --git a/coderd/database/querier.go b/coderd/database/querier.go index d335698c39ef5..a1171c1fc7b1b 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -1,6 +1,6 @@ // Code generated by sqlc. DO NOT EDIT. // versions: -// sqlc v1.17.2 +// sqlc v1.18.0 package database diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 621ca59fd2b8c..8c0df264894b4 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1,6 +1,6 @@ // Code generated by sqlc. DO NOT EDIT. // versions: -// sqlc v1.17.2 +// sqlc v1.18.0 package database @@ -4606,7 +4606,7 @@ SELECT FROM users WHERE - status = 'active'::user_status AND deleted = false + status = 'active'::user_status AND deleted = false ` func (q *sqlQuerier) GetActiveUserCount(ctx context.Context) (int64, error) { @@ -4814,12 +4814,12 @@ func (q *sqlQuerier) GetUserCount(ctx context.Context) (int64, error) { const getUserOauthMergeState = `-- name: GetUserOauthMergeState :one SELECT - state_string, created_at, expires_at, oauth_id, user_id + state_string, created_at, expires_at, to_login_type, user_id FROM oauth_merge_state WHERE - user_id = $1 AND - state_string = $2 + user_id = $1 AND + state_string = $2 ` type GetUserOauthMergeStateParams struct { @@ -4834,7 +4834,7 @@ func (q *sqlQuerier) GetUserOauthMergeState(ctx context.Context, arg GetUserOaut &i.StateString, &i.CreatedAt, &i.ExpiresAt, - &i.OauthID, + &i.ToLoginType, &i.UserID, ) return i, err @@ -4886,9 +4886,9 @@ WHERE -- Filter by rbac_roles AND CASE -- @rbac_role allows filtering by rbac roles. If 'member' is included, show everyone, as - -- everyone is a member. + -- everyone is a member. WHEN cardinality($4 :: text[]) > 0 AND 'member' != ANY($4 :: text[]) THEN - rbac_roles && $4 :: text[] + rbac_roles && $4 :: text[] ELSE true END -- End of filters @@ -5074,18 +5074,18 @@ INSERT INTO oauth_merge_state ( user_id, state_string, - oauth_id, + to_login_type, created_at, expires_at ) VALUES - ($1, $2, $3, $4, $5) RETURNING state_string, created_at, expires_at, oauth_id, user_id + ($1, $2, $3, $4, $5) RETURNING state_string, created_at, expires_at, to_login_type, user_id ` type InsertUserOauthMergeStateParams struct { UserID uuid.UUID `db:"user_id" json:"user_id"` StateString string `db:"state_string" json:"state_string"` - OauthID string `db:"oauth_id" json:"oauth_id"` + ToLoginType LoginType `db:"to_login_type" json:"to_login_type"` CreatedAt time.Time `db:"created_at" json:"created_at"` ExpiresAt time.Time `db:"expires_at" json:"expires_at"` } @@ -5094,7 +5094,7 @@ func (q *sqlQuerier) InsertUserOauthMergeState(ctx context.Context, arg InsertUs row := q.db.QueryRowContext(ctx, insertUserOauthMergeState, arg.UserID, arg.StateString, - arg.OauthID, + arg.ToLoginType, arg.CreatedAt, arg.ExpiresAt, ) @@ -5103,7 +5103,7 @@ func (q *sqlQuerier) InsertUserOauthMergeState(ctx context.Context, arg InsertUs &i.StateString, &i.CreatedAt, &i.ExpiresAt, - &i.OauthID, + &i.ToLoginType, &i.UserID, ) return i, err diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index e8e0457ba81e1..812b12f4facbb 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -4,15 +4,15 @@ SELECT FROM oauth_merge_state WHERE - user_id = @user_id AND - state_string = @state_string; + user_id = @user_id AND + state_string = @state_string; -- name: InsertUserOauthMergeState :one INSERT INTO oauth_merge_state ( user_id, state_string, - oauth_id, + to_login_type, created_at, expires_at ) @@ -60,7 +60,7 @@ SELECT FROM users WHERE - status = 'active'::user_status AND deleted = false; + status = 'active'::user_status AND deleted = false; -- name: GetFilteredUserCount :one -- This will never count deleted users. @@ -197,9 +197,9 @@ WHERE -- Filter by rbac_roles AND CASE -- @rbac_role allows filtering by rbac roles. If 'member' is included, show everyone, as - -- everyone is a member. + -- everyone is a member. WHEN cardinality(@rbac_role :: text[]) > 0 AND 'member' != ANY(@rbac_role :: text[]) THEN - rbac_roles && @rbac_role :: text[] + rbac_roles && @rbac_role :: text[] ELSE true END -- End of filters diff --git a/coderd/userauth.go b/coderd/userauth.go index 8de9228bdf09a..2a1526dd21e37 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -12,8 +12,6 @@ import ( "strings" "time" - "github.com/coder/coder/cryptorand" - "github.com/coreos/go-oidc/v3/oidc" "github.com/google/go-github/v43/github" "github.com/google/uuid" @@ -31,9 +29,12 @@ import ( "github.com/coder/coder/coderd/rbac" "github.com/coder/coder/coderd/userpassword" "github.com/coder/coder/codersdk" + "github.com/coder/coder/cryptorand" ) -func (api *API) postConvertToOauth(rw http.ResponseWriter, r *http.Request) { +// postConvertLoginType replies with an oauth state token capable of converting +// the user to an oauth user. +func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) { var ( ctx = r.Context() auditor = api.Auditor.Load() @@ -49,21 +50,22 @@ func (api *API) postConvertToOauth(rw http.ResponseWriter, r *http.Request) { aReq.Old = database.APIKey{} defer commitAudit() - var req codersdk.UpgradeToOIDCRequest + var req codersdk.ConvertLoginRequest if !httpapi.Read(ctx, rw, r, &req) { return } - var oauthID string - switch strings.ToLower(req.OauthProvider) { - case "oidc": - oauthID = "oidc" - case "github": - oauthID = "github" + switch req.ToLoginType { + case codersdk.LoginTypeGithub, codersdk.LoginTypeOIDC: + case codersdk.LoginTypeNone, codersdk.LoginTypePassword, codersdk.LoginTypeToken: + // These login types are not allowed to be converted to at this time. + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: fmt.Sprintf("Cannot convert to login type %q.", req.ToLoginType), + }) + return default: httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: fmt.Sprintf("Unknown oauth provider %q. Choose from '%s' or '%s'", - req.OauthProvider, "oidc", "github"), + Message: fmt.Sprintf("Unknown login type %q.", req.ToLoginType), }) return } @@ -73,14 +75,14 @@ func (api *API) postConvertToOauth(rw http.ResponseWriter, r *http.Request) { return } - // The user wants to convert their password based authentication to OIDC. + // Only support converting from password auth. if user.LoginType != database.LoginTypePassword { // This is checked in loginRequest, but checked again here in case that shared // function changes its checks. Just some defensive programming. // This login type is **required** to be password based to prevent // users from converting other login types to OIDC. httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "User account must be have password based authentication.", + Message: "User account must have password based authentication.", }) return } @@ -98,7 +100,7 @@ func (api *API) postConvertToOauth(rw http.ResponseWriter, r *http.Request) { mergeState, err := api.Database.InsertUserOauthMergeState(ctx, database.InsertUserOauthMergeStateParams{ UserID: user.ID, StateString: stateString, - OauthID: oauthID, + ToLoginType: database.LoginType(req.ToLoginType), CreatedAt: now, ExpiresAt: now.Add(time.Minute * 5), }) @@ -113,15 +115,9 @@ func (api *API) postConvertToOauth(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusCreated, codersdk.OauthConversionResponse{ StateString: mergeState.StateString, ExpiresAt: mergeState.ExpiresAt, - OAuthID: mergeState.OauthID, + ToLoginType: codersdk.LoginType(mergeState.ToLoginType), UserID: mergeState.UserID, }) - - //// Redirect the user back to where they were after the account merge. - //redirectURL := fmt.Sprintf("/api/v2/users/%s/callback?redirect=%s&oidc_merge_state=%s", oauthID, url.QueryEscape(r.URL.Path), mergeState.StateString) - // - //// Redirect the user to the normal OIDC flow with the special 'oidc_merge_state' state string. - //http.Redirect(rw, r, redirectURL, http.StatusTemporaryRedirect) } // Authenticates the user with an email and password. diff --git a/codersdk/users.go b/codersdk/users.go index 282b1d4ef3f35..e94585c6b6cfc 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -93,8 +93,9 @@ type UserRoles struct { OrganizationRoles map[uuid.UUID][]string `json:"organization_roles"` } -type UpgradeToOIDCRequest struct { - OauthProvider string `json:"oauth_provider" validate:"required"` +type ConvertLoginRequest struct { + // ToLoginType is the login type to convert to. + ToLoginType LoginType `json:"to_login_type" validate:"required"` LoginWithPasswordRequest } @@ -112,7 +113,7 @@ type LoginWithPasswordResponse struct { type OauthConversionResponse struct { StateString string `json:"state_string"` ExpiresAt time.Time `json:"expires_at"` - OAuthID string `json:"oauth_id"` + ToLoginType LoginType `json:"to_login_type"` UserID uuid.UUID `json:"user_id"` } diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index ed7a8b9f4ebb5..62bc65f877615 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -139,6 +139,11 @@ export interface BuildInfoResponse { readonly workspace_proxy: boolean } +// From codersdk/users.go +export interface ConvertLoginRequest extends LoginWithPasswordRequest { + readonly to_login_type: LoginType +} + // From codersdk/users.go export interface CreateFirstUserRequest { readonly email: string @@ -548,7 +553,7 @@ export interface OIDCConfig { export interface OauthConversionResponse { readonly state_string: string readonly expires_at: string - readonly oauth_id: string + readonly to_login_type: LoginType readonly user_id: string } @@ -1013,11 +1018,6 @@ export interface UpdateWorkspaceTTLRequest { readonly ttl_ms?: number } -// From codersdk/users.go -export interface UpgradeToOIDCRequest extends LoginWithPasswordRequest { - readonly oauth_provider: string -} - // From codersdk/files.go export interface UploadResponse { readonly hash: string From 069d6897080e62378cf50abdd173de59d8957a34 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 20 Jun 2023 09:46:01 -0500 Subject: [PATCH 07/58] Add dbfakes, delete mergestates, and implement the convert --- coderd/database/dbauthz/dbauthz.go | 46 +++++++++++------ coderd/database/dbfake/dbfake.go | 71 ++++++++++++++++++++++++-- coderd/database/dbmetrics/dbmetrics.go | 14 +++++ coderd/database/dbmock/dbmock.go | 29 +++++++++++ coderd/database/querier.go | 2 + coderd/database/queries.sql.go | 43 ++++++++++++++++ coderd/database/queries/users.sql | 11 ++++ coderd/httpmw/oauth2.go | 8 ++- coderd/userauth.go | 61 ++++++++++++++++------ 9 files changed, 248 insertions(+), 37 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 1fdcbf1e61ab8..850f3c91b5335 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -671,22 +671,6 @@ func authorizedTemplateVersionFromJob(ctx context.Context, q *querier, job datab } } -func (q *querier) GetUserOauthMergeState(ctx context.Context, arg database.GetUserOauthMergeStateParams) (database.OauthMergeState, error) { - if err := q.authorizeContext(ctx, rbac.ActionRead, rbac.ResourceSystem); err != nil { - return database.OauthMergeState{}, err - } - return q.db.GetUserOauthMergeState(ctx, arg) -} - -func (q *querier) InsertUserOauthMergeState(ctx context.Context, arg database.InsertUserOauthMergeStateParams) (database.OauthMergeState, error) { - // TODO: @emyrk this permission feels right? - if err := q.authorizeContext(ctx, rbac.ActionCreate, rbac.ResourceAPIKey.WithOwner(arg.UserID.String())); err != nil { - return database.OauthMergeState{}, err - } - - return q.db.InsertUserOauthMergeState(ctx, arg) -} - func (q *querier) AcquireLock(ctx context.Context, id int64) error { return q.db.AcquireLock(ctx, id) } @@ -781,6 +765,13 @@ func (q *querier) DeleteReplicasUpdatedBefore(ctx context.Context, updatedAt tim return q.db.DeleteReplicasUpdatedBefore(ctx, updatedAt) } +func (q *querier) DeleteUserOauthMergeStates(ctx context.Context, userID uuid.UUID) error { + if err := q.authorizeContext(ctx, rbac.ActionDelete, rbac.ResourceSystem); err != nil { + return err + } + return q.db.DeleteUserOauthMergeStates(ctx, userID) +} + func (q *querier) GetAPIKeyByID(ctx context.Context, id string) (database.APIKey, error) { return fetch(q.log, q.auth, q.db.GetAPIKeyByID)(ctx, id) } @@ -1352,6 +1343,13 @@ func (q *querier) GetUserLinkByUserIDLoginType(ctx context.Context, arg database return q.db.GetUserLinkByUserIDLoginType(ctx, arg) } +func (q *querier) GetUserOauthMergeState(ctx context.Context, arg database.GetUserOauthMergeStateParams) (database.OauthMergeState, error) { + if err := q.authorizeContext(ctx, rbac.ActionRead, rbac.ResourceSystem); err != nil { + return database.OauthMergeState{}, err + } + return q.db.GetUserOauthMergeState(ctx, arg) +} + func (q *querier) GetUsers(ctx context.Context, arg database.GetUsersParams) ([]database.GetUsersRow, error) { // TODO: We should use GetUsersWithCount with a better method signature. return fetchWithPostFilter(q.auth, q.db.GetUsers)(ctx, arg) @@ -1866,6 +1864,15 @@ func (q *querier) InsertUserLink(ctx context.Context, arg database.InsertUserLin return q.db.InsertUserLink(ctx, arg) } +func (q *querier) InsertUserOauthMergeState(ctx context.Context, arg database.InsertUserOauthMergeStateParams) (database.OauthMergeState, error) { + // TODO: @emyrk this permission feels right? + if err := q.authorizeContext(ctx, rbac.ActionCreate, rbac.ResourceAPIKey.WithOwner(arg.UserID.String())); err != nil { + return database.OauthMergeState{}, err + } + + return q.db.InsertUserOauthMergeState(ctx, arg) +} + func (q *querier) InsertWorkspace(ctx context.Context, arg database.InsertWorkspaceParams) (database.Workspace, error) { obj := rbac.ResourceWorkspace.WithOwner(arg.OwnerID.String()).InOrg(arg.OrganizationID) return insert(q.log, q.auth, obj, q.db.InsertWorkspace)(ctx, arg) @@ -2276,6 +2283,13 @@ func (q *querier) UpdateUserLinkedID(ctx context.Context, arg database.UpdateUse return q.db.UpdateUserLinkedID(ctx, arg) } +func (q *querier) UpdateUserLoginType(ctx context.Context, arg database.UpdateUserLoginTypeParams) (database.User, error) { + if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceSystem); err != nil { + return database.User{}, err + } + return q.db.UpdateUserLoginType(ctx, arg) +} + func (q *querier) UpdateUserProfile(ctx context.Context, arg database.UpdateUserProfileParams) (database.User, error) { u, err := q.db.GetUserByID(ctx, arg.ID) if err != nil { diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index f0aa04aa2fde2..fd7e29bc52a16 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -137,6 +137,7 @@ type data struct { workspaceResources []database.WorkspaceResource workspaces []database.Workspace workspaceProxies []database.WorkspaceProxy + oauthMergeStates []database.OauthMergeState // Locks is a map of lock names. Any keys within the map are currently // locked. @@ -1174,6 +1175,27 @@ func (q *fakeQuerier) DeleteReplicasUpdatedBefore(_ context.Context, before time return nil } +func (q *fakeQuerier) DeleteUserOauthMergeStates(ctx context.Context, userID uuid.UUID) error { + q.mutex.RLock() + defer q.mutex.RUnlock() + + i := 0 + for { + if i >= len(q.oauthMergeStates) { + break + } + k := q.oauthMergeStates[i] + if k.UserID == userID { + q.oauthMergeStates[i] = q.oauthMergeStates[len(q.oauthMergeStates)-1] + q.oauthMergeStates = q.oauthMergeStates[:len(q.oauthMergeStates)-1] + // We removed an element, so decrement + i-- + } + i++ + } + return nil +} + func (q *fakeQuerier) GetAPIKeyByID(_ context.Context, id string) (database.APIKey, error) { q.mutex.RLock() defer q.mutex.RUnlock() @@ -2549,8 +2571,16 @@ func (q *fakeQuerier) GetUserLinkByUserIDLoginType(_ context.Context, params dat return database.UserLink{}, sql.ErrNoRows } -func (q *fakeQuerier) GetUserOauthMergeState(ctx context.Context, arg database.GetUserOauthMergeStateParams) (database.OauthMergeState, error) { - panic("Not implemented") +func (q *fakeQuerier) GetUserOauthMergeState(_ context.Context, arg database.GetUserOauthMergeStateParams) (database.OauthMergeState, error) { + q.mutex.RLock() + defer q.mutex.RUnlock() + + for _, s := range q.oauthMergeStates { + if s.StateString == arg.StateString && s.UserID == arg.UserID { + return s, nil + } + } + return database.OauthMergeState{}, sql.ErrNoRows } func (q *fakeQuerier) GetUsers(_ context.Context, params database.GetUsersParams) ([]database.GetUsersRow, error) { @@ -3930,8 +3960,23 @@ func (q *fakeQuerier) InsertUserLink(_ context.Context, args database.InsertUser return link, nil } -func (q *fakeQuerier) InsertUserOauthMergeState(ctx context.Context, arg database.InsertUserOauthMergeStateParams) (database.OauthMergeState, error) { - panic("Not implemented") +func (q *fakeQuerier) InsertUserOauthMergeState(_ context.Context, arg database.InsertUserOauthMergeStateParams) (database.OauthMergeState, error) { + q.mutex.RLock() + defer q.mutex.RUnlock() + + if err := validateDatabaseType(arg); err != nil { + return database.OauthMergeState{}, err + } + + s := database.OauthMergeState{ + StateString: arg.StateString, + CreatedAt: arg.CreatedAt, + ExpiresAt: arg.ExpiresAt, + ToLoginType: arg.ToLoginType, + UserID: arg.UserID, + } + q.oauthMergeStates = append(q.oauthMergeStates, s) + return s, nil } func (q *fakeQuerier) InsertWorkspace(_ context.Context, arg database.InsertWorkspaceParams) (database.Workspace, error) { @@ -4755,6 +4800,24 @@ func (q *fakeQuerier) UpdateUserLinkedID(_ context.Context, params database.Upda return database.UserLink{}, sql.ErrNoRows } +func (q *fakeQuerier) UpdateUserLoginType(_ context.Context, arg database.UpdateUserLoginTypeParams) (database.User, error) { + if err := validateDatabaseType(arg); err != nil { + return database.User{}, err + } + + q.mutex.Lock() + defer q.mutex.Unlock() + + for i, u := range q.users { + if u.ID == arg.UserID { + u.LoginType = arg.LoginType + q.users[i] = u + return u, nil + } + } + return database.User{}, sql.ErrNoRows +} + func (q *fakeQuerier) UpdateUserProfile(_ context.Context, arg database.UpdateUserProfileParams) (database.User, error) { if err := validateDatabaseType(arg); err != nil { return database.User{}, err diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index 12266c129823a..e69ebc2900f0e 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -199,6 +199,13 @@ func (m metricsStore) DeleteReplicasUpdatedBefore(ctx context.Context, updatedAt return err } +func (m metricsStore) DeleteUserOauthMergeStates(ctx context.Context, userID uuid.UUID) error { + start := time.Now() + r0 := m.s.DeleteUserOauthMergeStates(ctx, userID) + m.queryLatencies.WithLabelValues("DeleteUserOauthMergeStates").Observe(time.Since(start).Seconds()) + return r0 +} + func (m metricsStore) GetAPIKeyByID(ctx context.Context, id string) (database.APIKey, error) { start := time.Now() apiKey, err := m.s.GetAPIKeyByID(ctx, id) @@ -1382,6 +1389,13 @@ func (m metricsStore) UpdateUserLinkedID(ctx context.Context, arg database.Updat return link, err } +func (m metricsStore) UpdateUserLoginType(ctx context.Context, arg database.UpdateUserLoginTypeParams) (database.User, error) { + start := time.Now() + r0, r1 := m.s.UpdateUserLoginType(ctx, arg) + m.queryLatencies.WithLabelValues("UpdateUserLoginType").Observe(time.Since(start).Seconds()) + return r0, r1 +} + func (m metricsStore) UpdateUserProfile(ctx context.Context, arg database.UpdateUserProfileParams) (database.User, error) { start := time.Now() user, err := m.s.UpdateUserProfile(ctx, arg) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 1a3d9bf30bad3..984aaeeee5ff1 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -223,6 +223,20 @@ func (mr *MockStoreMockRecorder) DeleteReplicasUpdatedBefore(arg0, arg1 interfac return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteReplicasUpdatedBefore", reflect.TypeOf((*MockStore)(nil).DeleteReplicasUpdatedBefore), arg0, arg1) } +// DeleteUserOauthMergeStates mocks base method. +func (m *MockStore) DeleteUserOauthMergeStates(arg0 context.Context, arg1 uuid.UUID) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeleteUserOauthMergeStates", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// DeleteUserOauthMergeStates indicates an expected call of DeleteUserOauthMergeStates. +func (mr *MockStoreMockRecorder) DeleteUserOauthMergeStates(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteUserOauthMergeStates", reflect.TypeOf((*MockStore)(nil).DeleteUserOauthMergeStates), arg0, arg1) +} + // GetAPIKeyByID mocks base method. func (m *MockStore) GetAPIKeyByID(arg0 context.Context, arg1 string) (database.APIKey, error) { m.ctrl.T.Helper() @@ -2846,6 +2860,21 @@ func (mr *MockStoreMockRecorder) UpdateUserLinkedID(arg0, arg1 interface{}) *gom return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateUserLinkedID", reflect.TypeOf((*MockStore)(nil).UpdateUserLinkedID), arg0, arg1) } +// UpdateUserLoginType mocks base method. +func (m *MockStore) UpdateUserLoginType(arg0 context.Context, arg1 database.UpdateUserLoginTypeParams) (database.User, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "UpdateUserLoginType", arg0, arg1) + ret0, _ := ret[0].(database.User) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// UpdateUserLoginType indicates an expected call of UpdateUserLoginType. +func (mr *MockStoreMockRecorder) UpdateUserLoginType(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateUserLoginType", reflect.TypeOf((*MockStore)(nil).UpdateUserLoginType), arg0, arg1) +} + // UpdateUserProfile mocks base method. func (m *MockStore) UpdateUserProfile(arg0 context.Context, arg1 database.UpdateUserProfileParams) (database.User, error) { m.ctrl.T.Helper() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index a1171c1fc7b1b..839ecea5ec629 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -39,6 +39,7 @@ type sqlcQuerier interface { DeleteOldWorkspaceAgentStartupLogs(ctx context.Context) error DeleteOldWorkspaceAgentStats(ctx context.Context) error DeleteReplicasUpdatedBefore(ctx context.Context, updatedAt time.Time) error + DeleteUserOauthMergeStates(ctx context.Context, userID uuid.UUID) error GetAPIKeyByID(ctx context.Context, id string) (APIKey, error) // there is no unique constraint on empty token names GetAPIKeyByName(ctx context.Context, arg GetAPIKeyByNameParams) (APIKey, error) @@ -236,6 +237,7 @@ type sqlcQuerier interface { UpdateUserLastSeenAt(ctx context.Context, arg UpdateUserLastSeenAtParams) (User, error) UpdateUserLink(ctx context.Context, arg UpdateUserLinkParams) (UserLink, error) UpdateUserLinkedID(ctx context.Context, arg UpdateUserLinkedIDParams) (UserLink, error) + UpdateUserLoginType(ctx context.Context, arg UpdateUserLoginTypeParams) (User, error) UpdateUserProfile(ctx context.Context, arg UpdateUserProfileParams) (User, error) UpdateUserRoles(ctx context.Context, arg UpdateUserRolesParams) (User, error) UpdateUserStatus(ctx context.Context, arg UpdateUserStatusParams) (User, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 8c0df264894b4..f4d22f9c37578 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -4600,6 +4600,15 @@ func (q *sqlQuerier) UpdateUserLinkedID(ctx context.Context, arg UpdateUserLinke return i, err } +const deleteUserOauthMergeStates = `-- name: DeleteUserOauthMergeStates :exec +DELETE FROM oauth_merge_state WHERE user_id = $1 +` + +func (q *sqlQuerier) DeleteUserOauthMergeStates(ctx context.Context, userID uuid.UUID) error { + _, err := q.db.ExecContext(ctx, deleteUserOauthMergeStates, userID) + return err +} + const getActiveUserCount = `-- name: GetActiveUserCount :one SELECT COUNT(*) @@ -5183,6 +5192,40 @@ func (q *sqlQuerier) UpdateUserLastSeenAt(ctx context.Context, arg UpdateUserLas return i, err } +const updateUserLoginType = `-- name: UpdateUserLoginType :one +UPDATE + users +SET + login_type = $1 +WHERE + id = $2 RETURNING id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, login_type, avatar_url, deleted, last_seen_at +` + +type UpdateUserLoginTypeParams struct { + LoginType LoginType `db:"login_type" json:"login_type"` + UserID uuid.UUID `db:"user_id" json:"user_id"` +} + +func (q *sqlQuerier) UpdateUserLoginType(ctx context.Context, arg UpdateUserLoginTypeParams) (User, error) { + row := q.db.QueryRowContext(ctx, updateUserLoginType, arg.LoginType, arg.UserID) + var i User + err := row.Scan( + &i.ID, + &i.Email, + &i.Username, + &i.HashedPassword, + &i.CreatedAt, + &i.UpdatedAt, + &i.Status, + &i.RBACRoles, + &i.LoginType, + &i.AvatarURL, + &i.Deleted, + &i.LastSeenAt, + ) + return i, err +} + const updateUserProfile = `-- name: UpdateUserProfile :one UPDATE users diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index 812b12f4facbb..5897c5767e7eb 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -19,6 +19,17 @@ INSERT INTO VALUES ($1, $2, $3, $4, $5) RETURNING *; +-- name: DeleteUserOauthMergeStates :exec +DELETE FROM oauth_merge_state WHERE user_id = @user_id; + +-- name: UpdateUserLoginType :one +UPDATE + users +SET + login_type = @login_type +WHERE + id = @user_id RETURNING *; + -- name: GetUserByID :one SELECT * diff --git a/coderd/httpmw/oauth2.go b/coderd/httpmw/oauth2.go index 47747b8cae7a9..d2226b99bd4ff 100644 --- a/coderd/httpmw/oauth2.go +++ b/coderd/httpmw/oauth2.go @@ -97,9 +97,15 @@ func ExtractOAuth2(config OAuth2Config, client *http.Client, authURLOpts map[str // their account with an OIDC account. Their password would have // been required to get to this point, so we do not need to verify // their password again. - // TODO: @emyrk should we check their api key here? oidcMergeState := r.URL.Query().Get("oidc_merge_state") if oidcMergeState != "" { + _, ok := APIKeyOptional(r) + if !ok { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Must be logged in to merge account.", + }) + return + } state = oidcMergeState } else { var err error diff --git a/coderd/userauth.go b/coderd/userauth.go index 2a1526dd21e37..9e2a2b044a533 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -97,13 +97,28 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) { } now := time.Now() - mergeState, err := api.Database.InsertUserOauthMergeState(ctx, database.InsertUserOauthMergeStateParams{ - UserID: user.ID, - StateString: stateString, - ToLoginType: database.LoginType(req.ToLoginType), - CreatedAt: now, - ExpiresAt: now.Add(time.Minute * 5), - }) + var mergeState database.OauthMergeState + err = api.Database.InTx(func(store database.Store) error { + // We should only ever have 1 oauth merge state per user. So delete + // any existing if they exist. + err := api.Database.DeleteUserOauthMergeStates(ctx, user.ID) + if err != nil && !xerrors.Is(err, sql.ErrNoRows) { + return err + } + + mergeState, err = api.Database.InsertUserOauthMergeState(ctx, database.InsertUserOauthMergeStateParams{ + UserID: user.ID, + StateString: stateString, + ToLoginType: database.LoginType(req.ToLoginType), + CreatedAt: now, + ExpiresAt: now.Add(time.Minute * 5), + }) + if err != nil { + return err + } + return nil + }, nil) + if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal Server Error", @@ -970,20 +985,34 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook UserID: user.ID, StateString: params.State.StateString, }) - var _ = mergeState - if err == nil { + failedMsg := fmt.Sprintf("Request to convert login type from %s to %s failed", user.LoginType, params.LoginType) + if err != nil { return httpError{ code: http.StatusForbidden, - msg: "Hey! This upgrade would have worked if I let it", + msg: failedMsg, + } + } + if user.ID != mergeState.UserID { + // User tried to use someone else's merge state? + return httpError{ + code: http.StatusForbidden, + msg: failedMsg, } } - return httpError{ - code: http.StatusForbidden, - msg: fmt.Sprintf("Incorrect login type, attempting to use %q but user is of login type %q", - params.LoginType, - user.LoginType, - ), + // Convert the user and default to the normal login flow. + // If the login succeeds, this transaction will commit and the user + // will be converted. + // nolint:gocritic // system query to update user login type + user, err = tx.UpdateUserLoginType(dbauthz.AsSystemRestricted(ctx), database.UpdateUserLoginTypeParams{ + LoginType: params.LoginType, + UserID: user.ID, + }) + if err != nil { + return httpError{ + code: http.StatusInternalServerError, + msg: "Failed to convert user to new login type", + } } } From 6d6a46e5675b037bea96497989bf57985e4f529f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 20 Jun 2023 11:22:24 -0500 Subject: [PATCH 08/58] Add flag to enable this feature --- coderd/apidoc/docs.go | 3 + coderd/apidoc/swagger.json | 3 + coderd/coderd.go | 10 ++- .../000127_merge_oidc_account.down.sql | 5 ++ coderd/httpmw/oauth2.go | 16 +++-- coderd/userauth.go | 65 ++++++++++++------- codersdk/deployment.go | 10 +++ docs/api/general.md | 1 + docs/api/schemas.md | 3 + docs/cli/server.md | 10 +++ site/src/api/api.ts | 6 +- site/src/api/typesGenerated.ts | 1 + 12 files changed, 99 insertions(+), 34 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index bff3b11e8089b..6d3bacbfa3fba 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -7176,6 +7176,9 @@ const docTemplate = `{ "disable_session_expiry_refresh": { "type": "boolean" }, + "enable_oauth_account_conversion": { + "type": "boolean" + }, "experiments": { "type": "array", "items": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index a990b936ae2cf..c99fa5a20f301 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -6409,6 +6409,9 @@ "disable_session_expiry_refresh": { "type": "boolean" }, + "enable_oauth_account_conversion": { + "type": "boolean" + }, "experiments": { "type": "array", "items": { diff --git a/coderd/coderd.go b/coderd/coderd.go index 6efa66951ff4d..c90e821419866 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -398,6 +398,8 @@ func New(options *Options) *API { Optional: true, }) + allowOauthConversion := options.DeploymentValues.EnableOauthAccountConversion.Value() + // API rate limit middleware. The counter is local and not shared between // replicas or instances of this middleware. apiRateLimiter := httpmw.RateLimit(options.APIRateLimit, time.Minute) @@ -620,7 +622,13 @@ func New(options *Options) *API { r.Use( apiKeyMiddleware, ) - r.Post("/", api.postConvertLoginType) + if allowOauthConversion { + r.Post("/", api.postConvertLoginType) + } else { + r.Post("/", func(rw http.ResponseWriter, r *http.Request) { + http.Error(rw, "Oauth conversion is not allowed, contact an administrator to turn on this feature.", http.StatusForbidden) + }) + } }) }) r.Group(func(r chi.Router) { diff --git a/coderd/database/migrations/000127_merge_oidc_account.down.sql b/coderd/database/migrations/000127_merge_oidc_account.down.sql index e69de29bb2d1d..e622373024f56 100644 --- a/coderd/database/migrations/000127_merge_oidc_account.down.sql +++ b/coderd/database/migrations/000127_merge_oidc_account.down.sql @@ -0,0 +1,5 @@ +BEGIN; + +DROP TABLE oauth_merge_state; + +COMMIT; diff --git a/coderd/httpmw/oauth2.go b/coderd/httpmw/oauth2.go index d2226b99bd4ff..1012b6f5327e6 100644 --- a/coderd/httpmw/oauth2.go +++ b/coderd/httpmw/oauth2.go @@ -99,13 +99,6 @@ func ExtractOAuth2(config OAuth2Config, client *http.Client, authURLOpts map[str // their password again. oidcMergeState := r.URL.Query().Get("oidc_merge_state") if oidcMergeState != "" { - _, ok := APIKeyOptional(r) - if !ok { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Must be logged in to merge account.", - }) - return - } state = oidcMergeState } else { var err error @@ -185,3 +178,12 @@ func ExtractOAuth2(config OAuth2Config, client *http.Client, authURLOpts map[str }) } } + +func RemoveOauthStateCookie(rw http.ResponseWriter) { + http.SetCookie(rw, &http.Cookie{ + Name: codersdk.OAuth2StateCookie, + Value: "", + Path: "/", + MaxAge: -1, + }) +} diff --git a/coderd/userauth.go b/coderd/userauth.go index 9e2a2b044a533..057c8b4466583 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -101,7 +101,8 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) { err = api.Database.InTx(func(store database.Store) error { // We should only ever have 1 oauth merge state per user. So delete // any existing if they exist. - err := api.Database.DeleteUserOauthMergeStates(ctx, user.ID) + //nolint:gocritic // Keeping the table clean + err := api.Database.DeleteUserOauthMergeStates(dbauthz.AsSystemRestricted(ctx), user.ID) if err != nil && !xerrors.Is(err, sql.ErrNoRows) { return err } @@ -521,15 +522,16 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { } cookie, key, err := api.oauthLogin(r, oauthLoginParams{ - User: user, - Link: link, - State: state, - LinkedID: githubLinkedID(ghUser), - LoginType: database.LoginTypeGithub, - AllowSignups: api.GithubOAuth2Config.AllowSignups, - Email: verifiedEmail.GetEmail(), - Username: ghUser.GetLogin(), - AvatarURL: ghUser.GetAvatarURL(), + User: user, + Link: link, + State: state, + LinkedID: githubLinkedID(ghUser), + LoginType: database.LoginTypeGithub, + AllowSignups: api.GithubOAuth2Config.AllowSignups, + Email: verifiedEmail.GetEmail(), + Username: ghUser.GetLogin(), + AvatarURL: ghUser.GetAvatarURL(), + OauthConversionEnabled: api.DeploymentValues.EnableOauthAccountConversion.Value(), }) var httpErr httpError if xerrors.As(err, &httpErr) { @@ -850,17 +852,18 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { } cookie, key, err := api.oauthLogin(r, oauthLoginParams{ - User: user, - Link: link, - State: state, - LinkedID: oidcLinkedID(idToken), - LoginType: database.LoginTypeOIDC, - AllowSignups: api.OIDCConfig.AllowSignups, - Email: email, - Username: username, - AvatarURL: picture, - UsingGroups: usingGroups, - Groups: groups, + User: user, + Link: link, + State: state, + LinkedID: oidcLinkedID(idToken), + LoginType: database.LoginTypeOIDC, + AllowSignups: api.OIDCConfig.AllowSignups, + Email: email, + Username: username, + AvatarURL: picture, + UsingGroups: usingGroups, + Groups: groups, + OauthConversionEnabled: api.DeploymentValues.EnableOauthAccountConversion.Value(), }) var httpErr httpError if xerrors.As(err, &httpErr) { @@ -940,8 +943,9 @@ type oauthLoginParams struct { AvatarURL string // Is UsingGroups is true, then the user will be assigned // to the Groups provided. - UsingGroups bool - Groups []string + UsingGroups bool + Groups []string + OauthConversionEnabled bool } type httpError struct { @@ -980,11 +984,26 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook } } + // If this is not a new user and the login type is different, + // we need to check if the user is trying to change their login type. if user.ID != uuid.Nil && user.LoginType != params.LoginType { + wrongLoginTypeErr := httpError{ + code: http.StatusForbidden, + msg: fmt.Sprintf("Incorrect login type, attempting to use %q but user is of login type %q", + params.LoginType, + user.LoginType, + ), + } + if !params.OauthConversionEnabled { + return wrongLoginTypeErr + } mergeState, err := api.Database.GetUserOauthMergeState(dbauthz.AsSystemRestricted(ctx), database.GetUserOauthMergeStateParams{ UserID: user.ID, StateString: params.State.StateString, }) + if xerrors.Is(err, sql.ErrNoRows) { + return wrongLoginTypeErr + } failedMsg := fmt.Sprintf("Request to convert login type from %s to %s failed", user.LoginType, params.LoginType) if err != nil { return httpError{ diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 6972e94e3a338..6fac75fd90a2d 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -158,6 +158,7 @@ type DeploymentValues struct { SessionDuration clibase.Duration `json:"max_session_expiry,omitempty" typescript:",notnull"` DisableSessionExpiryRefresh clibase.Bool `json:"disable_session_expiry_refresh,omitempty" typescript:",notnull"` DisablePasswordAuth clibase.Bool `json:"disable_password_auth,omitempty" typescript:",notnull"` + EnableOauthAccountConversion clibase.Bool `json:"enable_oauth_account_conversion,omitempty" typescript:",notnull"` Support SupportConfig `json:"support,omitempty" typescript:",notnull"` GitAuthProviders clibase.Struct[[]GitAuthConfig] `json:"git_auth,omitempty" typescript:",notnull"` SSHConfig SSHConfig `json:"config_ssh,omitempty" typescript:",notnull"` @@ -1426,6 +1427,15 @@ when required by your organization's security policy.`, Group: &deploymentGroupNetworkingHTTP, YAML: "disablePasswordAuth", }, + { + Name: "Enable Oauth account conversion", + Description: "If enabled, users can switch from password based authentication to oauth based authentication by logging into an oidc account with the same email address.", + Flag: "enable-oauth-auth-conversion", + Env: "CODER_ENABLE_OAUTH_AUTH_CONVERSION", + Value: &c.EnableOauthAccountConversion, + Group: &deploymentGroupNetworkingHTTP, + YAML: "enableOauthAuthConversion", + }, { Name: "Config Path", Description: `Specify a YAML file to load configuration from.`, diff --git a/docs/api/general.md b/docs/api/general.md index 57fe3716efc22..0b85bc8293af3 100644 --- a/docs/api/general.md +++ b/docs/api/general.md @@ -195,6 +195,7 @@ curl -X GET http://coder-server:8080/api/v2/deployment/config \ "disable_password_auth": true, "disable_path_apps": true, "disable_session_expiry_refresh": true, + "enable_oauth_account_conversion": true, "experiments": ["string"], "git_auth": { "value": [ diff --git a/docs/api/schemas.md b/docs/api/schemas.md index 28d63f3628daa..73a29854358ec 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -1863,6 +1863,7 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in "disable_password_auth": true, "disable_path_apps": true, "disable_session_expiry_refresh": true, + "enable_oauth_account_conversion": true, "experiments": ["string"], "git_auth": { "value": [ @@ -2190,6 +2191,7 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in "disable_password_auth": true, "disable_path_apps": true, "disable_session_expiry_refresh": true, + "enable_oauth_account_conversion": true, "experiments": ["string"], "git_auth": { "value": [ @@ -2381,6 +2383,7 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in | `disable_password_auth` | boolean | false | | | | `disable_path_apps` | boolean | false | | | | `disable_session_expiry_refresh` | boolean | false | | | +| `enable_oauth_account_conversion` | boolean | false | | | | `experiments` | array of string | false | | | | `git_auth` | [clibase.Struct-array_codersdk_GitAuthConfig](#clibasestruct-array_codersdk_gitauthconfig) | false | | | | `http_address` | string | false | | Http address is a string because it may be set to zero to disable. | diff --git a/docs/cli/server.md b/docs/cli/server.md index 9dcd9e8d9cdeb..1564d73b9fcee 100644 --- a/docs/cli/server.md +++ b/docs/cli/server.md @@ -213,6 +213,16 @@ Disable workspace apps that are not served from subdomains. Path-based apps can Disable automatic session expiry bumping due to activity. This forces all sessions to become invalid after the session expiry duration has been reached. +### --enable-oauth-auth-conversion + +| | | +| ----------- | ------------------------------------------------------ | +| Type | bool | +| Environment | $CODER_ENABLE_OAUTH_AUTH_CONVERSION | +| YAML | networking.http.enableOauthAuthConversion | + +If enabled, users can switch from password based authentication to oauth based authentication by logging into an oidc account with the same email address. + ### --swagger-enable | | | diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 5343193338ca5..79c1e1d4c4537 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -110,16 +110,16 @@ export const login = async ( export const convertToOauth = async ( email: string, password: string, - oauth_provider: string, + to_login_type: string, ): Promise => { const payload = JSON.stringify({ email, password, - oauth_provider, + to_login_type, }) try { - const response = await axios.post("/api/v2/users/upgrade-to-oidc", + const response = await axios.post("/api/v2/users/convert-login", payload, { headers: { ...CONTENT_TYPE_JSON }, diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 62bc65f877615..5501bf7ec4b10 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -368,6 +368,7 @@ export interface DeploymentValues { readonly max_session_expiry?: number readonly disable_session_expiry_refresh?: boolean readonly disable_password_auth?: boolean + readonly enable_oauth_account_conversion?: boolean readonly support?: SupportConfig // Named type "github.com/coder/coder/cli/clibase.Struct[[]github.com/coder/coder/codersdk.GitAuthConfig]" unknown, using "any" // eslint-disable-next-line @typescript-eslint/no-explicit-any -- External type From a4961b9dba17ff62da03f8465f2e29b00007f4e6 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 20 Jun 2023 14:40:38 -0500 Subject: [PATCH 09/58] Add unit test --- coderd/audit/diff.go | 3 +- coderd/audit/request.go | 7 +++++ coderd/database/dbfake/dbfake.go | 10 +++---- coderd/database/models.go | 1 + coderd/userauth.go | 35 ++++++++++++++++++----- coderd/userauth_test.go | 48 ++++++++++++++++++++++++++++++-- codersdk/deployment.go | 2 ++ codersdk/users.go | 20 +++++++++++++ enterprise/audit/table.go | 7 +++++ 9 files changed, 118 insertions(+), 15 deletions(-) diff --git a/coderd/audit/diff.go b/coderd/audit/diff.go index 965c0e0851c02..d6898571cab41 100644 --- a/coderd/audit/diff.go +++ b/coderd/audit/diff.go @@ -17,7 +17,8 @@ type Auditable interface { database.WorkspaceBuild | database.AuditableGroup | database.License | - database.WorkspaceProxy + database.WorkspaceProxy | + database.OauthMergeState } // Map is a map of changed fields in an audited resource. It maps field names to diff --git a/coderd/audit/request.go b/coderd/audit/request.go index b060ad1f3949f..dfbebb796fcd6 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -84,6 +84,8 @@ func ResourceTarget[T Auditable](tgt T) string { return strconv.Itoa(int(typed.ID)) case database.WorkspaceProxy: return typed.Name + case database.OauthMergeState: + return typed.StateString default: panic(fmt.Sprintf("unknown resource %T", tgt)) } @@ -111,6 +113,9 @@ func ResourceID[T Auditable](tgt T) uuid.UUID { return typed.UUID case database.WorkspaceProxy: return typed.ID + case database.OauthMergeState: + // The merge state is for the given user + return typed.UserID default: panic(fmt.Sprintf("unknown resource %T", tgt)) } @@ -138,6 +143,8 @@ func ResourceType[T Auditable](tgt T) database.ResourceType { return database.ResourceTypeLicense case database.WorkspaceProxy: return database.ResourceTypeWorkspaceProxy + case database.OauthMergeState: + return database.ResourceTypeConvertLogin default: panic(fmt.Sprintf("unknown resource %T", typed)) } diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index fd7e29bc52a16..3c44681dcba5d 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -1175,9 +1175,9 @@ func (q *fakeQuerier) DeleteReplicasUpdatedBefore(_ context.Context, before time return nil } -func (q *fakeQuerier) DeleteUserOauthMergeStates(ctx context.Context, userID uuid.UUID) error { - q.mutex.RLock() - defer q.mutex.RUnlock() +func (q *fakeQuerier) DeleteUserOauthMergeStates(_ context.Context, userID uuid.UUID) error { + q.mutex.Lock() + defer q.mutex.Unlock() i := 0 for { @@ -3961,8 +3961,8 @@ func (q *fakeQuerier) InsertUserLink(_ context.Context, args database.InsertUser } func (q *fakeQuerier) InsertUserOauthMergeState(_ context.Context, arg database.InsertUserOauthMergeStateParams) (database.OauthMergeState, error) { - q.mutex.RLock() - defer q.mutex.RUnlock() + q.mutex.Lock() + defer q.mutex.Unlock() if err := validateDatabaseType(arg); err != nil { return database.OauthMergeState{}, err diff --git a/coderd/database/models.go b/coderd/database/models.go index d8c449fd58007..441d6082b1e45 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -891,6 +891,7 @@ const ( ResourceTypeWorkspaceBuild ResourceType = "workspace_build" ResourceTypeLicense ResourceType = "license" ResourceTypeWorkspaceProxy ResourceType = "workspace_proxy" + ResourceTypeConvertLogin ResourceType = "convert_login" ) func (e *ResourceType) Scan(src interface{}) error { diff --git a/coderd/userauth.go b/coderd/userauth.go index 057c8b4466583..c5cedbf9e285d 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -38,16 +38,14 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) { var ( ctx = r.Context() auditor = api.Auditor.Load() - aReq, commitAudit = audit.InitRequest[database.APIKey](rw, &audit.RequestParams{ + aReq, commitAudit = audit.InitRequest[database.OauthMergeState](rw, &audit.RequestParams{ Audit: *auditor, Log: api.Logger, Request: r, Action: database.AuditActionCreate, }) ) - // TODO: @emyrk This does make a new api key. Make a new auditable resource - // for oidc state. - aReq.Old = database.APIKey{} + aReq.Old = database.OauthMergeState{} defer commitAudit() var req codersdk.ConvertLoginRequest @@ -102,12 +100,12 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) { // We should only ever have 1 oauth merge state per user. So delete // any existing if they exist. //nolint:gocritic // Keeping the table clean - err := api.Database.DeleteUserOauthMergeStates(dbauthz.AsSystemRestricted(ctx), user.ID) + err := store.DeleteUserOauthMergeStates(dbauthz.AsSystemRestricted(ctx), user.ID) if err != nil && !xerrors.Is(err, sql.ErrNoRows) { return err } - mergeState, err = api.Database.InsertUserOauthMergeState(ctx, database.InsertUserOauthMergeStateParams{ + mergeState, err = store.InsertUserOauthMergeState(ctx, database.InsertUserOauthMergeStateParams{ UserID: user.ID, StateString: stateString, ToLoginType: database.LoginType(req.ToLoginType), @@ -134,6 +132,7 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) { ToLoginType: codersdk.LoginType(mergeState.ToLoginType), UserID: mergeState.UserID, }) + aReq.New = mergeState } // Authenticates the user with an email and password. @@ -532,6 +531,9 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { Username: ghUser.GetLogin(), AvatarURL: ghUser.GetAvatarURL(), OauthConversionEnabled: api.DeploymentValues.EnableOauthAccountConversion.Value(), + InitAuditRequest: func(params *audit.RequestParams) (*audit.Request[database.OauthMergeState], func()) { + return audit.InitRequest[database.OauthMergeState](rw, params) + }, }) var httpErr httpError if xerrors.As(err, &httpErr) { @@ -864,6 +866,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { UsingGroups: usingGroups, Groups: groups, OauthConversionEnabled: api.DeploymentValues.EnableOauthAccountConversion.Value(), + InitAuditRequest: func(params *audit.RequestParams) (*audit.Request[database.OauthMergeState], func()) { + return audit.InitRequest[database.OauthMergeState](rw, params) + }, }) var httpErr httpError if xerrors.As(err, &httpErr) { @@ -946,6 +951,8 @@ type oauthLoginParams struct { UsingGroups bool Groups []string OauthConversionEnabled bool + + InitAuditRequest func(params *audit.RequestParams) (*audit.Request[database.OauthMergeState], func()) } type httpError struct { @@ -997,13 +1004,27 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook if !params.OauthConversionEnabled { return wrongLoginTypeErr } - mergeState, err := api.Database.GetUserOauthMergeState(dbauthz.AsSystemRestricted(ctx), database.GetUserOauthMergeStateParams{ + var ( + auditor = *api.Auditor.Load() + oauthConvertAudit, commitOauthConvertAudit = params.InitAuditRequest(&audit.RequestParams{ + Audit: auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionLogin, + }) + ) + defer commitOauthConvertAudit() + + // nolint:gocritic // Required to auth the oidc convert + mergeState, err := tx.GetUserOauthMergeState(dbauthz.AsSystemRestricted(ctx), database.GetUserOauthMergeStateParams{ UserID: user.ID, StateString: params.State.StateString, }) if xerrors.Is(err, sql.ErrNoRows) { return wrongLoginTypeErr } + oauthConvertAudit.Old = mergeState + failedMsg := fmt.Sprintf("Request to convert login type from %s to %s failed", user.LoginType, params.LoginType) if err != nil { return httpError{ diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 4af69271cf6af..4edde20b14644 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -782,6 +782,46 @@ func TestUserOIDC(t *testing.T) { }) } + t.Run("OIDCConvert", func(t *testing.T) { + t.Parallel() + auditor := audit.NewMock() + conf := coderdtest.NewOIDCConfig(t, "") + + config := conf.OIDCConfig(t, nil) + config.AllowSignups = true + + cfg := coderdtest.DeploymentValues(t) + cfg.EnableOauthAccountConversion = true + client := coderdtest.New(t, &coderdtest.Options{ + Auditor: auditor, + OIDCConfig: config, + DeploymentValues: cfg, + }) + owner := coderdtest.CreateFirstUser(t, client) + + user, userData := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) + + numLogs := len(auditor.AuditLogs()) + + code := conf.EncodeClaims(t, jwt.MapClaims{ + "email": userData.Email, + }) + + numLogs++ // add an audit log for login + ctx := testutil.Context(t, testutil.WaitShort) + convertResponse, err := user.ConvertToOAuthLogin(ctx, codersdk.ConvertLoginRequest{ + ToLoginType: codersdk.LoginTypeOIDC, + LoginWithPasswordRequest: codersdk.LoginWithPasswordRequest{ + Email: userData.Email, + Password: "SomeSecurePassword!", + }, + }) + require.NoError(t, err) + + resp := oidcCallbackWithState(t, client, code, convertResponse.StateString) + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + }) + t.Run("AlternateUsername", func(t *testing.T) { t.Parallel() auditor := audit.NewMock() @@ -1004,17 +1044,21 @@ func oauth2Callback(t *testing.T, client *codersdk.Client) *http.Response { } func oidcCallback(t *testing.T, client *codersdk.Client, code string) *http.Response { + return oidcCallbackWithState(t, client, code, "somestate") +} + +func oidcCallbackWithState(t *testing.T, client *codersdk.Client, code, state string) *http.Response { t.Helper() client.HTTPClient.CheckRedirect = func(req *http.Request, via []*http.Request) error { return http.ErrUseLastResponse } - oauthURL, err := client.URL.Parse(fmt.Sprintf("/api/v2/users/oidc/callback?code=%s&state=somestate", code)) + oauthURL, err := client.URL.Parse(fmt.Sprintf("/api/v2/users/oidc/callback?code=%s&state=%s", code, state)) require.NoError(t, err) req, err := http.NewRequestWithContext(context.Background(), "GET", oauthURL.String(), nil) require.NoError(t, err) req.AddCookie(&http.Cookie{ Name: codersdk.OAuth2StateCookie, - Value: "somestate", + Value: state, }) res, err := client.HTTPClient.Do(req) require.NoError(t, err) diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 6fac75fd90a2d..013f1c532f5bc 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -1435,6 +1435,8 @@ when required by your organization's security policy.`, Value: &c.EnableOauthAccountConversion, Group: &deploymentGroupNetworkingHTTP, YAML: "enableOauthAuthConversion", + // Do not show this until the feature is fully ready. + Hidden: true, }, { Name: "Config Path", diff --git a/codersdk/users.go b/codersdk/users.go index e94585c6b6cfc..3241cce07030e 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -312,6 +312,26 @@ func (c *Client) LoginWithPassword(ctx context.Context, req LoginWithPasswordReq return resp, nil } +// ConvertToOAuthLogin will send a request to convert the user from password +// based authentication to oauth based. The response has the oauth state code +// to use in the oauth flow. +func (c *Client) ConvertToOAuthLogin(ctx context.Context, req ConvertLoginRequest) (OauthConversionResponse, error) { + res, err := c.Request(ctx, http.MethodPost, "/api/v2/users/convert-login", req) + if err != nil { + return OauthConversionResponse{}, err + } + defer res.Body.Close() + if res.StatusCode != http.StatusCreated { + return OauthConversionResponse{}, ReadBodyAsError(res) + } + var resp OauthConversionResponse + err = json.NewDecoder(res.Body).Decode(&resp) + if err != nil { + return OauthConversionResponse{}, err + } + return resp, nil +} + // Logout calls the /logout API // Call `ClearSessionToken()` to clear the session token of the client. func (c *Client) Logout(ctx context.Context) error { diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go index c8b90b8b23567..6972063e5168c 100644 --- a/enterprise/audit/table.go +++ b/enterprise/audit/table.go @@ -155,6 +155,13 @@ var auditableResourcesTypes = map[any]map[string]Action{ "scope": ActionIgnore, "token_name": ActionIgnore, }, + &database.OauthMergeState{}: { + "state_string": ActionSecret, + "created_at": ActionTrack, + "expires_at": ActionTrack, + "to_login_type": ActionTrack, + "user_id": ActionTrack, + }, // TODO: track an ID here when the below ticket is completed: // https://github.com/coder/coder/pull/6012 &database.License{}: { From ab94b85d3669883b11b23fd07d5df50c5ffb743c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 20 Jun 2023 16:01:38 -0500 Subject: [PATCH 10/58] Add FE unit test for audit logS --- coderd/apidoc/docs.go | 6 ++- coderd/apidoc/swagger.json | 6 ++- coderd/audit.go | 4 ++ coderd/audit/request.go | 2 +- coderd/database/dump.sql | 3 +- .../000127_merge_oidc_account.up.sql | 4 ++ coderd/database/models.go | 4 +- coderd/userauth.go | 2 +- codersdk/audit.go | 3 ++ docs/admin/audit-logs.md | 1 + docs/api/schemas.md | 1 + docs/cli/server.md | 10 ----- site/src/api/typesGenerated.ts | 2 + .../AuditLogDescription.test.tsx | 20 ++++++++++ site/src/testHelpers/entities.ts | 37 +++++++++++++++++++ 15 files changed, 87 insertions(+), 18 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 6d3bacbfa3fba..497321154a7de 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -8275,7 +8275,8 @@ const docTemplate = `{ "git_ssh_key", "api_key", "group", - "license" + "license", + "convert_login" ], "x-enum-varnames": [ "ResourceTypeTemplate", @@ -8286,7 +8287,8 @@ const docTemplate = `{ "ResourceTypeGitSSHKey", "ResourceTypeAPIKey", "ResourceTypeGroup", - "ResourceTypeLicense" + "ResourceTypeLicense", + "ResourceTypeConvertLogin" ] }, "codersdk.Response": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index c99fa5a20f301..5623c6e2542c1 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -7432,7 +7432,8 @@ "git_ssh_key", "api_key", "group", - "license" + "license", + "convert_login" ], "x-enum-varnames": [ "ResourceTypeTemplate", @@ -7443,7 +7444,8 @@ "ResourceTypeGitSSHKey", "ResourceTypeAPIKey", "ResourceTypeGroup", - "ResourceTypeLicense" + "ResourceTypeLicense", + "ResourceTypeConvertLogin" ] }, "codersdk.Response": { diff --git a/coderd/audit.go b/coderd/audit.go index e0f000c495a3a..394ea565fafdc 100644 --- a/coderd/audit.go +++ b/coderd/audit.go @@ -270,6 +270,10 @@ func auditLogDescription(alog database.GetAuditLogsOffsetRow) string { str += fmt.Sprintf(" %s", codersdk.ResourceType(alog.ResourceType).FriendlyString()) + if alog.ResourceType == database.ResourceTypeConvertLogin { + str += " to" + } + str += " {target}" return str diff --git a/coderd/audit/request.go b/coderd/audit/request.go index dfbebb796fcd6..1fe5e02ccb1f8 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -85,7 +85,7 @@ func ResourceTarget[T Auditable](tgt T) string { case database.WorkspaceProxy: return typed.Name case database.OauthMergeState: - return typed.StateString + return string(typed.ToLoginType) default: panic(fmt.Sprintf("unknown resource %T", tgt)) } diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index c6616bbe88477..efcf5f8138d50 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -99,7 +99,8 @@ CREATE TYPE resource_type AS ENUM ( 'group', 'workspace_build', 'license', - 'workspace_proxy' + 'workspace_proxy', + 'convert_login' ); CREATE TYPE startup_script_behavior AS ENUM ( diff --git a/coderd/database/migrations/000127_merge_oidc_account.up.sql b/coderd/database/migrations/000127_merge_oidc_account.up.sql index 066850c76929e..4c6f0e9ea2234 100644 --- a/coderd/database/migrations/000127_merge_oidc_account.up.sql +++ b/coderd/database/migrations/000127_merge_oidc_account.up.sql @@ -18,3 +18,7 @@ COMMENT ON COLUMN oauth_merge_state.expires_at IS 'The time at which the state s COMMENT ON COLUMN oauth_merge_state.to_login_type IS 'The login type the user is converting to. Should be github or oidc.'; COMMIT; + + +-- This has to be outside a transaction +ALTER TYPE resource_type ADD VALUE IF NOT EXISTS 'convert_login'; diff --git a/coderd/database/models.go b/coderd/database/models.go index 441d6082b1e45..0a939adf44064 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -941,7 +941,8 @@ func (e ResourceType) Valid() bool { ResourceTypeGroup, ResourceTypeWorkspaceBuild, ResourceTypeLicense, - ResourceTypeWorkspaceProxy: + ResourceTypeWorkspaceProxy, + ResourceTypeConvertLogin: return true } return false @@ -960,6 +961,7 @@ func AllResourceTypeValues() []ResourceType { ResourceTypeWorkspaceBuild, ResourceTypeLicense, ResourceTypeWorkspaceProxy, + ResourceTypeConvertLogin, } } diff --git a/coderd/userauth.go b/coderd/userauth.go index c5cedbf9e285d..410c89258cad9 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -126,13 +126,13 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) { return } + aReq.New = mergeState httpapi.Write(ctx, rw, http.StatusCreated, codersdk.OauthConversionResponse{ StateString: mergeState.StateString, ExpiresAt: mergeState.ExpiresAt, ToLoginType: codersdk.LoginType(mergeState.ToLoginType), UserID: mergeState.UserID, }) - aReq.New = mergeState } // Authenticates the user with an email and password. diff --git a/codersdk/audit.go b/codersdk/audit.go index 061f98f118ba6..1ff2fb8ac97a1 100644 --- a/codersdk/audit.go +++ b/codersdk/audit.go @@ -23,6 +23,7 @@ const ( ResourceTypeAPIKey ResourceType = "api_key" ResourceTypeGroup ResourceType = "group" ResourceTypeLicense ResourceType = "license" + ResourceTypeConvertLogin ResourceType = "convert_login" ) func (r ResourceType) FriendlyString() string { @@ -47,6 +48,8 @@ func (r ResourceType) FriendlyString() string { return "group" case ResourceTypeLicense: return "license" + case ResourceTypeConvertLogin: + return "login type conversion" default: return "unknown" } diff --git a/docs/admin/audit-logs.md b/docs/admin/audit-logs.md index 85b1e8aa7dbb6..fccbf69baea9f 100644 --- a/docs/admin/audit-logs.md +++ b/docs/admin/audit-logs.md @@ -15,6 +15,7 @@ We track the following resources: | Group
create, write, delete |
FieldTracked
avatar_urltrue
idtrue
memberstrue
nametrue
organization_idfalse
quota_allowancetrue
| | GitSSHKey
create |
FieldTracked
created_atfalse
private_keytrue
public_keytrue
updated_atfalse
user_idtrue
| | License
create, delete |
FieldTracked
exptrue
idfalse
jwtfalse
uploaded_attrue
uuidtrue
| +| OauthMergeState
|
FieldTracked
created_attrue
expires_attrue
state_stringtrue
to_login_typetrue
user_idtrue
| | Template
write, delete |
FieldTracked
active_version_idtrue
allow_user_autostarttrue
allow_user_autostoptrue
allow_user_cancel_workspace_jobstrue
created_atfalse
created_bytrue
default_ttltrue
deletedfalse
descriptiontrue
display_nametrue
failure_ttltrue
group_acltrue
icontrue
idtrue
inactivity_ttltrue
max_ttltrue
nametrue
organization_idfalse
provisionertrue
updated_atfalse
user_acltrue
| | TemplateVersion
create, write |
FieldTracked
created_atfalse
created_bytrue
git_auth_providersfalse
idtrue
job_idfalse
nametrue
organization_idfalse
readmetrue
template_idtrue
updated_atfalse
| | User
create, write, delete |
FieldTracked
avatar_urlfalse
created_atfalse
deletedtrue
emailtrue
hashed_passwordtrue
idtrue
last_seen_atfalse
login_typefalse
rbac_rolestrue
statustrue
updated_atfalse
usernametrue
| diff --git a/docs/api/schemas.md b/docs/api/schemas.md index 73a29854358ec..967cb1a725100 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -3485,6 +3485,7 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in | `api_key` | | `group` | | `license` | +| `convert_login` | ## codersdk.Response diff --git a/docs/cli/server.md b/docs/cli/server.md index 1564d73b9fcee..9dcd9e8d9cdeb 100644 --- a/docs/cli/server.md +++ b/docs/cli/server.md @@ -213,16 +213,6 @@ Disable workspace apps that are not served from subdomains. Path-based apps can Disable automatic session expiry bumping due to activity. This forces all sessions to become invalid after the session expiry duration has been reached. -### --enable-oauth-auth-conversion - -| | | -| ----------- | ------------------------------------------------------ | -| Type | bool | -| Environment | $CODER_ENABLE_OAUTH_AUTH_CONVERSION | -| YAML | networking.http.enableOauthAuthConversion | - -If enabled, users can switch from password based authentication to oauth based authentication by logging into an oidc account with the same email address. - ### --swagger-enable | | | diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 5501bf7ec4b10..9dd30ab80abdc 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -1514,6 +1514,7 @@ export const RBACResources: RBACResource[] = [ // From codersdk/audit.go export type ResourceType = | "api_key" + | "convert_login" | "git_ssh_key" | "group" | "license" @@ -1524,6 +1525,7 @@ export type ResourceType = | "workspace_build" export const ResourceTypes: ResourceType[] = [ "api_key", + "convert_login", "git_ssh_key", "group", "license", diff --git a/site/src/components/AuditLogRow/AuditLogDescription/AuditLogDescription.test.tsx b/site/src/components/AuditLogRow/AuditLogDescription/AuditLogDescription.test.tsx index be9be76b5ac27..1c47fce2e23e2 100644 --- a/site/src/components/AuditLogRow/AuditLogDescription/AuditLogDescription.test.tsx +++ b/site/src/components/AuditLogRow/AuditLogDescription/AuditLogDescription.test.tsx @@ -4,6 +4,7 @@ import { MockWorkspaceCreateAuditLogForDifferentOwner, MockAuditLogSuccessfulLogin, MockAuditLogUnsuccessfulLoginKnownUser, + MockAuditOauthConvert, } from "testHelpers/entities" import { AuditLogDescription } from "./AuditLogDescription" import { AuditLogRow } from "../AuditLogRow" @@ -84,6 +85,25 @@ describe("AuditLogDescription", () => { const statusPill = screen.getByRole("status") expect(statusPill).toHaveTextContent("201") }) + it("renders the correct string for login type conversion", async () => { + render() + + expect( + screen.getByText( + t("auditLog:table.logRow.description.unlinkedAuditDescription", { + truncatedDescription: `${MockAuditOauthConvert.user?.username} created login type conversion to ${MockAuditOauthConvert.resource_target}`, + target: "", + onBehalfOf: undefined, + }) + .replace(/<[^>]*>/g, " ") + .replace(/\s{2,}/g, " ") + .trim(), + ), + ).toBeInTheDocument() + + const statusPill = screen.getByRole("status") + expect(statusPill).toHaveTextContent("201") + }) it("renders the correct string for unsuccessful login for a known user", async () => { render() diff --git a/site/src/testHelpers/entities.ts b/site/src/testHelpers/entities.ts index 6697f83c8ddb2..b03ad3a897da7 100644 --- a/site/src/testHelpers/entities.ts +++ b/site/src/testHelpers/entities.ts @@ -1503,6 +1503,43 @@ export const MockAuditLogGitSSH: TypesGen.AuditLog = { }, } +export const MockAuditOauthConvert: TypesGen.AuditLog = { + ...MockAuditLog, + resource_type: "convert_login", + resource_target: "oidc", + action: "create", + status_code: 201, + description: "{user} created login type conversion to {target}}", + diff: { + created_at: { + old: "0001-01-01T00:00:00Z", + new: "2023-06-20T20:44:54.243019Z", + secret: false + }, + expires_at: { + old: "0001-01-01T00:00:00Z", + new: "2023-06-20T20:49:54.243019Z", + secret: false + }, + state_string: { + old: "", + new: "", + secret: true + }, + to_login_type: { + old: "", + new: "oidc", + secret: false + }, + user_id: { + old: "", + new: "dc790496-eaec-4f88-a53f-8ce1f61a1fff", + secret: false + } + }, +} + + export const MockAuditLogSuccessfulLogin: TypesGen.AuditLog = { ...MockAuditLog, resource_type: "api_key", From 8390fb825cc2cf53e0ab8bf6d8713ab7da1a1e90 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 20 Jun 2023 16:17:01 -0500 Subject: [PATCH 11/58] Conditionally show account setting --- coderd/apidoc/docs.go | 8 +++ coderd/apidoc/swagger.json | 8 +++ coderd/coderd.go | 6 ++- coderd/userauth.go | 16 ++++++ codersdk/users.go | 9 ++-- docs/api/schemas.md | 12 +++-- docs/api/users.md | 1 + site/src/api/typesGenerated.ts | 1 + .../AccountPage/AccountPage.tsx | 54 ++++++++++--------- 9 files changed, 82 insertions(+), 33 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 497321154a7de..ec463761a45fe 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -6469,6 +6469,14 @@ const docTemplate = `{ "github": { "$ref": "#/definitions/codersdk.AuthMethod" }, + "me_login_type": { + "description": "UserAuthenticationType returns the authentication method for the given\ncaller if the request is an authenticated request. Otherwise it is empty.", + "allOf": [ + { + "$ref": "#/definitions/codersdk.LoginType" + } + ] + }, "oidc": { "$ref": "#/definitions/codersdk.OIDCAuthMethod" }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 5623c6e2542c1..82849a8e9b25a 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -5758,6 +5758,14 @@ "github": { "$ref": "#/definitions/codersdk.AuthMethod" }, + "me_login_type": { + "description": "UserAuthenticationType returns the authentication method for the given\ncaller if the request is an authenticated request. Otherwise it is empty.", + "allOf": [ + { + "$ref": "#/definitions/codersdk.LoginType" + } + ] + }, "oidc": { "$ref": "#/definitions/codersdk.OIDCAuthMethod" }, diff --git a/coderd/coderd.go b/coderd/coderd.go index c90e821419866..2b13a178c2338 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -597,7 +597,11 @@ func New(options *Options) *API { r.Route("/users", func(r chi.Router) { r.Get("/first", api.firstUser) r.Post("/first", api.postFirstUser) - r.Get("/authmethods", api.userAuthMethods) + r.Route("/authmethods", func(r chi.Router) { + r.Use(apiKeyMiddlewareOptional) + r.Get("/", api.userAuthMethods) + }) + r.Group(func(r chi.Router) { // We use a tight limit for password login to protect against // audit-log write DoS, pbkdf2 DoS, and simple brute-force diff --git a/coderd/userauth.go b/coderd/userauth.go index 410c89258cad9..527e7696bd167 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -362,6 +362,21 @@ type GithubOAuth2Config struct { // @Success 200 {object} codersdk.AuthMethods // @Router /users/authmethods [get] func (api *API) userAuthMethods(rw http.ResponseWriter, r *http.Request) { + var ( + key, ok = httpmw.APIKeyOptional(r) + ) + var currentUserLoginType codersdk.LoginType + if ok { + user, err := api.Database.GetUserByID(r.Context(), key.UserID) + if err != nil { + httpapi.Write(r.Context(), rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error.", + Detail: err.Error(), + }) + return + } + currentUserLoginType = codersdk.LoginType(user.LoginType) + } var signInText string var iconURL string @@ -373,6 +388,7 @@ func (api *API) userAuthMethods(rw http.ResponseWriter, r *http.Request) { } httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.AuthMethods{ + UserAuthenticationType: currentUserLoginType, Password: codersdk.AuthMethod{ Enabled: !api.DeploymentValues.DisablePasswordAuth.Value(), }, diff --git a/codersdk/users.go b/codersdk/users.go index 3241cce07030e..0bfb2e424c5c9 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -123,9 +123,12 @@ type CreateOrganizationRequest struct { // AuthMethods contains authentication method information like whether they are enabled or not or custom text, etc. type AuthMethods struct { - Password AuthMethod `json:"password"` - Github AuthMethod `json:"github"` - OIDC OIDCAuthMethod `json:"oidc"` + // UserAuthenticationType returns the authentication method for the given + // caller if the request is an authenticated request. Otherwise it is empty. + UserAuthenticationType LoginType `json:"me_login_type,omitempty"` + Password AuthMethod `json:"password"` + Github AuthMethod `json:"github"` + OIDC OIDCAuthMethod `json:"oidc"` } type AuthMethod struct { diff --git a/docs/api/schemas.md b/docs/api/schemas.md index 967cb1a725100..6d713c4fd361b 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -1108,6 +1108,7 @@ "github": { "enabled": true }, + "me_login_type": "password", "oidc": { "enabled": true, "iconUrl": "string", @@ -1121,11 +1122,12 @@ ### Properties -| Name | Type | Required | Restrictions | Description | -| ---------- | -------------------------------------------------- | -------- | ------------ | ----------- | -| `github` | [codersdk.AuthMethod](#codersdkauthmethod) | false | | | -| `oidc` | [codersdk.OIDCAuthMethod](#codersdkoidcauthmethod) | false | | | -| `password` | [codersdk.AuthMethod](#codersdkauthmethod) | false | | | +| Name | Type | Required | Restrictions | Description | +| --------------- | -------------------------------------------------- | -------- | ------------ | --------------------------------------------------------------------------------------------------------------------------------------- | +| `github` | [codersdk.AuthMethod](#codersdkauthmethod) | false | | | +| `me_login_type` | [codersdk.LoginType](#codersdklogintype) | false | | Me login type returns the authentication method for the given caller if the request is an authenticated request. Otherwise it is empty. | +| `oidc` | [codersdk.OIDCAuthMethod](#codersdkoidcauthmethod) | false | | | +| `password` | [codersdk.AuthMethod](#codersdkauthmethod) | false | | | ## codersdk.AuthorizationCheck diff --git a/docs/api/users.md b/docs/api/users.md index 44b3467363361..cc01acac6d366 100644 --- a/docs/api/users.md +++ b/docs/api/users.md @@ -143,6 +143,7 @@ curl -X GET http://coder-server:8080/api/v2/users/authmethods \ "github": { "enabled": true }, + "me_login_type": "password", "oidc": { "enabled": true, "iconUrl": "string", diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 9dd30ab80abdc..6a5df5ec92f31 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -104,6 +104,7 @@ export interface AuthMethod { // From codersdk/users.go export interface AuthMethods { + readonly me_login_type?: LoginType readonly password: AuthMethod readonly github: AuthMethod readonly oidc: OIDCAuthMethod diff --git a/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx b/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx index 5d7bb59bc11bf..72bee763c9398 100644 --- a/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx +++ b/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx @@ -10,6 +10,7 @@ import { useQuery } from "@tanstack/react-query" import { convertToOauth, getAuthMethods } from "api/api" import { AuthMethods } from "api/typesGenerated" import axios from "axios" +import { Maybe } from "components/Conditionals/Maybe" export const AccountPage: FC = () => { const queryKey = ["get-auth-methods"] @@ -60,36 +61,41 @@ export const AccountPage: FC = () => { }} /> - { - const mergeState = await convertToOauth( - credentials.email, - credentials.password, - "oidc", - ) + + { + const mergeState = await convertToOauth( + credentials.email, + credentials.password, + "oidc", + ) - window.location.href = `/api/v2/users/oidc/callback?oidc_merge_state=${ - mergeState?.state_string - }&redirect=${encodeURIComponent(redirectTo)}` - // await axios.get( - // `/api/v2/users/oidc/callback?oidc_merge_state=${ - // mergeState?.state_string - // }&redirect=${encodeURIComponent(redirectTo)}`, - // ) + window.location.href = `/api/v2/users/oidc/callback?oidc_merge_state=${ + mergeState?.state_string + }&redirect=${encodeURIComponent(redirectTo)}` + // await axios.get( + // `/api/v2/users/oidc/callback?oidc_merge_state=${ + // mergeState?.state_string + // }&redirect=${encodeURIComponent(redirectTo)}`, + // ) - { - /* */ - } - }} - > + } + }} + > + ) } From b082e49c37c19f58cb36b8ba4a0e1c992783a8a1 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 20 Jun 2023 21:49:38 +0000 Subject: [PATCH 12/58] Make gen --- docs/admin/audit-logs.md | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/docs/admin/audit-logs.md b/docs/admin/audit-logs.md index fccbf69baea9f..df6ba404582d2 100644 --- a/docs/admin/audit-logs.md +++ b/docs/admin/audit-logs.md @@ -9,19 +9,19 @@ We track the following resources: -| Resource | | -| -------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| APIKey
login, logout, register, create, delete |
FieldTracked
created_attrue
expires_attrue
hashed_secretfalse
idfalse
ip_addressfalse
last_usedtrue
lifetime_secondsfalse
login_typefalse
scopefalse
token_namefalse
updated_atfalse
user_idtrue
| -| Group
create, write, delete |
FieldTracked
avatar_urltrue
idtrue
memberstrue
nametrue
organization_idfalse
quota_allowancetrue
| -| GitSSHKey
create |
FieldTracked
created_atfalse
private_keytrue
public_keytrue
updated_atfalse
user_idtrue
| -| License
create, delete |
FieldTracked
exptrue
idfalse
jwtfalse
uploaded_attrue
uuidtrue
| -| OauthMergeState
|
FieldTracked
created_attrue
expires_attrue
state_stringtrue
to_login_typetrue
user_idtrue
| -| Template
write, delete |
FieldTracked
active_version_idtrue
allow_user_autostarttrue
allow_user_autostoptrue
allow_user_cancel_workspace_jobstrue
created_atfalse
created_bytrue
default_ttltrue
deletedfalse
descriptiontrue
display_nametrue
failure_ttltrue
group_acltrue
icontrue
idtrue
inactivity_ttltrue
max_ttltrue
nametrue
organization_idfalse
provisionertrue
updated_atfalse
user_acltrue
| -| TemplateVersion
create, write |
FieldTracked
created_atfalse
created_bytrue
git_auth_providersfalse
idtrue
job_idfalse
nametrue
organization_idfalse
readmetrue
template_idtrue
updated_atfalse
| -| User
create, write, delete |
FieldTracked
avatar_urlfalse
created_atfalse
deletedtrue
emailtrue
hashed_passwordtrue
idtrue
last_seen_atfalse
login_typefalse
rbac_rolestrue
statustrue
updated_atfalse
usernametrue
| -| Workspace
create, write, delete |
FieldTracked
autostart_scheduletrue
created_atfalse
deletedfalse
idtrue
last_used_atfalse
nametrue
organization_idfalse
owner_idtrue
template_idtrue
ttltrue
updated_atfalse
| -| WorkspaceBuild
start, stop |
FieldTracked
build_numberfalse
created_atfalse
daily_costfalse
deadlinefalse
idfalse
initiator_idfalse
job_idfalse
max_deadlinefalse
provisioner_statefalse
reasonfalse
template_version_idtrue
transitionfalse
updated_atfalse
workspace_idfalse
| -| WorkspaceProxy
|
FieldTracked
created_attrue
deletedfalse
display_nametrue
icontrue
idtrue
nametrue
token_hashed_secrettrue
updated_atfalse
urltrue
wildcard_hostnametrue
| +| Resource | | +| -------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| APIKey
login, logout, register, create, delete |
FieldTracked
created_attrue
expires_attrue
hashed_secretfalse
idfalse
ip_addressfalse
last_usedtrue
lifetime_secondsfalse
login_typefalse
scopefalse
token_namefalse
updated_atfalse
user_idtrue
| +| Group
create, write, delete |
FieldTracked
avatar_urltrue
idtrue
memberstrue
nametrue
organization_idfalse
quota_allowancetrue
| +| GitSSHKey
create |
FieldTracked
created_atfalse
private_keytrue
public_keytrue
updated_atfalse
user_idtrue
| +| License
create, delete |
FieldTracked
exptrue
idfalse
jwtfalse
uploaded_attrue
uuidtrue
| +| OauthMergeState
|
FieldTracked
created_attrue
expires_attrue
state_stringtrue
to_login_typetrue
user_idtrue
| +| Template
write, delete |
FieldTracked
active_version_idtrue
allow_user_autostarttrue
allow_user_autostoptrue
allow_user_cancel_workspace_jobstrue
created_atfalse
created_bytrue
default_ttltrue
deletedfalse
descriptiontrue
display_nametrue
failure_ttltrue
group_acltrue
icontrue
idtrue
inactivity_ttltrue
locked_ttltrue
max_ttltrue
nametrue
organization_idfalse
provisionertrue
updated_atfalse
user_acltrue
| +| TemplateVersion
create, write |
FieldTracked
created_atfalse
created_bytrue
git_auth_providersfalse
idtrue
job_idfalse
nametrue
organization_idfalse
readmetrue
template_idtrue
updated_atfalse
| +| User
create, write, delete |
FieldTracked
avatar_urlfalse
created_atfalse
deletedtrue
emailtrue
hashed_passwordtrue
idtrue
last_seen_atfalse
login_typefalse
rbac_rolestrue
statustrue
updated_atfalse
usernametrue
| +| Workspace
create, write, delete |
FieldTracked
autostart_scheduletrue
created_atfalse
deletedfalse
idtrue
last_used_atfalse
nametrue
organization_idfalse
owner_idtrue
template_idtrue
ttltrue
updated_atfalse
| +| WorkspaceBuild
start, stop |
FieldTracked
build_numberfalse
created_atfalse
daily_costfalse
deadlinefalse
idfalse
initiator_idfalse
job_idfalse
max_deadlinefalse
provisioner_statefalse
reasonfalse
template_version_idtrue
transitionfalse
updated_atfalse
workspace_idfalse
| +| WorkspaceProxy
|
FieldTracked
created_attrue
deletedfalse
display_nametrue
icontrue
idtrue
nametrue
token_hashed_secrettrue
updated_atfalse
urltrue
wildcard_hostnametrue
| From ecf37892d000cd15c1a9ee4357d84c755813844a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 20 Jun 2023 16:49:49 -0500 Subject: [PATCH 13/58] Linting --- coderd/userauth_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 4edde20b14644..86188cd14bea8 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -807,7 +807,6 @@ func TestUserOIDC(t *testing.T) { "email": userData.Email, }) - numLogs++ // add an audit log for login ctx := testutil.Context(t, testutil.WaitShort) convertResponse, err := user.ConvertToOAuthLogin(ctx, codersdk.ConvertLoginRequest{ ToLoginType: codersdk.LoginTypeOIDC, From 95ff11a913b46d65deb5d85db604769955c62943 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 20 Jun 2023 17:54:52 -0500 Subject: [PATCH 14/58] Remove test lint --- coderd/userauth_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 86188cd14bea8..5e4a2e271b7f5 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -801,8 +801,6 @@ func TestUserOIDC(t *testing.T) { user, userData := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) - numLogs := len(auditor.AuditLogs()) - code := conf.EncodeClaims(t, jwt.MapClaims{ "email": userData.Email, }) From 6e69f90257000b260041b427fb0328b7f3cb5d95 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 20 Jun 2023 18:01:15 -0500 Subject: [PATCH 15/58] Bump migration number --- ...e_oidc_account.down.sql => 000130_merge_oidc_account.down.sql} | 0 ...merge_oidc_account.up.sql => 000130_merge_oidc_account.up.sql} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename coderd/database/migrations/{000127_merge_oidc_account.down.sql => 000130_merge_oidc_account.down.sql} (100%) rename coderd/database/migrations/{000127_merge_oidc_account.up.sql => 000130_merge_oidc_account.up.sql} (100%) diff --git a/coderd/database/migrations/000127_merge_oidc_account.down.sql b/coderd/database/migrations/000130_merge_oidc_account.down.sql similarity index 100% rename from coderd/database/migrations/000127_merge_oidc_account.down.sql rename to coderd/database/migrations/000130_merge_oidc_account.down.sql diff --git a/coderd/database/migrations/000127_merge_oidc_account.up.sql b/coderd/database/migrations/000130_merge_oidc_account.up.sql similarity index 100% rename from coderd/database/migrations/000127_merge_oidc_account.up.sql rename to coderd/database/migrations/000130_merge_oidc_account.up.sql From 8c2fc33fb7457b029020a7db2178259f725cc919 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 20 Jun 2023 18:35:50 -0500 Subject: [PATCH 16/58] Swagger annotations --- coderd/userauth.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/coderd/userauth.go b/coderd/userauth.go index 527e7696bd167..2e0e4b1b1cf3c 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -34,6 +34,15 @@ import ( // postConvertLoginType replies with an oauth state token capable of converting // the user to an oauth user. +// +// @Summary Convert user from password to oauth authentication +// @ID convert-login-type +// @Accept json +// @Produce json +// @Tags Authorization +// @Param request body codersdk.ConvertLoginRequest true "Convert request" +// @Success 201 {object} codersdk.OauthConversionResponse +// @Router /users/convert-login [post] func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) { var ( ctx = r.Context() From cf19d8c6e7ecfd40138557b6aeb5411f6869d939 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 20 Jun 2023 20:47:07 -0500 Subject: [PATCH 17/58] Swagger annotations --- coderd/userauth.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 2e0e4b1b1cf3c..cf4a52b936166 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -36,7 +36,8 @@ import ( // the user to an oauth user. // // @Summary Convert user from password to oauth authentication -// @ID convert-login-type +// @ID convert-user-from-password-to-oauth-authentication +// @Security CoderSessionToken // @Accept json // @Produce json // @Tags Authorization From 694702118025e63017e44aa32c6693aa960b04c2 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 20 Jun 2023 23:09:54 -0500 Subject: [PATCH 18/58] golden file update --- cli/testdata/server-config.yaml.golden | 4 ++ coderd/apidoc/docs.go | 81 ++++++++++++++++++++++++++ coderd/apidoc/swagger.json | 71 ++++++++++++++++++++++ docs/api/authorization.md | 51 ++++++++++++++++ docs/api/schemas.md | 38 ++++++++++++ 5 files changed, 245 insertions(+) diff --git a/cli/testdata/server-config.yaml.golden b/cli/testdata/server-config.yaml.golden index 79a678e7abd2f..ac7bf533b0635 100644 --- a/cli/testdata/server-config.yaml.golden +++ b/cli/testdata/server-config.yaml.golden @@ -33,6 +33,10 @@ networking: # directly in the database. # (default: , type: bool) disablePasswordAuth: false + # If enabled, users can switch from password based authentication to oauth based + # authentication by logging into an oidc account with the same email address. + # (default: , type: bool) + enableOauthAuthConversion: false # The interval in which coderd should be checking the status of workspace proxies. # (default: 1m0s, type: duration) proxyHealthInterval: 1m0s diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 37a85f5f816cf..a1ffa4c963016 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -2902,6 +2902,45 @@ const docTemplate = `{ } } }, + "/users/convert-login": { + "post": { + "security": [ + { + "CoderSessionToken": [] + } + ], + "consumes": [ + "application/json" + ], + "produces": [ + "application/json" + ], + "tags": [ + "Authorization" + ], + "summary": "Convert user from password to oauth authentication", + "operationId": "convert-user-from-password-to-oauth-authentication", + "parameters": [ + { + "description": "Convert request", + "name": "request", + "in": "body", + "required": true, + "schema": { + "$ref": "#/definitions/codersdk.ConvertLoginRequest" + } + } + ], + "responses": { + "201": { + "description": "Created", + "schema": { + "$ref": "#/definitions/codersdk.OauthConversionResponse" + } + } + } + } + }, "/users/first": { "get": { "security": [ @@ -6594,6 +6633,31 @@ const docTemplate = `{ "BuildReasonAutostop" ] }, + "codersdk.ConvertLoginRequest": { + "type": "object", + "required": [ + "email", + "password", + "to_login_type" + ], + "properties": { + "email": { + "type": "string", + "format": "email" + }, + "password": { + "type": "string" + }, + "to_login_type": { + "description": "ToLoginType is the login type to convert to.", + "allOf": [ + { + "$ref": "#/definitions/codersdk.LoginType" + } + ] + } + } + }, "codersdk.CreateFirstUserRequest": { "type": "object", "required": [ @@ -7796,6 +7860,23 @@ const docTemplate = `{ } } }, + "codersdk.OauthConversionResponse": { + "type": "object", + "properties": { + "expires_at": { + "type": "string" + }, + "state_string": { + "type": "string" + }, + "to_login_type": { + "$ref": "#/definitions/codersdk.LoginType" + }, + "user_id": { + "type": "string" + } + } + }, "codersdk.Organization": { "type": "object", "required": [ diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 2fb73dc1d5c39..1f7aa2a53dd23 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -2556,6 +2556,39 @@ } } }, + "/users/convert-login": { + "post": { + "security": [ + { + "CoderSessionToken": [] + } + ], + "consumes": ["application/json"], + "produces": ["application/json"], + "tags": ["Authorization"], + "summary": "Convert user from password to oauth authentication", + "operationId": "convert-user-from-password-to-oauth-authentication", + "parameters": [ + { + "description": "Convert request", + "name": "request", + "in": "body", + "required": true, + "schema": { + "$ref": "#/definitions/codersdk.ConvertLoginRequest" + } + } + ], + "responses": { + "201": { + "description": "Created", + "schema": { + "$ref": "#/definitions/codersdk.OauthConversionResponse" + } + } + } + } + }, "/users/first": { "get": { "security": [ @@ -5874,6 +5907,27 @@ "BuildReasonAutostop" ] }, + "codersdk.ConvertLoginRequest": { + "type": "object", + "required": ["email", "password", "to_login_type"], + "properties": { + "email": { + "type": "string", + "format": "email" + }, + "password": { + "type": "string" + }, + "to_login_type": { + "description": "ToLoginType is the login type to convert to.", + "allOf": [ + { + "$ref": "#/definitions/codersdk.LoginType" + } + ] + } + } + }, "codersdk.CreateFirstUserRequest": { "type": "object", "required": ["email", "password", "username"], @@ -6987,6 +7041,23 @@ } } }, + "codersdk.OauthConversionResponse": { + "type": "object", + "properties": { + "expires_at": { + "type": "string" + }, + "state_string": { + "type": "string" + }, + "to_login_type": { + "$ref": "#/definitions/codersdk.LoginType" + }, + "user_id": { + "type": "string" + } + } + }, "codersdk.Organization": { "type": "object", "required": ["created_at", "id", "name", "updated_at"], diff --git a/docs/api/authorization.md b/docs/api/authorization.md index a75a477656224..094820344c6fa 100644 --- a/docs/api/authorization.md +++ b/docs/api/authorization.md @@ -66,6 +66,57 @@ curl -X POST http://coder-server:8080/api/v2/authcheck \ To perform this operation, you must be authenticated. [Learn more](authentication.md). +## Convert user from password to oauth authentication + +### Code samples + +```shell +# Example request using curl +curl -X POST http://coder-server:8080/api/v2/users/convert-login \ + -H 'Content-Type: application/json' \ + -H 'Accept: application/json' \ + -H 'Coder-Session-Token: API_KEY' +``` + +`POST /users/convert-login` + +> Body parameter + +```json +{ + "email": "user@example.com", + "password": "string", + "to_login_type": "password" +} +``` + +### Parameters + +| Name | In | Type | Required | Description | +| ------ | ---- | ---------------------------------------------------------------------- | -------- | --------------- | +| `body` | body | [codersdk.ConvertLoginRequest](schemas.md#codersdkconvertloginrequest) | true | Convert request | + +### Example responses + +> 201 Response + +```json +{ + "expires_at": "string", + "state_string": "string", + "to_login_type": "password", + "user_id": "string" +} +``` + +### Responses + +| Status | Meaning | Description | Schema | +| ------ | ------------------------------------------------------------ | ----------- | ------------------------------------------------------------------------------ | +| 201 | [Created](https://tools.ietf.org/html/rfc7231#section-6.3.2) | Created | [codersdk.OauthConversionResponse](schemas.md#codersdkoauthconversionresponse) | + +To perform this operation, you must be authenticated. [Learn more](authentication.md). + ## Log in user ### Code samples diff --git a/docs/api/schemas.md b/docs/api/schemas.md index 7dc0cc43c4fec..2757d044e9d00 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -1270,6 +1270,24 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in | `autostart` | | `autostop` | +## codersdk.ConvertLoginRequest + +```json +{ + "email": "user@example.com", + "password": "string", + "to_login_type": "password" +} +``` + +### Properties + +| Name | Type | Required | Restrictions | Description | +| --------------- | ---------------------------------------- | -------- | ------------ | ---------------------------------------------- | +| `email` | string | true | | | +| `password` | string | true | | | +| `to_login_type` | [codersdk.LoginType](#codersdklogintype) | true | | To login type is the login type to convert to. | + ## codersdk.CreateFirstUserRequest ```json @@ -2992,6 +3010,26 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in | `sign_in_text` | string | false | | | | `username_field` | string | false | | | +## codersdk.OauthConversionResponse + +```json +{ + "expires_at": "string", + "state_string": "string", + "to_login_type": "password", + "user_id": "string" +} +``` + +### Properties + +| Name | Type | Required | Restrictions | Description | +| --------------- | ---------------------------------------- | -------- | ------------ | ----------- | +| `expires_at` | string | false | | | +| `state_string` | string | false | | | +| `to_login_type` | [codersdk.LoginType](#codersdklogintype) | false | | | +| `user_id` | string | false | | | + ## codersdk.Organization ```json From 27bf5189fbbf54e5e5c01bdba38ac77929a9bfd7 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 20 Jun 2023 23:26:53 -0500 Subject: [PATCH 19/58] Add format tags --- coderd/userauth.go | 4 +--- codersdk/users.go | 4 ++-- site/src/api/api.ts | 12 +++++++----- site/src/testHelpers/entities.ts | 29 ++++++++++++++--------------- 4 files changed, 24 insertions(+), 25 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index cf4a52b936166..44a8a9190f288 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -372,9 +372,7 @@ type GithubOAuth2Config struct { // @Success 200 {object} codersdk.AuthMethods // @Router /users/authmethods [get] func (api *API) userAuthMethods(rw http.ResponseWriter, r *http.Request) { - var ( - key, ok = httpmw.APIKeyOptional(r) - ) + key, ok := httpmw.APIKeyOptional(r) var currentUserLoginType codersdk.LoginType if ok { user, err := api.Database.GetUserByID(r.Context(), key.UserID) diff --git a/codersdk/users.go b/codersdk/users.go index 0bfb2e424c5c9..18ff02d607e95 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -112,9 +112,9 @@ type LoginWithPasswordResponse struct { type OauthConversionResponse struct { StateString string `json:"state_string"` - ExpiresAt time.Time `json:"expires_at"` + ExpiresAt time.Time `json:"expires_at" format:"date-time"` ToLoginType LoginType `json:"to_login_type"` - UserID uuid.UUID `json:"user_id"` + UserID uuid.UUID `json:"user_id" format:"uuid"` } type CreateOrganizationRequest struct { diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 342ed2931bb93..44efa2fefd3d3 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -120,11 +120,13 @@ export const convertToOauth = async ( }) try { - const response = await axios.post("/api/v2/users/convert-login", - payload, - { - headers: { ...CONTENT_TYPE_JSON }, - }) + const response = await axios.post( + "/api/v2/users/convert-login", + payload, + { + headers: { ...CONTENT_TYPE_JSON }, + }, + ) return response.data } catch (error) { if (axios.isAxiosError(error) && error.response?.status === 401) { diff --git a/site/src/testHelpers/entities.ts b/site/src/testHelpers/entities.ts index 1fdf45eb02a02..d370a57ddf5b4 100644 --- a/site/src/testHelpers/entities.ts +++ b/site/src/testHelpers/entities.ts @@ -1517,32 +1517,31 @@ export const MockAuditOauthConvert: TypesGen.AuditLog = { created_at: { old: "0001-01-01T00:00:00Z", new: "2023-06-20T20:44:54.243019Z", - secret: false + secret: false, }, expires_at: { - old: "0001-01-01T00:00:00Z", - new: "2023-06-20T20:49:54.243019Z", - secret: false + old: "0001-01-01T00:00:00Z", + new: "2023-06-20T20:49:54.243019Z", + secret: false, }, state_string: { - old: "", - new: "", - secret: true + old: "", + new: "", + secret: true, }, to_login_type: { - old: "", - new: "oidc", - secret: false + old: "", + new: "oidc", + secret: false, }, user_id: { - old: "", - new: "dc790496-eaec-4f88-a53f-8ce1f61a1fff", - secret: false - } + old: "", + new: "dc790496-eaec-4f88-a53f-8ce1f61a1fff", + secret: false, + }, }, } - export const MockAuditLogSuccessfulLogin: TypesGen.AuditLog = { ...MockAuditLog, resource_type: "api_key", From 3c8ee52a7849b44c7370c6e4cf90b2991b612799 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 20 Jun 2023 23:46:16 -0500 Subject: [PATCH 20/58] Fix audit log mistake --- coderd/userauth.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/userauth.go b/coderd/userauth.go index 44a8a9190f288..f678fb2fbe675 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -179,6 +179,7 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { // user failed to login return } + aReq.UserID = user.ID userSubj := rbac.Subject{ ID: user.ID.String(), From bc506c1af12db5d5f0cfb8f493947e93b3c04777 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 21 Jun 2023 08:43:38 -0500 Subject: [PATCH 21/58] Make gen --- coderd/apidoc/docs.go | 6 ++++-- coderd/apidoc/swagger.json | 6 ++++-- coderd/database/dbmock/dbmock.go | 28 ++++++++++++++-------------- docs/api/authorization.md | 4 ++-- docs/api/schemas.md | 4 ++-- 5 files changed, 26 insertions(+), 22 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index a28e2fa36635a..d156cfa473bdb 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -7862,7 +7862,8 @@ const docTemplate = `{ "type": "object", "properties": { "expires_at": { - "type": "string" + "type": "string", + "format": "date-time" }, "state_string": { "type": "string" @@ -7871,7 +7872,8 @@ const docTemplate = `{ "$ref": "#/definitions/codersdk.LoginType" }, "user_id": { - "type": "string" + "type": "string", + "format": "uuid" } } }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 554b971ce964f..4f9b677871744 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -7041,7 +7041,8 @@ "type": "object", "properties": { "expires_at": { - "type": "string" + "type": "string", + "format": "date-time" }, "state_string": { "type": "string" @@ -7050,7 +7051,8 @@ "$ref": "#/definitions/codersdk.LoginType" }, "user_id": { - "type": "string" + "type": "string", + "format": "uuid" } } }, diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 71a6f51e99c1f..0d43d5e1631ca 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -237,20 +237,6 @@ func (mr *MockStoreMockRecorder) DeleteReplicasUpdatedBefore(arg0, arg1 interfac return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteReplicasUpdatedBefore", reflect.TypeOf((*MockStore)(nil).DeleteReplicasUpdatedBefore), arg0, arg1) } -// DeleteUserOauthMergeStates mocks base method. -func (m *MockStore) DeleteUserOauthMergeStates(arg0 context.Context, arg1 uuid.UUID) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteUserOauthMergeStates", arg0, arg1) - ret0, _ := ret[0].(error) - return ret0 -} - -// DeleteUserOauthMergeStates indicates an expected call of DeleteUserOauthMergeStates. -func (mr *MockStoreMockRecorder) DeleteUserOauthMergeStates(arg0, arg1 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteUserOauthMergeStates", reflect.TypeOf((*MockStore)(nil).DeleteUserOauthMergeStates), arg0, arg1) -} - // DeleteTailnetAgent mocks base method. func (m *MockStore) DeleteTailnetAgent(arg0 context.Context, arg1 database.DeleteTailnetAgentParams) (database.DeleteTailnetAgentRow, error) { m.ctrl.T.Helper() @@ -281,6 +267,20 @@ func (mr *MockStoreMockRecorder) DeleteTailnetClient(arg0, arg1 interface{}) *go return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteTailnetClient", reflect.TypeOf((*MockStore)(nil).DeleteTailnetClient), arg0, arg1) } +// DeleteUserOauthMergeStates mocks base method. +func (m *MockStore) DeleteUserOauthMergeStates(arg0 context.Context, arg1 uuid.UUID) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeleteUserOauthMergeStates", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// DeleteUserOauthMergeStates indicates an expected call of DeleteUserOauthMergeStates. +func (mr *MockStoreMockRecorder) DeleteUserOauthMergeStates(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteUserOauthMergeStates", reflect.TypeOf((*MockStore)(nil).DeleteUserOauthMergeStates), arg0, arg1) +} + // GetAPIKeyByID mocks base method. func (m *MockStore) GetAPIKeyByID(arg0 context.Context, arg1 string) (database.APIKey, error) { m.ctrl.T.Helper() diff --git a/docs/api/authorization.md b/docs/api/authorization.md index 094820344c6fa..582356e1af0cf 100644 --- a/docs/api/authorization.md +++ b/docs/api/authorization.md @@ -102,10 +102,10 @@ curl -X POST http://coder-server:8080/api/v2/users/convert-login \ ```json { - "expires_at": "string", + "expires_at": "2019-08-24T14:15:22Z", "state_string": "string", "to_login_type": "password", - "user_id": "string" + "user_id": "a169451c-8525-4352-b8ca-070dd449a1a5" } ``` diff --git a/docs/api/schemas.md b/docs/api/schemas.md index 1d9c92def0ef9..2a140faafa5e3 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -3013,10 +3013,10 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in ```json { - "expires_at": "string", + "expires_at": "2019-08-24T14:15:22Z", "state_string": "string", "to_login_type": "password", - "user_id": "string" + "user_id": "a169451c-8525-4352-b8ca-070dd449a1a5" } ``` From 4ea859c0dc976249bd1eaf5d5baa19ad22a40ded Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 21 Jun 2023 08:47:47 -0500 Subject: [PATCH 22/58] Fix audit log on login failure --- coderd/userauth.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index bf97920af7914..86d9ddfc66811 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -175,11 +175,12 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { } user, roles, ok := api.loginRequest(ctx, rw, loginWithPassword) + // 'user.ID' will be empty, or will be an actual value. + aReq.UserID = user.ID if !ok { // user failed to login return } - aReq.UserID = user.ID userSubj := rbac.Subject{ ID: user.ID.String(), @@ -224,7 +225,7 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error.", }) - return database.User{}, database.GetAuthorizationUserRolesRow{}, false + return user, database.GetAuthorizationUserRolesRow{}, false } // If the user doesn't exist, it will be a default struct. @@ -233,7 +234,7 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error.", }) - return database.User{}, database.GetAuthorizationUserRolesRow{}, false + return user, database.GetAuthorizationUserRolesRow{}, false } if !equal { @@ -242,7 +243,7 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{ Message: "Incorrect email or password.", }) - return database.User{}, database.GetAuthorizationUserRolesRow{}, false + return user, database.GetAuthorizationUserRolesRow{}, false } // If password authentication is disabled and the user does not have the @@ -251,14 +252,14 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ Message: "Password authentication is disabled.", }) - return database.User{}, database.GetAuthorizationUserRolesRow{}, false + return user, database.GetAuthorizationUserRolesRow{}, false } if user.LoginType != database.LoginTypePassword { httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ Message: fmt.Sprintf("Incorrect login type, attempting to use %q but user is of login type %q", database.LoginTypePassword, user.LoginType), }) - return database.User{}, database.GetAuthorizationUserRolesRow{}, false + return user, database.GetAuthorizationUserRolesRow{}, false } //nolint:gocritic // System needs to fetch user roles in order to login user. @@ -267,7 +268,7 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error.", }) - return database.User{}, database.GetAuthorizationUserRolesRow{}, false + return user, database.GetAuthorizationUserRolesRow{}, false } // If the user logged into a suspended account, reject the login request. @@ -275,7 +276,7 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{ Message: "Your account is suspended. Contact an admin to reactivate your account.", }) - return database.User{}, database.GetAuthorizationUserRolesRow{}, false + return user, database.GetAuthorizationUserRolesRow{}, false } return user, roles, true From 847dc643bdea2065c99a68fbd8c89f72dc27fcfd Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 21 Jun 2023 08:48:51 -0500 Subject: [PATCH 23/58] fixup! Fix audit log on login failure --- coderd/userauth.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/coderd/userauth.go b/coderd/userauth.go index 86d9ddfc66811..42fd528ccb04f 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -216,6 +216,9 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { // loginRequest will process a LoginWithPasswordRequest and return the user if // the credentials are correct. If 'false' is returned, the authentication failed // and the appropriate error will be written to the ResponseWriter. +// +// The user struct is always returned, even if authentication failed. This is +// to support knowing what user attempted to login. func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req codersdk.LoginWithPasswordRequest) (database.User, database.GetAuthorizationUserRolesRow, bool) { //nolint:gocritic // In order to login, we need to get the user first! user, err := api.Database.GetUserByEmailOrUsername(dbauthz.AsSystemRestricted(ctx), database.GetUserByEmailOrUsernameParams{ From 59e89245b4c9103084adf3a7dba51c58c103e51c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 21 Jun 2023 08:54:44 -0500 Subject: [PATCH 24/58] Bump migration --- coderd/database/dbauthz/dbauthz.go | 1 - ..._oidc_account.down.sql => 000131_merge_oidc_account.down.sql} | 0 ...erge_oidc_account.up.sql => 000131_merge_oidc_account.up.sql} | 0 3 files changed, 1 deletion(-) rename coderd/database/migrations/{000130_merge_oidc_account.down.sql => 000131_merge_oidc_account.down.sql} (100%) rename coderd/database/migrations/{000130_merge_oidc_account.up.sql => 000131_merge_oidc_account.up.sql} (100%) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 9a29e3e1d6100..df62080a2d7e5 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -779,7 +779,6 @@ func (q *querier) DeleteUserOauthMergeStates(ctx context.Context, userID uuid.UU return q.db.DeleteUserOauthMergeStates(ctx, userID) } - func (q *querier) DeleteTailnetAgent(ctx context.Context, arg database.DeleteTailnetAgentParams) (database.DeleteTailnetAgentRow, error) { if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceTailnetCoordinator); err != nil { return database.DeleteTailnetAgentRow{}, err diff --git a/coderd/database/migrations/000130_merge_oidc_account.down.sql b/coderd/database/migrations/000131_merge_oidc_account.down.sql similarity index 100% rename from coderd/database/migrations/000130_merge_oidc_account.down.sql rename to coderd/database/migrations/000131_merge_oidc_account.down.sql diff --git a/coderd/database/migrations/000130_merge_oidc_account.up.sql b/coderd/database/migrations/000131_merge_oidc_account.up.sql similarity index 100% rename from coderd/database/migrations/000130_merge_oidc_account.up.sql rename to coderd/database/migrations/000131_merge_oidc_account.up.sql From 368fa2be926adb67c1ffbd169f6660cf9733419f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 21 Jun 2023 09:12:38 -0500 Subject: [PATCH 25/58] Make gen and add test fixtures Remove TS changes --- coderd/database/dbauthz/dbauthz.go | 14 ++-- coderd/database/dbfake/dbfake.go | 16 ++--- coderd/database/dbmetrics/dbmetrics.go | 14 ++-- .../fixtures/000131_oauth_merge_state.up.sql | 13 ++++ coderd/database/querier.go | 2 +- .../AuditLogDescription.test.tsx | 20 ------ site/src/components/SignInForm/SignInForm.tsx | 2 +- .../AccountPage/AccountPage.tsx | 67 ------------------- 8 files changed, 37 insertions(+), 111 deletions(-) create mode 100644 coderd/database/migrations/testdata/fixtures/000131_oauth_merge_state.up.sql diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index df62080a2d7e5..6761488f82026 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -772,13 +772,6 @@ func (q *querier) DeleteReplicasUpdatedBefore(ctx context.Context, updatedAt tim return q.db.DeleteReplicasUpdatedBefore(ctx, updatedAt) } -func (q *querier) DeleteUserOauthMergeStates(ctx context.Context, userID uuid.UUID) error { - if err := q.authorizeContext(ctx, rbac.ActionDelete, rbac.ResourceSystem); err != nil { - return err - } - return q.db.DeleteUserOauthMergeStates(ctx, userID) -} - func (q *querier) DeleteTailnetAgent(ctx context.Context, arg database.DeleteTailnetAgentParams) (database.DeleteTailnetAgentRow, error) { if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceTailnetCoordinator); err != nil { return database.DeleteTailnetAgentRow{}, err @@ -793,6 +786,13 @@ func (q *querier) DeleteTailnetClient(ctx context.Context, arg database.DeleteTa return q.db.DeleteTailnetClient(ctx, arg) } +func (q *querier) DeleteUserOauthMergeStates(ctx context.Context, userID uuid.UUID) error { + if err := q.authorizeContext(ctx, rbac.ActionDelete, rbac.ResourceSystem); err != nil { + return err + } + return q.db.DeleteUserOauthMergeStates(ctx, userID) +} + func (q *querier) GetAPIKeyByID(ctx context.Context, id string) (database.APIKey, error) { return fetch(q.log, q.auth, q.db.GetAPIKeyByID)(ctx, id) } diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 8b43d6fce1462..a2211e0d3cafe 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -1187,6 +1187,14 @@ func (q *fakeQuerier) DeleteReplicasUpdatedBefore(_ context.Context, before time return nil } +func (*fakeQuerier) DeleteTailnetAgent(context.Context, database.DeleteTailnetAgentParams) (database.DeleteTailnetAgentRow, error) { + return database.DeleteTailnetAgentRow{}, ErrUnimplemented +} + +func (*fakeQuerier) DeleteTailnetClient(context.Context, database.DeleteTailnetClientParams) (database.DeleteTailnetClientRow, error) { + return database.DeleteTailnetClientRow{}, ErrUnimplemented +} + func (q *fakeQuerier) DeleteUserOauthMergeStates(_ context.Context, userID uuid.UUID) error { q.mutex.Lock() defer q.mutex.Unlock() @@ -1208,14 +1216,6 @@ func (q *fakeQuerier) DeleteUserOauthMergeStates(_ context.Context, userID uuid. return nil } -func (*fakeQuerier) DeleteTailnetAgent(context.Context, database.DeleteTailnetAgentParams) (database.DeleteTailnetAgentRow, error) { - return database.DeleteTailnetAgentRow{}, ErrUnimplemented -} - -func (*fakeQuerier) DeleteTailnetClient(context.Context, database.DeleteTailnetClientParams) (database.DeleteTailnetClientRow, error) { - return database.DeleteTailnetClientRow{}, ErrUnimplemented -} - func (q *fakeQuerier) GetAPIKeyByID(_ context.Context, id string) (database.APIKey, error) { q.mutex.RLock() defer q.mutex.RUnlock() diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index 6883a2d42638b..175eb07576a58 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -205,13 +205,6 @@ func (m metricsStore) DeleteReplicasUpdatedBefore(ctx context.Context, updatedAt return err } -func (m metricsStore) DeleteUserOauthMergeStates(ctx context.Context, userID uuid.UUID) error { - start := time.Now() - r0 := m.s.DeleteUserOauthMergeStates(ctx, userID) - m.queryLatencies.WithLabelValues("DeleteUserOauthMergeStates").Observe(time.Since(start).Seconds()) - return r0 -} - func (m metricsStore) DeleteTailnetAgent(ctx context.Context, arg database.DeleteTailnetAgentParams) (database.DeleteTailnetAgentRow, error) { start := time.Now() defer m.queryLatencies.WithLabelValues("DeleteTailnetAgent").Observe(time.Since(start).Seconds()) @@ -224,6 +217,13 @@ func (m metricsStore) DeleteTailnetClient(ctx context.Context, arg database.Dele return m.s.DeleteTailnetClient(ctx, arg) } +func (m metricsStore) DeleteUserOauthMergeStates(ctx context.Context, userID uuid.UUID) error { + start := time.Now() + r0 := m.s.DeleteUserOauthMergeStates(ctx, userID) + m.queryLatencies.WithLabelValues("DeleteUserOauthMergeStates").Observe(time.Since(start).Seconds()) + return r0 +} + func (m metricsStore) GetAPIKeyByID(ctx context.Context, id string) (database.APIKey, error) { start := time.Now() apiKey, err := m.s.GetAPIKeyByID(ctx, id) diff --git a/coderd/database/migrations/testdata/fixtures/000131_oauth_merge_state.up.sql b/coderd/database/migrations/testdata/fixtures/000131_oauth_merge_state.up.sql new file mode 100644 index 0000000000000..8b84d45753695 --- /dev/null +++ b/coderd/database/migrations/testdata/fixtures/000131_oauth_merge_state.up.sql @@ -0,0 +1,13 @@ +INSERT INTO oauth_merge_state( + state_string, + created_at, + expires_at, + to_login_type, + user_id +) VALUES ( + gen_random_uuid()::text, + now(), + now() + interval '24 hour', + 'oidc', + (SELECT id FROM users LIMIT 1) +) diff --git a/coderd/database/querier.go b/coderd/database/querier.go index d1581c74a7ec5..3621f6310f2a3 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -40,9 +40,9 @@ type sqlcQuerier interface { DeleteOldWorkspaceAgentStartupLogs(ctx context.Context) error DeleteOldWorkspaceAgentStats(ctx context.Context) error DeleteReplicasUpdatedBefore(ctx context.Context, updatedAt time.Time) error - DeleteUserOauthMergeStates(ctx context.Context, userID uuid.UUID) error DeleteTailnetAgent(ctx context.Context, arg DeleteTailnetAgentParams) (DeleteTailnetAgentRow, error) DeleteTailnetClient(ctx context.Context, arg DeleteTailnetClientParams) (DeleteTailnetClientRow, error) + DeleteUserOauthMergeStates(ctx context.Context, userID uuid.UUID) error GetAPIKeyByID(ctx context.Context, id string) (APIKey, error) // there is no unique constraint on empty token names GetAPIKeyByName(ctx context.Context, arg GetAPIKeyByNameParams) (APIKey, error) diff --git a/site/src/components/AuditLogRow/AuditLogDescription/AuditLogDescription.test.tsx b/site/src/components/AuditLogRow/AuditLogDescription/AuditLogDescription.test.tsx index 1c47fce2e23e2..be9be76b5ac27 100644 --- a/site/src/components/AuditLogRow/AuditLogDescription/AuditLogDescription.test.tsx +++ b/site/src/components/AuditLogRow/AuditLogDescription/AuditLogDescription.test.tsx @@ -4,7 +4,6 @@ import { MockWorkspaceCreateAuditLogForDifferentOwner, MockAuditLogSuccessfulLogin, MockAuditLogUnsuccessfulLoginKnownUser, - MockAuditOauthConvert, } from "testHelpers/entities" import { AuditLogDescription } from "./AuditLogDescription" import { AuditLogRow } from "../AuditLogRow" @@ -85,25 +84,6 @@ describe("AuditLogDescription", () => { const statusPill = screen.getByRole("status") expect(statusPill).toHaveTextContent("201") }) - it("renders the correct string for login type conversion", async () => { - render() - - expect( - screen.getByText( - t("auditLog:table.logRow.description.unlinkedAuditDescription", { - truncatedDescription: `${MockAuditOauthConvert.user?.username} created login type conversion to ${MockAuditOauthConvert.resource_target}`, - target: "", - onBehalfOf: undefined, - }) - .replace(/<[^>]*>/g, " ") - .replace(/\s{2,}/g, " ") - .trim(), - ), - ).toBeInTheDocument() - - const statusPill = screen.getByRole("status") - expect(statusPill).toHaveTextContent("201") - }) it("renders the correct string for unsuccessful login for a known user", async () => { render() diff --git a/site/src/components/SignInForm/SignInForm.tsx b/site/src/components/SignInForm/SignInForm.tsx index 0ebb930ae99ac..4e5d552fc4371 100644 --- a/site/src/components/SignInForm/SignInForm.tsx +++ b/site/src/components/SignInForm/SignInForm.tsx @@ -99,7 +99,7 @@ export const SignInForm: FC> = ({ {loginPageTranslation.t("signInTo")}{" "} {commonTranslation.t("coder")} - +
diff --git a/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx b/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx index 72bee763c9398..bc2cdc793fb2a 100644 --- a/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx +++ b/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx @@ -4,44 +4,13 @@ import { AccountForm } from "../../../components/SettingsAccountForm/SettingsAcc import { useAuth } from "components/AuthProvider/AuthProvider" import { useMe } from "hooks/useMe" import { usePermissions } from "hooks/usePermissions" -import { SignInForm } from "components/SignInForm/SignInForm" -import { retrieveRedirect } from "utils/redirect" -import { useQuery } from "@tanstack/react-query" -import { convertToOauth, getAuthMethods } from "api/api" -import { AuthMethods } from "api/typesGenerated" -import axios from "axios" -import { Maybe } from "components/Conditionals/Maybe" export const AccountPage: FC = () => { - const queryKey = ["get-auth-methods"] - const { - data: authMethodsData, - error: authMethodsError, - isLoading: authMethodsLoading, - isFetched: authMethodsFetched, - } = useQuery({ - // select: (res: AuthMethods) => { - // return { - // ...res, - // // Disable the password auth in this account section. For merging accounts, - // // we only want to support oidc. - // password: { - // enabled: false, - // }, - // } - // }, - queryKey, - queryFn: getAuthMethods, - }) - const [authState, authSend] = useAuth() const me = useMe() const permissions = usePermissions() const { updateProfileError } = authState.context const canEditUsers = permissions && permissions.updateUsers - // Extra - const redirectTo = retrieveRedirect(location.search) - console.log(authState.context.data, authMethodsError) return (
@@ -60,42 +29,6 @@ export const AccountPage: FC = () => { }) }} /> - - - { - const mergeState = await convertToOauth( - credentials.email, - credentials.password, - "oidc", - ) - - window.location.href = `/api/v2/users/oidc/callback?oidc_merge_state=${ - mergeState?.state_string - }&redirect=${encodeURIComponent(redirectTo)}` - // await axios.get( - // `/api/v2/users/oidc/callback?oidc_merge_state=${ - // mergeState?.state_string - // }&redirect=${encodeURIComponent(redirectTo)}`, - // ) - - { - /* */ - } - }} - > -
) } From 9cfb69cf99deb05d434856d60f193500cdba11e0 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 21 Jun 2023 10:14:51 -0500 Subject: [PATCH 26/58] Add from_login_type --- coderd/coderd.go | 3 ++ coderd/database/dbfake/dbfake.go | 11 ++++---- coderd/database/dump.sql | 1 + .../000131_merge_oidc_account.up.sql | 1 + .../fixtures/000131_oauth_merge_state.up.sql | 2 ++ coderd/database/models.go | 3 +- coderd/database/queries.sql.go | 19 ++++++++----- coderd/database/queries/users.sql | 3 +- coderd/httpmw/oauth2.go | 9 ------ coderd/userauth.go | 28 +++++++++++++------ docs/admin/audit-logs.md | 2 +- enterprise/audit/table.go | 11 ++++---- 12 files changed, 55 insertions(+), 38 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index f0b63f7607a8b..6546fa9f33ea2 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -595,6 +595,9 @@ func New(options *Options) *API { r.Get("/first", api.firstUser) r.Post("/first", api.postFirstUser) r.Route("/authmethods", func(r chi.Router) { + // The API Key allows this method to return the auth method + // for the logged-in user. This information is useful for the + // caller. If not authenticated, this information is omitted. r.Use(apiKeyMiddlewareOptional) r.Get("/", api.userAuthMethods) }) diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index a2211e0d3cafe..954e48e3a6e75 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -4044,11 +4044,12 @@ func (q *fakeQuerier) InsertUserOauthMergeState(_ context.Context, arg database. } s := database.OauthMergeState{ - StateString: arg.StateString, - CreatedAt: arg.CreatedAt, - ExpiresAt: arg.ExpiresAt, - ToLoginType: arg.ToLoginType, - UserID: arg.UserID, + StateString: arg.StateString, + CreatedAt: arg.CreatedAt, + ExpiresAt: arg.ExpiresAt, + FromLoginType: arg.FromLoginType, + ToLoginType: arg.ToLoginType, + UserID: arg.UserID, } q.oauthMergeStates = append(q.oauthMergeStates, s) return s, nil diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index b45a253bf8ab1..b31022efffd39 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -310,6 +310,7 @@ CREATE TABLE oauth_merge_state ( state_string text NOT NULL, created_at timestamp with time zone NOT NULL, expires_at timestamp with time zone NOT NULL, + from_login_type login_type NOT NULL, to_login_type login_type NOT NULL, user_id uuid NOT NULL ); diff --git a/coderd/database/migrations/000131_merge_oidc_account.up.sql b/coderd/database/migrations/000131_merge_oidc_account.up.sql index 4c6f0e9ea2234..af774ff1f301f 100644 --- a/coderd/database/migrations/000131_merge_oidc_account.up.sql +++ b/coderd/database/migrations/000131_merge_oidc_account.up.sql @@ -4,6 +4,7 @@ CREATE TABLE IF NOT EXISTS oauth_merge_state ( state_string text NOT NULL, created_at timestamptz NOT NULL, expires_at timestamptz NOT NULL, + from_login_type login_type NOT NULL, to_login_type login_type NOT NULL, user_id uuid NOT NULL REFERENCES users (id) ON DELETE CASCADE, diff --git a/coderd/database/migrations/testdata/fixtures/000131_oauth_merge_state.up.sql b/coderd/database/migrations/testdata/fixtures/000131_oauth_merge_state.up.sql index 8b84d45753695..5ac3ff510875b 100644 --- a/coderd/database/migrations/testdata/fixtures/000131_oauth_merge_state.up.sql +++ b/coderd/database/migrations/testdata/fixtures/000131_oauth_merge_state.up.sql @@ -2,12 +2,14 @@ INSERT INTO oauth_merge_state( state_string, created_at, expires_at, + from_login_type, to_login_type, user_id ) VALUES ( gen_random_uuid()::text, now(), now() + interval '24 hour', + 'password', 'oidc', (SELECT id FROM users LIMIT 1) ) diff --git a/coderd/database/models.go b/coderd/database/models.go index 08e421c881cfd..decf36e026349 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -1434,7 +1434,8 @@ type OauthMergeState struct { StateString string `db:"state_string" json:"state_string"` CreatedAt time.Time `db:"created_at" json:"created_at"` // The time at which the state string expires, a merge request times out if the user does not perform it quick enough. - ExpiresAt time.Time `db:"expires_at" json:"expires_at"` + ExpiresAt time.Time `db:"expires_at" json:"expires_at"` + FromLoginType LoginType `db:"from_login_type" json:"from_login_type"` // The login type the user is converting to. Should be github or oidc. ToLoginType LoginType `db:"to_login_type" json:"to_login_type"` UserID uuid.UUID `db:"user_id" json:"user_id"` diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index a129b77ac69df..b6f2d554c26fd 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5150,7 +5150,7 @@ func (q *sqlQuerier) GetUserCount(ctx context.Context) (int64, error) { const getUserOauthMergeState = `-- name: GetUserOauthMergeState :one SELECT - state_string, created_at, expires_at, to_login_type, user_id + state_string, created_at, expires_at, from_login_type, to_login_type, user_id FROM oauth_merge_state WHERE @@ -5170,6 +5170,7 @@ func (q *sqlQuerier) GetUserOauthMergeState(ctx context.Context, arg GetUserOaut &i.StateString, &i.CreatedAt, &i.ExpiresAt, + &i.FromLoginType, &i.ToLoginType, &i.UserID, ) @@ -5410,26 +5411,29 @@ INSERT INTO oauth_merge_state ( user_id, state_string, + from_login_type, to_login_type, created_at, expires_at ) VALUES - ($1, $2, $3, $4, $5) RETURNING state_string, created_at, expires_at, to_login_type, user_id + ($1, $2, $3, $4, $5, $6) RETURNING state_string, created_at, expires_at, from_login_type, to_login_type, user_id ` type InsertUserOauthMergeStateParams struct { - UserID uuid.UUID `db:"user_id" json:"user_id"` - StateString string `db:"state_string" json:"state_string"` - ToLoginType LoginType `db:"to_login_type" json:"to_login_type"` - CreatedAt time.Time `db:"created_at" json:"created_at"` - ExpiresAt time.Time `db:"expires_at" json:"expires_at"` + UserID uuid.UUID `db:"user_id" json:"user_id"` + StateString string `db:"state_string" json:"state_string"` + FromLoginType LoginType `db:"from_login_type" json:"from_login_type"` + ToLoginType LoginType `db:"to_login_type" json:"to_login_type"` + CreatedAt time.Time `db:"created_at" json:"created_at"` + ExpiresAt time.Time `db:"expires_at" json:"expires_at"` } func (q *sqlQuerier) InsertUserOauthMergeState(ctx context.Context, arg InsertUserOauthMergeStateParams) (OauthMergeState, error) { row := q.db.QueryRowContext(ctx, insertUserOauthMergeState, arg.UserID, arg.StateString, + arg.FromLoginType, arg.ToLoginType, arg.CreatedAt, arg.ExpiresAt, @@ -5439,6 +5443,7 @@ func (q *sqlQuerier) InsertUserOauthMergeState(ctx context.Context, arg InsertUs &i.StateString, &i.CreatedAt, &i.ExpiresAt, + &i.FromLoginType, &i.ToLoginType, &i.UserID, ) diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index 5897c5767e7eb..6d0ee0f318855 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -12,12 +12,13 @@ INSERT INTO oauth_merge_state ( user_id, state_string, + from_login_type, to_login_type, created_at, expires_at ) VALUES - ($1, $2, $3, $4, $5) RETURNING *; + ($1, $2, $3, $4, $5, $6) RETURNING *; -- name: DeleteUserOauthMergeStates :exec DELETE FROM oauth_merge_state WHERE user_id = @user_id; diff --git a/coderd/httpmw/oauth2.go b/coderd/httpmw/oauth2.go index 1012b6f5327e6..57521b7f103af 100644 --- a/coderd/httpmw/oauth2.go +++ b/coderd/httpmw/oauth2.go @@ -178,12 +178,3 @@ func ExtractOAuth2(config OAuth2Config, client *http.Client, authURLOpts map[str }) } } - -func RemoveOauthStateCookie(rw http.ResponseWriter) { - http.SetCookie(rw, &http.Cookie{ - Name: codersdk.OAuth2StateCookie, - Value: "", - Path: "/", - MaxAge: -1, - }) -} diff --git a/coderd/userauth.go b/coderd/userauth.go index 42fd528ccb04f..615ec262f381b 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -65,6 +65,7 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) { switch req.ToLoginType { case codersdk.LoginTypeGithub, codersdk.LoginTypeOIDC: + // Allowed! case codersdk.LoginTypeNone, codersdk.LoginTypePassword, codersdk.LoginTypeToken: // These login types are not allowed to be converted to at this time. httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ @@ -78,6 +79,7 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) { return } + // This handles the email/pass checking. user, _, ok := api.loginRequest(ctx, rw, req.LoginWithPasswordRequest) if !ok { return @@ -116,11 +118,12 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) { } mergeState, err = store.InsertUserOauthMergeState(ctx, database.InsertUserOauthMergeStateParams{ - UserID: user.ID, - StateString: stateString, - ToLoginType: database.LoginType(req.ToLoginType), - CreatedAt: now, - ExpiresAt: now.Add(time.Minute * 5), + UserID: user.ID, + StateString: stateString, + FromLoginType: user.LoginType, + ToLoginType: database.LoginType(req.ToLoginType), + CreatedAt: now, + ExpiresAt: now.Add(time.Minute * 5), }) if err != nil { return err @@ -175,7 +178,8 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { } user, roles, ok := api.loginRequest(ctx, rw, loginWithPassword) - // 'user.ID' will be empty, or will be an actual value. + // 'user.ID' will be empty, or will be an actual value. Either is correct + // here. aReq.UserID = user.ID if !ok { // user failed to login @@ -1030,9 +1034,13 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook user.LoginType, ), } + // If we do not allow converting to oauth, return an error. if !params.OauthConversionEnabled { return wrongLoginTypeErr } + + // At this point, this request could be an attempt to convert from + // password auth to oauth auth. var ( auditor = *api.Auditor.Load() oauthConvertAudit, commitOauthConvertAudit = params.InitAuditRequest(&audit.RequestParams{ @@ -1052,7 +1060,6 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook if xerrors.Is(err, sql.ErrNoRows) { return wrongLoginTypeErr } - oauthConvertAudit.Old = mergeState failedMsg := fmt.Sprintf("Request to convert login type from %s to %s failed", user.LoginType, params.LoginType) if err != nil { @@ -1061,8 +1068,11 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook msg: failedMsg, } } - if user.ID != mergeState.UserID { - // User tried to use someone else's merge state? + oauthConvertAudit.Old = mergeState + // Make sure the merge state generated matches this OIDC login request. + // It needs to have the correct login type information for this + // user. + if user.ID != mergeState.UserID || user.LoginType != mergeState.FromLoginType || params.LoginType != mergeState.ToLoginType { return httpError{ code: http.StatusForbidden, msg: failedMsg, diff --git a/docs/admin/audit-logs.md b/docs/admin/audit-logs.md index df6ba404582d2..522fa7e5f7d26 100644 --- a/docs/admin/audit-logs.md +++ b/docs/admin/audit-logs.md @@ -15,7 +15,7 @@ We track the following resources: | Group
create, write, delete |
FieldTracked
avatar_urltrue
idtrue
memberstrue
nametrue
organization_idfalse
quota_allowancetrue
| | GitSSHKey
create |
FieldTracked
created_atfalse
private_keytrue
public_keytrue
updated_atfalse
user_idtrue
| | License
create, delete |
FieldTracked
exptrue
idfalse
jwtfalse
uploaded_attrue
uuidtrue
| -| OauthMergeState
|
FieldTracked
created_attrue
expires_attrue
state_stringtrue
to_login_typetrue
user_idtrue
| +| OauthMergeState
|
FieldTracked
created_attrue
expires_attrue
from_login_typetrue
state_stringtrue
to_login_typetrue
user_idtrue
| | Template
write, delete |
FieldTracked
active_version_idtrue
allow_user_autostarttrue
allow_user_autostoptrue
allow_user_cancel_workspace_jobstrue
created_atfalse
created_bytrue
default_ttltrue
deletedfalse
descriptiontrue
display_nametrue
failure_ttltrue
group_acltrue
icontrue
idtrue
inactivity_ttltrue
locked_ttltrue
max_ttltrue
nametrue
organization_idfalse
provisionertrue
updated_atfalse
user_acltrue
| | TemplateVersion
create, write |
FieldTracked
created_atfalse
created_bytrue
git_auth_providersfalse
idtrue
job_idfalse
nametrue
organization_idfalse
readmetrue
template_idtrue
updated_atfalse
| | User
create, write, delete |
FieldTracked
avatar_urlfalse
created_atfalse
deletedtrue
emailtrue
hashed_passwordtrue
idtrue
last_seen_atfalse
login_typefalse
rbac_rolestrue
statustrue
updated_atfalse
usernametrue
| diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go index dec5e34b27006..100e705b91543 100644 --- a/enterprise/audit/table.go +++ b/enterprise/audit/table.go @@ -157,11 +157,12 @@ var auditableResourcesTypes = map[any]map[string]Action{ "token_name": ActionIgnore, }, &database.OauthMergeState{}: { - "state_string": ActionSecret, - "created_at": ActionTrack, - "expires_at": ActionTrack, - "to_login_type": ActionTrack, - "user_id": ActionTrack, + "state_string": ActionSecret, + "created_at": ActionTrack, + "expires_at": ActionTrack, + "from_login_type": ActionTrack, + "to_login_type": ActionTrack, + "user_id": ActionTrack, }, // TODO: track an ID here when the below ticket is completed: // https://github.com/coder/coder/pull/6012 From ddabcf7e70949e4980d5393d7e3320d9f578ee99 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 22 Jun 2023 14:22:33 -0500 Subject: [PATCH 27/58] Extract oauth convert into a helper function --- coderd/userauth.go | 169 +++++++++++++++++++++++++++------------------ 1 file changed, 102 insertions(+), 67 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 615ec262f381b..4d272e8eac836 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -32,6 +32,8 @@ import ( "github.com/coder/coder/cryptorand" ) +const mergeStateStringPrefix = "convert-" + // postConvertLoginType replies with an oauth state token capable of converting // the user to an oauth user. // @@ -105,6 +107,9 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) { }) return } + // The prefix is used to identify this state string as a conversion state + // without needing to hit the database. The random string is the CSRF protection. + stateString = fmt.Sprintf("%s%s", mergeStateStringPrefix, stateString) now := time.Now() var mergeState database.OauthMergeState @@ -1024,74 +1029,12 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook } } - // If this is not a new user and the login type is different, - // we need to check if the user is trying to change their login type. - if user.ID != uuid.Nil && user.LoginType != params.LoginType { - wrongLoginTypeErr := httpError{ - code: http.StatusForbidden, - msg: fmt.Sprintf("Incorrect login type, attempting to use %q but user is of login type %q", - params.LoginType, - user.LoginType, - ), - } - // If we do not allow converting to oauth, return an error. - if !params.OauthConversionEnabled { - return wrongLoginTypeErr - } - - // At this point, this request could be an attempt to convert from - // password auth to oauth auth. - var ( - auditor = *api.Auditor.Load() - oauthConvertAudit, commitOauthConvertAudit = params.InitAuditRequest(&audit.RequestParams{ - Audit: auditor, - Log: api.Logger, - Request: r, - Action: database.AuditActionLogin, - }) - ) - defer commitOauthConvertAudit() - - // nolint:gocritic // Required to auth the oidc convert - mergeState, err := tx.GetUserOauthMergeState(dbauthz.AsSystemRestricted(ctx), database.GetUserOauthMergeStateParams{ - UserID: user.ID, - StateString: params.State.StateString, - }) - if xerrors.Is(err, sql.ErrNoRows) { - return wrongLoginTypeErr - } - - failedMsg := fmt.Sprintf("Request to convert login type from %s to %s failed", user.LoginType, params.LoginType) + // If you do a convert to OIDC and your email does not match, we need to + // catch this and not make a new account. + if isMergeStateString(params.State.StateString) { + err := api.convertUserToOauth(ctx, r, tx, params) if err != nil { - return httpError{ - code: http.StatusForbidden, - msg: failedMsg, - } - } - oauthConvertAudit.Old = mergeState - // Make sure the merge state generated matches this OIDC login request. - // It needs to have the correct login type information for this - // user. - if user.ID != mergeState.UserID || user.LoginType != mergeState.FromLoginType || params.LoginType != mergeState.ToLoginType { - return httpError{ - code: http.StatusForbidden, - msg: failedMsg, - } - } - - // Convert the user and default to the normal login flow. - // If the login succeeds, this transaction will commit and the user - // will be converted. - // nolint:gocritic // system query to update user login type - user, err = tx.UpdateUserLoginType(dbauthz.AsSystemRestricted(ctx), database.UpdateUserLoginTypeParams{ - LoginType: params.LoginType, - UserID: user.ID, - }) - if err != nil { - return httpError{ - code: http.StatusInternalServerError, - msg: "Failed to convert user to new login type", - } + return err } } @@ -1258,6 +1201,91 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook return cookie, *key, nil } +// convertUserToOauth will convert a user from password base loginType to +// an oauth login type. If it fails, it will return a httpError +func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db database.Store, params oauthLoginParams) error { + user := params.User + + // Trying to convert to OIDC, but the email does not match. + // So do not make a new user, just block the request. + if user.ID == uuid.Nil { + return httpError{ + code: http.StatusBadRequest, + msg: fmt.Sprintf("The oidc account with the email %q does not match the email of the account you are trying to convert. Contact your administrator to resolve this issue.", params.Email), + } + } + + // nolint:gocritic // Required to auth the oidc convert + mergeState, err := db.GetUserOauthMergeState(dbauthz.AsSystemRestricted(ctx), database.GetUserOauthMergeStateParams{ + UserID: user.ID, + StateString: params.State.StateString, + }) + if xerrors.Is(err, sql.ErrNoRows) { + return httpError{ + code: http.StatusBadRequest, + msg: "No convert login request found with given state. Restart the convert process and try again.", + } + } + if err != nil { + return httpError{ + code: http.StatusInternalServerError, + msg: err.Error(), + } + } + + // At this point, this request could be an attempt to convert from + // password auth to oauth auth. Always log these attempts. + var ( + auditor = *api.Auditor.Load() + oauthConvertAudit, commitOauthConvertAudit = params.InitAuditRequest(&audit.RequestParams{ + Audit: auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionLogin, + }) + ) + oauthConvertAudit.Old = mergeState + defer commitOauthConvertAudit() + + // If we do not allow converting to oauth, return an error. + if !params.OauthConversionEnabled { + return httpError{ + code: http.StatusForbidden, + msg: fmt.Sprintf("Incorrect login type, attempting to use %q but user is of login type %q", + params.LoginType, + user.LoginType, + ), + } + } + + // Make sure the merge state generated matches this OIDC login request. + // It needs to have the correct login type information for this + // user. + if user.ID != mergeState.UserID || user.LoginType != mergeState.FromLoginType || params.LoginType != mergeState.ToLoginType { + return httpError{ + code: http.StatusForbidden, + msg: fmt.Sprintf("Request to convert login type from %s to %s failed", user.LoginType, params.LoginType), + } + } + + // Convert the user and default to the normal login flow. + // If the login succeeds, this transaction will commit and the user + // will be converted. + // nolint:gocritic // system query to update user login type + user, err = db.UpdateUserLoginType(dbauthz.AsSystemRestricted(ctx), database.UpdateUserLoginTypeParams{ + LoginType: params.LoginType, + UserID: user.ID, + }) + if err != nil { + return httpError{ + code: http.StatusInternalServerError, + msg: "Failed to convert user to new login type", + } + } + return nil + +} + // githubLinkedID returns the unique ID for a GitHub user. func githubLinkedID(u *github.User) string { return strconv.FormatInt(u.GetID(), 10) @@ -1324,3 +1352,10 @@ func findLinkedUser(ctx context.Context, db database.Store, linkedID string, ema return user, link, nil } + +func isMergeStateString(state string) bool { + if strings.HasPrefix(state, mergeStateStringPrefix) { + return true + } + return false +} From 57b36054192a547f0053117b53866e215eccd81a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 22 Jun 2023 15:07:50 -0500 Subject: [PATCH 28/58] chore: add boolean to auth methods to know if the deployment supports oidc conversion --- coderd/apidoc/docs.go | 3 +++ coderd/apidoc/swagger.json | 3 +++ coderd/userauth.go | 1 + codersdk/users.go | 1 + docs/api/schemas.md | 14 ++++++++------ docs/api/users.md | 1 + site/src/api/typesGenerated.ts | 1 + 7 files changed, 18 insertions(+), 6 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index d156cfa473bdb..fdee1fa1a88df 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -6514,6 +6514,9 @@ const docTemplate = `{ "codersdk.AuthMethods": { "type": "object", "properties": { + "convert_to_oidc_enabled": { + "type": "boolean" + }, "github": { "$ref": "#/definitions/codersdk.AuthMethod" }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 4f9b677871744..de1795a0c2f3e 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -5797,6 +5797,9 @@ "codersdk.AuthMethods": { "type": "object", "properties": { + "convert_to_oidc_enabled": { + "type": "boolean" + }, "github": { "$ref": "#/definitions/codersdk.AuthMethod" }, diff --git a/coderd/userauth.go b/coderd/userauth.go index 4d272e8eac836..d690681cef581 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -411,6 +411,7 @@ func (api *API) userAuthMethods(rw http.ResponseWriter, r *http.Request) { httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.AuthMethods{ UserAuthenticationType: currentUserLoginType, + ConvertToOIDCEnabled: api.Options.DeploymentValues.EnableOauthAccountConversion.Value(), Password: codersdk.AuthMethod{ Enabled: !api.DeploymentValues.DisablePasswordAuth.Value(), }, diff --git a/codersdk/users.go b/codersdk/users.go index 18ff02d607e95..497db3dc659d2 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -126,6 +126,7 @@ type AuthMethods struct { // UserAuthenticationType returns the authentication method for the given // caller if the request is an authenticated request. Otherwise it is empty. UserAuthenticationType LoginType `json:"me_login_type,omitempty"` + ConvertToOIDCEnabled bool `json:"convert_to_oidc_enabled"` Password AuthMethod `json:"password"` Github AuthMethod `json:"github"` OIDC OIDCAuthMethod `json:"oidc"` diff --git a/docs/api/schemas.md b/docs/api/schemas.md index 2a140faafa5e3..060c13bd14dde 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -1107,6 +1107,7 @@ ```json { + "convert_to_oidc_enabled": true, "github": { "enabled": true }, @@ -1124,12 +1125,13 @@ ### Properties -| Name | Type | Required | Restrictions | Description | -| --------------- | -------------------------------------------------- | -------- | ------------ | --------------------------------------------------------------------------------------------------------------------------------------- | -| `github` | [codersdk.AuthMethod](#codersdkauthmethod) | false | | | -| `me_login_type` | [codersdk.LoginType](#codersdklogintype) | false | | Me login type returns the authentication method for the given caller if the request is an authenticated request. Otherwise it is empty. | -| `oidc` | [codersdk.OIDCAuthMethod](#codersdkoidcauthmethod) | false | | | -| `password` | [codersdk.AuthMethod](#codersdkauthmethod) | false | | | +| Name | Type | Required | Restrictions | Description | +| ------------------------- | -------------------------------------------------- | -------- | ------------ | --------------------------------------------------------------------------------------------------------------------------------------- | +| `convert_to_oidc_enabled` | boolean | false | | | +| `github` | [codersdk.AuthMethod](#codersdkauthmethod) | false | | | +| `me_login_type` | [codersdk.LoginType](#codersdklogintype) | false | | Me login type returns the authentication method for the given caller if the request is an authenticated request. Otherwise it is empty. | +| `oidc` | [codersdk.OIDCAuthMethod](#codersdkoidcauthmethod) | false | | | +| `password` | [codersdk.AuthMethod](#codersdkauthmethod) | false | | | ## codersdk.AuthorizationCheck diff --git a/docs/api/users.md b/docs/api/users.md index cc01acac6d366..667080b581ee4 100644 --- a/docs/api/users.md +++ b/docs/api/users.md @@ -140,6 +140,7 @@ curl -X GET http://coder-server:8080/api/v2/users/authmethods \ ```json { + "convert_to_oidc_enabled": true, "github": { "enabled": true }, diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index f410d2bc1defb..7ddf883b836de 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -105,6 +105,7 @@ export interface AuthMethod { // From codersdk/users.go export interface AuthMethods { readonly me_login_type?: LoginType + readonly convert_to_oidc_enabled: boolean readonly password: AuthMethod readonly github: AuthMethod readonly oidc: OIDCAuthMethod From 2be844f02a616bc73cc6a0847839bd0a65e7bb70 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 26 Jun 2023 08:28:41 -0500 Subject: [PATCH 29/58] Add back missing code from merge --- coderd/userauth.go | 108 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 106 insertions(+), 2 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 4bf963f7567b7..718a5868eff88 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -1015,8 +1015,11 @@ type oauthLoginParams struct { AvatarURL string // Is UsingGroups is true, then the user will be assigned // to the Groups provided. - UsingGroups bool - Groups []string + UsingGroups bool + Groups []string + OauthConversionEnabled bool + + InitAuditRequest func(params *audit.RequestParams) (*audit.Request[database.OauthMergeState], func()) } type httpError struct { @@ -1048,6 +1051,15 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook user = params.User link = params.Link + // If you do a convert to OIDC and your email does not match, we need to + // catch this and not make a new account. + if isMergeStateString(params.State.StateString) { + err := api.convertUserToOauth(ctx, r, tx, params) + if err != nil { + return err + } + } + if user.ID == uuid.Nil && !params.AllowSignups { return httpError{ code: http.StatusForbidden, @@ -1228,6 +1240,91 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook return cookie, *key, nil } +// convertUserToOauth will convert a user from password base loginType to +// an oauth login type. If it fails, it will return a httpError +func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db database.Store, params oauthLoginParams) error { + user := params.User + + // Trying to convert to OIDC, but the email does not match. + // So do not make a new user, just block the request. + if user.ID == uuid.Nil { + return httpError{ + code: http.StatusBadRequest, + msg: fmt.Sprintf("The oidc account with the email %q does not match the email of the account you are trying to convert. Contact your administrator to resolve this issue.", params.Email), + } + } + + // nolint:gocritic // Required to auth the oidc convert + mergeState, err := db.GetUserOauthMergeState(dbauthz.AsSystemRestricted(ctx), database.GetUserOauthMergeStateParams{ + UserID: user.ID, + StateString: params.State.StateString, + }) + if xerrors.Is(err, sql.ErrNoRows) { + return httpError{ + code: http.StatusBadRequest, + msg: "No convert login request found with given state. Restart the convert process and try again.", + } + } + if err != nil { + return httpError{ + code: http.StatusInternalServerError, + msg: err.Error(), + } + } + + // At this point, this request could be an attempt to convert from + // password auth to oauth auth. Always log these attempts. + var ( + auditor = *api.Auditor.Load() + oauthConvertAudit, commitOauthConvertAudit = params.InitAuditRequest(&audit.RequestParams{ + Audit: auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionLogin, + }) + ) + oauthConvertAudit.Old = mergeState + defer commitOauthConvertAudit() + + // If we do not allow converting to oauth, return an error. + if !params.OauthConversionEnabled { + return httpError{ + code: http.StatusForbidden, + msg: fmt.Sprintf("Incorrect login type, attempting to use %q but user is of login type %q", + params.LoginType, + user.LoginType, + ), + } + } + + // Make sure the merge state generated matches this OIDC login request. + // It needs to have the correct login type information for this + // user. + if user.ID != mergeState.UserID || user.LoginType != mergeState.FromLoginType || params.LoginType != mergeState.ToLoginType { + return httpError{ + code: http.StatusForbidden, + msg: fmt.Sprintf("Request to convert login type from %s to %s failed", user.LoginType, params.LoginType), + } + } + + // Convert the user and default to the normal login flow. + // If the login succeeds, this transaction will commit and the user + // will be converted. + // nolint:gocritic // system query to update user login type + user, err = db.UpdateUserLoginType(dbauthz.AsSystemRestricted(ctx), database.UpdateUserLoginTypeParams{ + LoginType: params.LoginType, + UserID: user.ID, + }) + if err != nil { + return httpError{ + code: http.StatusInternalServerError, + msg: "Failed to convert user to new login type", + } + } + return nil + +} + // githubLinkedID returns the unique ID for a GitHub user. func githubLinkedID(u *github.User) string { return strconv.FormatInt(u.GetID(), 10) @@ -1294,3 +1391,10 @@ func findLinkedUser(ctx context.Context, db database.Store, linkedID string, ema return user, link, nil } + +func isMergeStateString(state string) bool { + if strings.HasPrefix(state, mergeStateStringPrefix) { + return true + } + return false +} From fde490852708f827e4cf9f9622bf0b8c60a39a36 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 26 Jun 2023 08:58:56 -0500 Subject: [PATCH 30/58] Linting --- coderd/userauth.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 718a5868eff88..8156caea55d2e 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -1322,7 +1322,6 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data } } return nil - } // githubLinkedID returns the unique ID for a GitHub user. @@ -1393,8 +1392,5 @@ func findLinkedUser(ctx context.Context, db database.Store, linkedID string, ema } func isMergeStateString(state string) bool { - if strings.HasPrefix(state, mergeStateStringPrefix) { - return true - } - return false + return strings.HasPrefix(state, mergeStateStringPrefix) } From 810e99615dbbbbd6afb1cfceaf5ef1246579c54f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 26 Jun 2023 09:03:57 -0500 Subject: [PATCH 31/58] Fix merge error --- coderd/userauth.go | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 8156caea55d2e..face05b414100 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -1054,10 +1054,11 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook // If you do a convert to OIDC and your email does not match, we need to // catch this and not make a new account. if isMergeStateString(params.State.StateString) { - err := api.convertUserToOauth(ctx, r, tx, params) + user, err = api.convertUserToOauth(ctx, r, tx, params) if err != nil { return err } + params.User = user } if user.ID == uuid.Nil && !params.AllowSignups { @@ -1242,13 +1243,13 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook // convertUserToOauth will convert a user from password base loginType to // an oauth login type. If it fails, it will return a httpError -func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db database.Store, params oauthLoginParams) error { +func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db database.Store, params oauthLoginParams) (database.User, error) { user := params.User // Trying to convert to OIDC, but the email does not match. // So do not make a new user, just block the request. if user.ID == uuid.Nil { - return httpError{ + return database.User{}, httpError{ code: http.StatusBadRequest, msg: fmt.Sprintf("The oidc account with the email %q does not match the email of the account you are trying to convert. Contact your administrator to resolve this issue.", params.Email), } @@ -1260,13 +1261,13 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data StateString: params.State.StateString, }) if xerrors.Is(err, sql.ErrNoRows) { - return httpError{ + return database.User{}, httpError{ code: http.StatusBadRequest, msg: "No convert login request found with given state. Restart the convert process and try again.", } } if err != nil { - return httpError{ + return database.User{}, httpError{ code: http.StatusInternalServerError, msg: err.Error(), } @@ -1288,7 +1289,7 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data // If we do not allow converting to oauth, return an error. if !params.OauthConversionEnabled { - return httpError{ + return database.User{}, httpError{ code: http.StatusForbidden, msg: fmt.Sprintf("Incorrect login type, attempting to use %q but user is of login type %q", params.LoginType, @@ -1301,7 +1302,7 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data // It needs to have the correct login type information for this // user. if user.ID != mergeState.UserID || user.LoginType != mergeState.FromLoginType || params.LoginType != mergeState.ToLoginType { - return httpError{ + return database.User{}, httpError{ code: http.StatusForbidden, msg: fmt.Sprintf("Request to convert login type from %s to %s failed", user.LoginType, params.LoginType), } @@ -1316,12 +1317,12 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data UserID: user.ID, }) if err != nil { - return httpError{ + return database.User{}, httpError{ code: http.StatusInternalServerError, msg: "Failed to convert user to new login type", } } - return nil + return user, nil } // githubLinkedID returns the unique ID for a GitHub user. From f3bce86243498afaadedc7cac4c71841b2e9102a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 26 Jun 2023 12:26:10 -0400 Subject: [PATCH 32/58] Move error message to route --- coderd/coderd.go | 8 +------- coderd/userauth.go | 7 +++++++ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 9d12193cdd31a..9791cf8f78a45 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -626,13 +626,7 @@ func New(options *Options) *API { r.Use( apiKeyMiddleware, ) - if allowOauthConversion { - r.Post("/", api.postConvertLoginType) - } else { - r.Post("/", func(rw http.ResponseWriter, r *http.Request) { - http.Error(rw, "Oauth conversion is not allowed, contact an administrator to turn on this feature.", http.StatusForbidden) - }) - } + r.Post("/", api.postConvertLoginType) }) }) r.Group(func(r chi.Router) { diff --git a/coderd/userauth.go b/coderd/userauth.go index face05b414100..3bf927bab1dcb 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -50,6 +50,13 @@ const mergeStateStringPrefix = "convert-" // @Success 201 {object} codersdk.OauthConversionResponse // @Router /users/convert-login [post] func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) { + if !api.Options.DeploymentValues.EnableOauthAccountConversion.Value() { + httpapi.Write(r.Context(), rw, http.StatusForbidden, codersdk.Response{ + Message: "Oauth conversion is not allowed, contact an administrator to turn on this feature.", + }) + return + } + var ( ctx = r.Context() auditor = api.Auditor.Load() From a2510a290735061d4efdb195014bd01a16afc0ff Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 26 Jun 2023 13:02:42 -0400 Subject: [PATCH 33/58] Rename state_string to state --- coderd/coderd.go | 2 -- coderd/database/dbfake/dbfake.go | 4 ++-- coderd/database/dump.sql | 4 ++-- .../migrations/000131_merge_oidc_account.up.sql | 4 ++-- coderd/database/models.go | 4 ++-- coderd/database/queries.sql.go | 16 ++++++++-------- coderd/database/queries/users.sql | 4 ++-- coderd/userauth.go | 4 ++-- docs/admin/audit-logs.md | 2 +- enterprise/audit/table.go | 2 +- 10 files changed, 22 insertions(+), 24 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 9791cf8f78a45..1de880ff5048f 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -395,8 +395,6 @@ func New(options *Options) *API { Optional: true, }) - allowOauthConversion := options.DeploymentValues.EnableOauthAccountConversion.Value() - // API rate limit middleware. The counter is local and not shared between // replicas or instances of this middleware. apiRateLimiter := httpmw.RateLimit(options.APIRateLimit, time.Minute) diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 87b3305e8ab91..286af7e28cf7a 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -2655,7 +2655,7 @@ func (q *fakeQuerier) GetUserOauthMergeState(_ context.Context, arg database.Get defer q.mutex.RUnlock() for _, s := range q.oauthMergeStates { - if s.StateString == arg.StateString && s.UserID == arg.UserID { + if s.State == arg.StateString && s.UserID == arg.UserID { return s, nil } } @@ -4092,7 +4092,7 @@ func (q *fakeQuerier) InsertUserOauthMergeState(_ context.Context, arg database. } s := database.OauthMergeState{ - StateString: arg.StateString, + State: arg.State, CreatedAt: arg.CreatedAt, ExpiresAt: arg.ExpiresAt, FromLoginType: arg.FromLoginType, diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index b31022efffd39..34e6ccdf91841 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -307,7 +307,7 @@ CREATE SEQUENCE licenses_id_seq ALTER SEQUENCE licenses_id_seq OWNED BY licenses.id; CREATE TABLE oauth_merge_state ( - state_string text NOT NULL, + state text NOT NULL, created_at timestamp with time zone NOT NULL, expires_at timestamp with time zone NOT NULL, from_login_type login_type NOT NULL, @@ -880,7 +880,7 @@ ALTER TABLE ONLY licenses ADD CONSTRAINT licenses_pkey PRIMARY KEY (id); ALTER TABLE ONLY oauth_merge_state - ADD CONSTRAINT oauth_merge_state_pkey PRIMARY KEY (state_string); + ADD CONSTRAINT oauth_merge_state_pkey PRIMARY KEY (state); ALTER TABLE ONLY organization_members ADD CONSTRAINT organization_members_pkey PRIMARY KEY (organization_id, user_id); diff --git a/coderd/database/migrations/000131_merge_oidc_account.up.sql b/coderd/database/migrations/000131_merge_oidc_account.up.sql index af774ff1f301f..482edc5433388 100644 --- a/coderd/database/migrations/000131_merge_oidc_account.up.sql +++ b/coderd/database/migrations/000131_merge_oidc_account.up.sql @@ -1,14 +1,14 @@ BEGIN; CREATE TABLE IF NOT EXISTS oauth_merge_state ( - state_string text NOT NULL, + state text NOT NULL, created_at timestamptz NOT NULL, expires_at timestamptz NOT NULL, from_login_type login_type NOT NULL, to_login_type login_type NOT NULL, user_id uuid NOT NULL REFERENCES users (id) ON DELETE CASCADE, - PRIMARY KEY (state_string) + PRIMARY KEY (state) ); COMMENT ON TABLE oauth_merge_state IS 'Stores the state string for Oauth merge requests. If an Oauth state string is found in this table, ' diff --git a/coderd/database/models.go b/coderd/database/models.go index decf36e026349..d6eb448a6577e 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -1431,8 +1431,8 @@ type License struct { // Stores the state string for Oauth merge requests. If an Oauth state string is found in this table, it is assumed the user had a LoginType "password" and is switching to an Oauth based authentication. type OauthMergeState struct { - StateString string `db:"state_string" json:"state_string"` - CreatedAt time.Time `db:"created_at" json:"created_at"` + State string `db:"state" json:"state"` + CreatedAt time.Time `db:"created_at" json:"created_at"` // The time at which the state string expires, a merge request times out if the user does not perform it quick enough. ExpiresAt time.Time `db:"expires_at" json:"expires_at"` FromLoginType LoginType `db:"from_login_type" json:"from_login_type"` diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 126ceaf9cca1c..7f50126835287 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5210,12 +5210,12 @@ func (q *sqlQuerier) GetUserCount(ctx context.Context) (int64, error) { const getUserOauthMergeState = `-- name: GetUserOauthMergeState :one SELECT - state_string, created_at, expires_at, from_login_type, to_login_type, user_id + state, created_at, expires_at, from_login_type, to_login_type, user_id FROM oauth_merge_state WHERE user_id = $1 AND - state_string = $2 + state = $2 ` type GetUserOauthMergeStateParams struct { @@ -5227,7 +5227,7 @@ func (q *sqlQuerier) GetUserOauthMergeState(ctx context.Context, arg GetUserOaut row := q.db.QueryRowContext(ctx, getUserOauthMergeState, arg.UserID, arg.StateString) var i OauthMergeState err := row.Scan( - &i.StateString, + &i.State, &i.CreatedAt, &i.ExpiresAt, &i.FromLoginType, @@ -5485,19 +5485,19 @@ const insertUserOauthMergeState = `-- name: InsertUserOauthMergeState :one INSERT INTO oauth_merge_state ( user_id, - state_string, + state, from_login_type, to_login_type, created_at, expires_at ) VALUES - ($1, $2, $3, $4, $5, $6) RETURNING state_string, created_at, expires_at, from_login_type, to_login_type, user_id + ($1, $2, $3, $4, $5, $6) RETURNING state, created_at, expires_at, from_login_type, to_login_type, user_id ` type InsertUserOauthMergeStateParams struct { UserID uuid.UUID `db:"user_id" json:"user_id"` - StateString string `db:"state_string" json:"state_string"` + State string `db:"state" json:"state"` FromLoginType LoginType `db:"from_login_type" json:"from_login_type"` ToLoginType LoginType `db:"to_login_type" json:"to_login_type"` CreatedAt time.Time `db:"created_at" json:"created_at"` @@ -5507,7 +5507,7 @@ type InsertUserOauthMergeStateParams struct { func (q *sqlQuerier) InsertUserOauthMergeState(ctx context.Context, arg InsertUserOauthMergeStateParams) (OauthMergeState, error) { row := q.db.QueryRowContext(ctx, insertUserOauthMergeState, arg.UserID, - arg.StateString, + arg.State, arg.FromLoginType, arg.ToLoginType, arg.CreatedAt, @@ -5515,7 +5515,7 @@ func (q *sqlQuerier) InsertUserOauthMergeState(ctx context.Context, arg InsertUs ) var i OauthMergeState err := row.Scan( - &i.StateString, + &i.State, &i.CreatedAt, &i.ExpiresAt, &i.FromLoginType, diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index 2298bba12893d..f60132484ddde 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -5,13 +5,13 @@ FROM oauth_merge_state WHERE user_id = @user_id AND - state_string = @state_string; + state = @state_string; -- name: InsertUserOauthMergeState :one INSERT INTO oauth_merge_state ( user_id, - state_string, + state, from_login_type, to_login_type, created_at, diff --git a/coderd/userauth.go b/coderd/userauth.go index 3bf927bab1dcb..e09b38cf3fe99 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -134,7 +134,7 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) { mergeState, err = store.InsertUserOauthMergeState(ctx, database.InsertUserOauthMergeStateParams{ UserID: user.ID, - StateString: stateString, + State: stateString, FromLoginType: user.LoginType, ToLoginType: database.LoginType(req.ToLoginType), CreatedAt: now, @@ -156,7 +156,7 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) { aReq.New = mergeState httpapi.Write(ctx, rw, http.StatusCreated, codersdk.OauthConversionResponse{ - StateString: mergeState.StateString, + StateString: mergeState.State, ExpiresAt: mergeState.ExpiresAt, ToLoginType: codersdk.LoginType(mergeState.ToLoginType), UserID: mergeState.UserID, diff --git a/docs/admin/audit-logs.md b/docs/admin/audit-logs.md index 522fa7e5f7d26..de3f5f720e108 100644 --- a/docs/admin/audit-logs.md +++ b/docs/admin/audit-logs.md @@ -15,7 +15,7 @@ We track the following resources: | Group
create, write, delete |
FieldTracked
avatar_urltrue
idtrue
memberstrue
nametrue
organization_idfalse
quota_allowancetrue
| | GitSSHKey
create |
FieldTracked
created_atfalse
private_keytrue
public_keytrue
updated_atfalse
user_idtrue
| | License
create, delete |
FieldTracked
exptrue
idfalse
jwtfalse
uploaded_attrue
uuidtrue
| -| OauthMergeState
|
FieldTracked
created_attrue
expires_attrue
from_login_typetrue
state_stringtrue
to_login_typetrue
user_idtrue
| +| OauthMergeState
|
FieldTracked
created_attrue
expires_attrue
from_login_typetrue
statetrue
to_login_typetrue
user_idtrue
| | Template
write, delete |
FieldTracked
active_version_idtrue
allow_user_autostarttrue
allow_user_autostoptrue
allow_user_cancel_workspace_jobstrue
created_atfalse
created_bytrue
default_ttltrue
deletedfalse
descriptiontrue
display_nametrue
failure_ttltrue
group_acltrue
icontrue
idtrue
inactivity_ttltrue
locked_ttltrue
max_ttltrue
nametrue
organization_idfalse
provisionertrue
updated_atfalse
user_acltrue
| | TemplateVersion
create, write |
FieldTracked
created_atfalse
created_bytrue
git_auth_providersfalse
idtrue
job_idfalse
nametrue
organization_idfalse
readmetrue
template_idtrue
updated_atfalse
| | User
create, write, delete |
FieldTracked
avatar_urlfalse
created_atfalse
deletedtrue
emailtrue
hashed_passwordtrue
idtrue
last_seen_atfalse
login_typefalse
rbac_rolestrue
statustrue
updated_atfalse
usernametrue
| diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go index 100e705b91543..883109c0b0a75 100644 --- a/enterprise/audit/table.go +++ b/enterprise/audit/table.go @@ -157,7 +157,7 @@ var auditableResourcesTypes = map[any]map[string]Action{ "token_name": ActionIgnore, }, &database.OauthMergeState{}: { - "state_string": ActionSecret, + "state": ActionSecret, "created_at": ActionTrack, "expires_at": ActionTrack, "from_login_type": ActionTrack, From 48db46cfa0c0ae8a5fc3b585d5cb3bf5c7bd7dea Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 28 Jun 2023 09:42:45 -0400 Subject: [PATCH 34/58] Fix js unit tests --- site/src/components/SignInForm/SignInForm.stories.tsx | 6 ++++++ site/src/pages/LoginPage/LoginPage.test.tsx | 2 ++ 2 files changed, 8 insertions(+) diff --git a/site/src/components/SignInForm/SignInForm.stories.tsx b/site/src/components/SignInForm/SignInForm.stories.tsx index ea3610e7ba49d..fb49792dc99de 100644 --- a/site/src/components/SignInForm/SignInForm.stories.tsx +++ b/site/src/components/SignInForm/SignInForm.stories.tsx @@ -28,6 +28,7 @@ SigningIn.args = { ...SignedOut.args, isSigningIn: true, authMethods: { + convert_to_oidc_enabled: false, password: { enabled: true }, github: { enabled: true }, oidc: { enabled: false, signInText: "", iconUrl: "" }, @@ -55,6 +56,7 @@ export const WithGithub = Template.bind({}) WithGithub.args = { ...SignedOut.args, authMethods: { + convert_to_oidc_enabled: false, password: { enabled: true }, github: { enabled: true }, oidc: { enabled: false, signInText: "", iconUrl: "" }, @@ -65,6 +67,7 @@ export const WithOIDC = Template.bind({}) WithOIDC.args = { ...SignedOut.args, authMethods: { + convert_to_oidc_enabled: false, password: { enabled: true }, github: { enabled: false }, oidc: { enabled: true, signInText: "", iconUrl: "" }, @@ -75,6 +78,7 @@ export const WithOIDCWithoutPassword = Template.bind({}) WithOIDCWithoutPassword.args = { ...SignedOut.args, authMethods: { + convert_to_oidc_enabled: false, password: { enabled: false }, github: { enabled: false }, oidc: { enabled: true, signInText: "", iconUrl: "" }, @@ -85,6 +89,7 @@ export const WithoutAny = Template.bind({}) WithoutAny.args = { ...SignedOut.args, authMethods: { + convert_to_oidc_enabled: false, password: { enabled: false }, github: { enabled: false }, oidc: { enabled: false, signInText: "", iconUrl: "" }, @@ -95,6 +100,7 @@ export const WithGithubAndOIDC = Template.bind({}) WithGithubAndOIDC.args = { ...SignedOut.args, authMethods: { + convert_to_oidc_enabled: false, password: { enabled: true }, github: { enabled: true }, oidc: { enabled: true, signInText: "", iconUrl: "" }, diff --git a/site/src/pages/LoginPage/LoginPage.test.tsx b/site/src/pages/LoginPage/LoginPage.test.tsx index e7dcd782fcfa5..3fc3ad1369874 100644 --- a/site/src/pages/LoginPage/LoginPage.test.tsx +++ b/site/src/pages/LoginPage/LoginPage.test.tsx @@ -61,6 +61,7 @@ describe("LoginPage", () => { it("shows github authentication when enabled", async () => { const authMethods: TypesGen.AuthMethods = { + convert_to_oidc_enabled: false, password: { enabled: true }, github: { enabled: true }, oidc: { enabled: true, signInText: "", iconUrl: "" }, @@ -112,6 +113,7 @@ describe("LoginPage", () => { it("hides password authentication if OIDC/GitHub is enabled and displays on click", async () => { const authMethods: TypesGen.AuthMethods = { + convert_to_oidc_enabled: false, password: { enabled: true }, github: { enabled: true }, oidc: { enabled: true, signInText: "", iconUrl: "" }, From c2aea700f14469ffdd93690ca9de4af1e67836c7 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 28 Jun 2023 10:02:38 -0400 Subject: [PATCH 35/58] Fix migration fixture --- .../testdata/fixtures/000131_oauth_merge_state.up.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/database/migrations/testdata/fixtures/000131_oauth_merge_state.up.sql b/coderd/database/migrations/testdata/fixtures/000131_oauth_merge_state.up.sql index 5ac3ff510875b..99247c471d11d 100644 --- a/coderd/database/migrations/testdata/fixtures/000131_oauth_merge_state.up.sql +++ b/coderd/database/migrations/testdata/fixtures/000131_oauth_merge_state.up.sql @@ -1,5 +1,5 @@ INSERT INTO oauth_merge_state( - state_string, + state, created_at, expires_at, from_login_type, From 6294fb5887cdcedc955b5c01e5af1a398e61a1f5 Mon Sep 17 00:00:00 2001 From: Bruno Quaresma Date: Wed, 28 Jun 2023 13:26:58 -0300 Subject: [PATCH 36/58] feat(site): add change OIDC UI (#8182) * Minor form spacing and verbiage improvements * Add basic flow using mocks * Change the UI flow * End * Add storybooks * Change label name * Fic previous tests * Add integration test * Fix issues * Fix storybook stories --- .vscode/settings.json | 3 +- coderd/userauth.go | 4 +- site/src/api/api.ts | 33 +-- .../SettingsAccountForm.test.tsx | 4 +- .../SettingsAccountForm.tsx | 12 +- .../src/components/SettingsLayout/Section.tsx | 11 +- .../SettingsSecurityForm.stories.tsx | 11 +- .../SettingsSecurityForm.tsx | 48 ++-- .../SecurityPage/SecurityPage.test.tsx | 196 +++++++------ .../SecurityPage/SecurityPage.tsx | 82 ++++-- .../SecurityPage/SecurityPageView.stories.tsx | 69 +++++ .../SecurityPage/SingleSignOnSection.tsx | 259 ++++++++++++++++++ site/src/testHelpers/entities.ts | 8 + 13 files changed, 565 insertions(+), 175 deletions(-) create mode 100644 site/src/pages/UserSettingsPage/SecurityPage/SecurityPageView.stories.tsx create mode 100644 site/src/pages/UserSettingsPage/SecurityPage/SingleSignOnSection.tsx diff --git a/.vscode/settings.json b/.vscode/settings.json index 1ae5f3419f969..1ff3ea0883482 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -211,6 +211,5 @@ "go.testFlags": ["-short", "-coverpkg=./..."], // We often use a version of TypeScript that's ahead of the version shipped // with VS Code. - "typescript.tsdk": "./site/node_modules/typescript/lib", - "prettier.prettierPath": "./node_modules/prettier" + "typescript.tsdk": "./site/node_modules/typescript/lib" } diff --git a/coderd/userauth.go b/coderd/userauth.go index e09b38cf3fe99..92e93474a8b51 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -840,7 +840,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { // Convert the []interface{} we get to a []string. groupsInterface, ok := groupsRaw.([]interface{}) if ok { - logger.Debug(ctx, "groups returned in oidc claims", + api.Logger.Debug(ctx, "groups returned in oidc claims", slog.F("len", len(groupsInterface)), slog.F("groups", groupsInterface), ) @@ -861,7 +861,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { groups = append(groups, group) } } else { - logger.Debug(ctx, "groups field was an unknown type", + api.Logger.Debug(ctx, "groups field was an unknown type", slog.F("type", fmt.Sprintf("%T", groupsRaw)), ) } diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 2ce560d22880c..0dff38cff4786 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -108,33 +108,12 @@ export const login = async ( return response.data } -export const convertToOauth = async ( - email: string, - password: string, - to_login_type: string, -): Promise => { - const payload = JSON.stringify({ - email, - password, - to_login_type, - }) - - try { - const response = await axios.post( - "/api/v2/users/convert-login", - payload, - { - headers: { ...CONTENT_TYPE_JSON }, - }, - ) - return response.data - } catch (error) { - if (axios.isAxiosError(error) && error.response?.status === 401) { - return undefined - } - - throw error - } +export const convertToOAUTH = async (request: TypesGen.ConvertLoginRequest) => { + const response = await axios.post( + "/api/v2/users/convert-login", + request, + ) + return response.data } export const logout = async (): Promise => { diff --git a/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx b/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx index df5268883ad2e..000e6a8adc3fc 100644 --- a/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx +++ b/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx @@ -31,7 +31,7 @@ describe("AccountForm", () => { const el = await screen.findByLabelText("Username") expect(el).toBeEnabled() const btn = await screen.findByRole("button", { - name: /Update settings/i, + name: /Update account/i, }) expect(btn).toBeEnabled() }) @@ -61,7 +61,7 @@ describe("AccountForm", () => { const el = await screen.findByLabelText("Username") expect(el).toBeDisabled() const btn = await screen.findByRole("button", { - name: /Update settings/i, + name: /Update account/i, }) expect(btn).toBeDisabled() }) diff --git a/site/src/components/SettingsAccountForm/SettingsAccountForm.tsx b/site/src/components/SettingsAccountForm/SettingsAccountForm.tsx index 1525e1ecb37a9..0141146e8f84b 100644 --- a/site/src/components/SettingsAccountForm/SettingsAccountForm.tsx +++ b/site/src/components/SettingsAccountForm/SettingsAccountForm.tsx @@ -8,8 +8,8 @@ import { onChangeTrimmed, } from "../../utils/formUtils" import { LoadingButton } from "../LoadingButton/LoadingButton" -import { Stack } from "../Stack/Stack" import { ErrorAlert } from "components/Alert/ErrorAlert" +import { Form, FormFields } from "components/Form/Form" export interface AccountFormValues { username: string @@ -18,7 +18,7 @@ export interface AccountFormValues { export const Language = { usernameLabel: "Username", emailLabel: "Email", - updateSettings: "Update settings", + updateSettings: "Update account", } const validationSchema = Yup.object({ @@ -59,8 +59,8 @@ export const AccountForm: FC> = ({ return ( <> -
- + + {Boolean(updateProfileError) && ( )} @@ -91,8 +91,8 @@ export const AccountForm: FC> = ({ {isLoading ? "" : Language.updateSettings} - -
+ + ) } diff --git a/site/src/components/SettingsLayout/Section.tsx b/site/src/components/SettingsLayout/Section.tsx index 29e761fdec030..35419ff4ab89d 100644 --- a/site/src/components/SettingsLayout/Section.tsx +++ b/site/src/components/SettingsLayout/Section.tsx @@ -6,6 +6,8 @@ import { SectionAction } from "../SectionAction/SectionAction" type SectionLayout = "fixed" | "fluid" export interface SectionProps { + // Useful for testing + id?: string title?: ReactNode | string description?: ReactNode toolbar?: ReactNode @@ -20,6 +22,7 @@ type SectionFC = FC> & { } export const Section: SectionFC = ({ + id, title, description, toolbar, @@ -30,12 +33,16 @@ export const Section: SectionFC = ({ }) => { const styles = useStyles({ layout }) return ( -
+
{(title || description) && (
- {title && {title}} + {title && ( + + {title} + + )} {description && typeof description === "string" && ( {description} diff --git a/site/src/components/SettingsSecurityForm/SettingsSecurityForm.stories.tsx b/site/src/components/SettingsSecurityForm/SettingsSecurityForm.stories.tsx index d2ca25ff16bd0..506b81270eebc 100644 --- a/site/src/components/SettingsSecurityForm/SettingsSecurityForm.stories.tsx +++ b/site/src/components/SettingsSecurityForm/SettingsSecurityForm.stories.tsx @@ -17,12 +17,6 @@ const Template: Story = (args: SecurityFormProps) => ( export const Example = Template.bind({}) Example.args = { isLoading: false, - initialValues: { - old_password: "", - password: "", - confirm_password: "", - }, - updateSecurityError: undefined, onSubmit: () => { return Promise.resolve() }, @@ -37,7 +31,7 @@ Loading.args = { export const WithError = Template.bind({}) WithError.args = { ...Example.args, - updateSecurityError: mockApiError({ + error: mockApiError({ message: "Old password is incorrect", validations: [ { @@ -46,7 +40,4 @@ WithError.args = { }, ], }), - initialTouched: { - old_password: true, - }, } diff --git a/site/src/components/SettingsSecurityForm/SettingsSecurityForm.tsx b/site/src/components/SettingsSecurityForm/SettingsSecurityForm.tsx index 0335b4cfd77c6..39230f3837b1b 100644 --- a/site/src/components/SettingsSecurityForm/SettingsSecurityForm.tsx +++ b/site/src/components/SettingsSecurityForm/SettingsSecurityForm.tsx @@ -1,11 +1,12 @@ import TextField from "@mui/material/TextField" -import { FormikContextType, FormikTouched, useFormik } from "formik" +import { FormikContextType, useFormik } from "formik" import { FC } from "react" import * as Yup from "yup" import { getFormHelpers } from "../../utils/formUtils" import { LoadingButton } from "../LoadingButton/LoadingButton" -import { Stack } from "../Stack/Stack" import { ErrorAlert } from "components/Alert/ErrorAlert" +import { Form, FormFields } from "components/Form/Form" +import { Alert } from "components/Alert/Alert" interface SecurityFormValues { old_password: string @@ -41,40 +42,43 @@ const validationSchema = Yup.object({ }) export interface SecurityFormProps { + disabled: boolean isLoading: boolean - initialValues: SecurityFormValues onSubmit: (values: SecurityFormValues) => void - updateSecurityError?: Error | unknown - // initialTouched is only used for testing the error state of the form. - initialTouched?: FormikTouched + error?: unknown } export const SecurityForm: FC = ({ + disabled, isLoading, onSubmit, - initialValues, - updateSecurityError, - initialTouched, + error, }) => { const form: FormikContextType = useFormik({ - initialValues, + initialValues: { + old_password: "", + password: "", + confirm_password: "", + }, validationSchema, onSubmit, - initialTouched, }) - const getFieldHelpers = getFormHelpers( - form, - updateSecurityError, - ) + const getFieldHelpers = getFormHelpers(form, error) + + if (disabled) { + return ( + + Password changes are only allowed for password based accounts. + + ) + } return ( <> -
- - {Boolean(updateSecurityError) && ( - - )} + + + {Boolean(error) && } = ({ {isLoading ? "" : Language.updatePassword}
- - + + ) } diff --git a/site/src/pages/UserSettingsPage/SecurityPage/SecurityPage.test.tsx b/site/src/pages/UserSettingsPage/SecurityPage/SecurityPage.test.tsx index 9a809d4b57602..eeb9a5eb639b6 100644 --- a/site/src/pages/UserSettingsPage/SecurityPage/SecurityPage.test.tsx +++ b/site/src/pages/UserSettingsPage/SecurityPage/SecurityPage.test.tsx @@ -1,116 +1,144 @@ -import { fireEvent, screen, waitFor } from "@testing-library/react" +import { fireEvent, screen, waitFor, within } from "@testing-library/react" import * as API from "../../../api/api" import * as SecurityForm from "../../../components/SettingsSecurityForm/SettingsSecurityForm" -import { renderWithAuth } from "../../../testHelpers/renderHelpers" +import { + renderWithAuth, + waitForLoaderToBeRemoved, +} from "../../../testHelpers/renderHelpers" import { SecurityPage } from "./SecurityPage" import i18next from "i18next" -import { mockApiError } from "testHelpers/entities" +import { + MockAuthMethodsWithPasswordType, + mockApiError, +} from "testHelpers/entities" +import userEvent from "@testing-library/user-event" const { t } = i18next -const renderPage = () => { - return renderWithAuth() +const renderPage = async () => { + const utils = renderWithAuth() + await waitForLoaderToBeRemoved() + return utils } -const newData = { +const newSecurityFormValues = { old_password: "password1", password: "password2", confirm_password: "password2", } -const fillAndSubmitForm = async () => { - await waitFor(() => screen.findByLabelText("Old Password")) +const fillAndSubmitSecurityForm = () => { fireEvent.change(screen.getByLabelText("Old Password"), { - target: { value: newData.old_password }, + target: { value: newSecurityFormValues.old_password }, }) fireEvent.change(screen.getByLabelText("New Password"), { - target: { value: newData.password }, + target: { value: newSecurityFormValues.password }, }) fireEvent.change(screen.getByLabelText("Confirm Password"), { - target: { value: newData.confirm_password }, + target: { value: newSecurityFormValues.confirm_password }, }) fireEvent.click(screen.getByText(SecurityForm.Language.updatePassword)) } -describe("SecurityPage", () => { - describe("when it is a success", () => { - it("shows the success message", async () => { - jest - .spyOn(API, "updateUserPassword") - .mockImplementationOnce((_userId, _data) => Promise.resolve(undefined)) - const { user } = renderPage() - await fillAndSubmitForm() - - const expectedMessage = t("securityUpdateSuccessMessage", { - ns: "userSettingsPage", - }) - const successMessage = await screen.findByText(expectedMessage) - expect(successMessage).toBeDefined() - expect(API.updateUserPassword).toBeCalledTimes(1) - expect(API.updateUserPassword).toBeCalledWith(user.id, newData) - - await waitFor(() => expect(window.location).toBeAt("/")) - }) +beforeEach(() => { + jest + .spyOn(API, "getAuthMethods") + .mockResolvedValue(MockAuthMethodsWithPasswordType) +}) + +test("update password successfully", async () => { + jest + .spyOn(API, "updateUserPassword") + .mockImplementationOnce((_userId, _data) => Promise.resolve(undefined)) + const { user } = await renderPage() + fillAndSubmitSecurityForm() + + const expectedMessage = t("securityUpdateSuccessMessage", { + ns: "userSettingsPage", }) + const successMessage = await screen.findByText(expectedMessage) + expect(successMessage).toBeDefined() + expect(API.updateUserPassword).toBeCalledTimes(1) + expect(API.updateUserPassword).toBeCalledWith(user.id, newSecurityFormValues) - describe("when the old_password is incorrect", () => { - it("shows an error", async () => { - jest.spyOn(API, "updateUserPassword").mockRejectedValueOnce( - mockApiError({ - message: "Incorrect password.", - validations: [ - { detail: "Incorrect password.", field: "old_password" }, - ], - }), - ) - - const { user } = renderPage() - await fillAndSubmitForm() - - const errorMessage = await screen.findAllByText("Incorrect password.") - expect(errorMessage).toBeDefined() - expect(errorMessage).toHaveLength(2) - expect(API.updateUserPassword).toBeCalledTimes(1) - expect(API.updateUserPassword).toBeCalledWith(user.id, newData) - }) + await waitFor(() => expect(window.location).toBeAt("/")) +}) + +test("update password with incorrect old password", async () => { + jest.spyOn(API, "updateUserPassword").mockRejectedValueOnce( + mockApiError({ + message: "Incorrect password.", + validations: [{ detail: "Incorrect password.", field: "old_password" }], + }), + ) + + const { user } = await renderPage() + fillAndSubmitSecurityForm() + + const errorMessage = await screen.findAllByText("Incorrect password.") + expect(errorMessage).toBeDefined() + expect(errorMessage).toHaveLength(2) + expect(API.updateUserPassword).toBeCalledTimes(1) + expect(API.updateUserPassword).toBeCalledWith(user.id, newSecurityFormValues) +}) + +test("update password with invalid password", async () => { + jest.spyOn(API, "updateUserPassword").mockRejectedValueOnce( + mockApiError({ + message: "Invalid password.", + validations: [{ detail: "Invalid password.", field: "password" }], + }), + ) + + const { user } = await renderPage() + fillAndSubmitSecurityForm() + + const errorMessage = await screen.findAllByText("Invalid password.") + expect(errorMessage).toBeDefined() + expect(errorMessage).toHaveLength(2) + expect(API.updateUserPassword).toBeCalledTimes(1) + expect(API.updateUserPassword).toBeCalledWith(user.id, newSecurityFormValues) +}) + +test("update password when submit returns an unknown error", async () => { + jest.spyOn(API, "updateUserPassword").mockRejectedValueOnce({ + data: "unknown error", }) - describe("when the password is invalid", () => { - it("shows an error", async () => { - jest.spyOn(API, "updateUserPassword").mockRejectedValueOnce( - mockApiError({ - message: "Invalid password.", - validations: [{ detail: "Invalid password.", field: "password" }], - }), - ) - - const { user } = renderPage() - await fillAndSubmitForm() - - const errorMessage = await screen.findAllByText("Invalid password.") - expect(errorMessage).toBeDefined() - expect(errorMessage).toHaveLength(2) - expect(API.updateUserPassword).toBeCalledTimes(1) - expect(API.updateUserPassword).toBeCalledWith(user.id, newData) - }) + const { user } = await renderPage() + fillAndSubmitSecurityForm() + + const errorText = t("warningsAndErrors.somethingWentWrong", { + ns: "common", }) + const errorMessage = await screen.findByText(errorText) + expect(errorMessage).toBeDefined() + expect(API.updateUserPassword).toBeCalledTimes(1) + expect(API.updateUserPassword).toBeCalledWith(user.id, newSecurityFormValues) +}) - describe("when it is an unknown error", () => { - it("shows a generic error message", async () => { - jest.spyOn(API, "updateUserPassword").mockRejectedValueOnce({ - data: "unknown error", - }) - - const { user } = renderPage() - await fillAndSubmitForm() - - const errorText = t("warningsAndErrors.somethingWentWrong", { - ns: "common", - }) - const errorMessage = await screen.findByText(errorText) - expect(errorMessage).toBeDefined() - expect(API.updateUserPassword).toBeCalledTimes(1) - expect(API.updateUserPassword).toBeCalledWith(user.id, newData) +test("change login type to OIDC", async () => { + const convertToOAUTHSpy = jest.spyOn(API, "convertToOAUTH") + const user = userEvent.setup() + const { user: userData } = await renderPage() + + const ssoSection = screen.getByTestId("sso-section") + const githubButton = within(ssoSection).getByText("GitHub", { exact: false }) + await user.click(githubButton) + + const confirmationDialog = await screen.findByTestId("dialog") + const confirmPasswordField = within(confirmationDialog).getByLabelText( + "Confirm your password", + ) + await user.type(confirmPasswordField, "password123") + const updateButton = within(confirmationDialog).getByText("Update") + await user.click(updateButton) + + await waitFor(() => { + expect(convertToOAUTHSpy).toHaveBeenCalledWith({ + password: "password123", + to_login_type: "github", + email: userData.email, }) }) }) diff --git a/site/src/pages/UserSettingsPage/SecurityPage/SecurityPage.tsx b/site/src/pages/UserSettingsPage/SecurityPage/SecurityPage.tsx index 75fa1290636bd..45361684efec9 100644 --- a/site/src/pages/UserSettingsPage/SecurityPage/SecurityPage.tsx +++ b/site/src/pages/UserSettingsPage/SecurityPage/SecurityPage.tsx @@ -1,13 +1,17 @@ import { useMachine } from "@xstate/react" import { useMe } from "hooks/useMe" -import { FC } from "react" +import { ComponentProps, FC } from "react" import { userSecuritySettingsMachine } from "xServices/userSecuritySettings/userSecuritySettingsXService" import { Section } from "../../../components/SettingsLayout/Section" import { SecurityForm } from "../../../components/SettingsSecurityForm/SettingsSecurityForm" - -export const Language = { - title: "Security", -} +import { useQuery } from "@tanstack/react-query" +import { getAuthMethods } from "api/api" +import { + SingleSignOnSection, + useSingleSignOnSection, +} from "./SingleSignOnSection" +import { Loader } from "components/Loader/Loader" +import { Stack } from "components/Stack/Stack" export const SecurityPage: FC = () => { const me = useMe() @@ -20,21 +24,63 @@ export const SecurityPage: FC = () => { }, ) const { error } = securityState.context + const { data: authMethods } = useQuery({ + queryKey: ["authMethods"], + queryFn: getAuthMethods, + }) + const singleSignOnSection = useSingleSignOnSection() + + if (!authMethods) { + return + } + + return ( + { + securitySend({ + type: "UPDATE_SECURITY", + data, + }) + }, + }, + }} + oidc={ + authMethods.convert_to_oidc_enabled + ? { + section: { + authMethods, + ...singleSignOnSection, + }, + } + : undefined + } + /> + ) +} +export const SecurityPageView = ({ + security, + oidc, +}: { + security: { + form: ComponentProps + } + oidc?: { + section: ComponentProps + } +}) => { return ( -
- { - securitySend({ - type: "UPDATE_SECURITY", - data, - }) - }} - /> -
+ +
+ +
+ {oidc && } + ) } diff --git a/site/src/pages/UserSettingsPage/SecurityPage/SecurityPageView.stories.tsx b/site/src/pages/UserSettingsPage/SecurityPage/SecurityPageView.stories.tsx new file mode 100644 index 0000000000000..09a34d5ac6f84 --- /dev/null +++ b/site/src/pages/UserSettingsPage/SecurityPage/SecurityPageView.stories.tsx @@ -0,0 +1,69 @@ +import type { Meta, StoryObj } from "@storybook/react" +import { SecurityPageView } from "./SecurityPage" +import { action } from "@storybook/addon-actions" +import { + MockAuthMethods, + MockAuthMethodsWithPasswordType, +} from "testHelpers/entities" +import { ComponentProps } from "react" +import set from "lodash/fp/set" + +const defaultArgs: ComponentProps = { + security: { + form: { + disabled: false, + error: undefined, + isLoading: false, + onSubmit: action("onSubmit"), + }, + }, + oidc: { + section: { + authMethods: MockAuthMethods, + closeConfirmation: action("closeConfirmation"), + confirm: action("confirm"), + error: undefined, + isConfirming: false, + isUpdating: false, + openConfirmation: action("openConfirmation"), + }, + }, +} + +const meta: Meta = { + title: "pages/SecurityPageView", + component: SecurityPageView, + args: defaultArgs, +} + +export default meta +type Story = StoryObj + +export const UsingOIDC: Story = {} + +export const NoOIDCAvailable: Story = { + args: { + ...defaultArgs, + oidc: undefined, + }, +} + +export const UserLoginTypeIsPassword: Story = { + args: set( + "oidc.section.authMethods", + MockAuthMethodsWithPasswordType, + defaultArgs, + ), +} + +export const ConfirmingOIDCConversion: Story = { + args: set( + "oidc.section", + { + ...defaultArgs.oidc?.section, + authMethods: MockAuthMethodsWithPasswordType, + isConfirming: true, + }, + defaultArgs, + ), +} diff --git a/site/src/pages/UserSettingsPage/SecurityPage/SingleSignOnSection.tsx b/site/src/pages/UserSettingsPage/SecurityPage/SingleSignOnSection.tsx new file mode 100644 index 0000000000000..9e10c328ba183 --- /dev/null +++ b/site/src/pages/UserSettingsPage/SecurityPage/SingleSignOnSection.tsx @@ -0,0 +1,259 @@ +import { useState } from "react" +import { Section } from "../../../components/SettingsLayout/Section" +import { useMe } from "hooks/useMe" +import TextField from "@mui/material/TextField" +import Box from "@mui/material/Box" +import GitHubIcon from "@mui/icons-material/GitHub" +import KeyIcon from "@mui/icons-material/VpnKey" +import Button from "@mui/material/Button" +import { useLocation } from "react-router-dom" +import { retrieveRedirect } from "utils/redirect" +import Typography from "@mui/material/Typography" +import { convertToOAUTH } from "api/api" +import { AuthMethods, LoginType } from "api/typesGenerated" +import Skeleton from "@mui/material/Skeleton" +import { Stack } from "components/Stack/Stack" +import { useMutation } from "@tanstack/react-query" +import { ConfirmDialog } from "components/Dialogs/ConfirmDialog/ConfirmDialog" +import { getErrorMessage } from "api/errors" +import CheckCircleOutlined from "@mui/icons-material/CheckCircleOutlined" + +type LoginTypeConfirmation = + | { + open: false + selectedType: undefined + } + | { + open: true + selectedType: LoginType + } + +export const useSingleSignOnSection = () => { + const me = useMe() + const location = useLocation() + const redirectTo = retrieveRedirect(location.search) + const [loginTypeConfirmation, setLoginTypeConfirmation] = + useState({ open: false, selectedType: undefined }) + + const mutation = useMutation(convertToOAUTH, { + onSuccess: (data) => { + window.location.href = `/api/v2/users/oidc/callback?oidc_merge_state=${ + data.state_string + }&redirect=${encodeURIComponent(redirectTo)}` + }, + }) + + const openConfirmation = (selectedType: LoginType) => { + setLoginTypeConfirmation({ open: true, selectedType }) + } + + const closeConfirmation = () => { + setLoginTypeConfirmation({ open: false, selectedType: undefined }) + mutation.reset() + } + + const confirm = (password: string) => { + if (!loginTypeConfirmation.selectedType) { + throw new Error("No login type selected") + } + mutation.mutate({ + to_login_type: loginTypeConfirmation.selectedType, + email: me.email, + password, + }) + } + + return { + openConfirmation, + closeConfirmation, + confirm, + // We still want to show it loading when it is success so the modal does not + // change until the redirect + isUpdating: mutation.isLoading || mutation.isSuccess, + isConfirming: loginTypeConfirmation.open, + error: mutation.error, + } +} + +type SingleSignOnSectionProps = ReturnType & { + authMethods: AuthMethods +} + +export const SingleSignOnSection = ({ + authMethods, + openConfirmation, + closeConfirmation, + confirm, + isUpdating, + isConfirming, + error, +}: SingleSignOnSectionProps) => { + return ( + <> +
+ + {authMethods ? ( + authMethods.me_login_type === "password" ? ( + <> + {authMethods.github.enabled && ( + + )} + {authMethods.oidc.enabled && ( + + )} + + ) : ( + theme.palette.background.paper, + borderRadius: 1, + border: (theme) => `1px solid ${theme.palette.divider}`, + padding: 2, + display: "flex", + gap: 2, + alignItems: "center", + fontSize: 14, + }} + > + theme.palette.success.light, + fontSize: 16, + }} + /> + + Authenticated with{" "} + + {authMethods.me_login_type === "github" + ? "GitHub" + : getOIDCLabel(authMethods)} + + + + {authMethods.me_login_type === "github" ? ( + + ) : ( + + )} + + + ) + ) : ( + + )} + +
+ + + + ) +} + +const OIDCIcon = ({ authMethods }: { authMethods: AuthMethods }) => { + return authMethods.oidc.iconUrl ? ( + + ) : ( + + ) +} + +const getOIDCLabel = (authMethods: AuthMethods) => { + return authMethods.oidc.signInText || "OpenID Connect" +} + +const ConfirmLoginTypeChangeModal = ({ + open, + loading, + error, + onClose, + onConfirm, +}: { + open: boolean + loading: boolean + error: unknown + onClose: () => void + onConfirm: (password: string) => void +}) => { + const [password, setPassword] = useState("") + + const handleConfirm = () => { + onConfirm(password) + } + + return ( + { + onClose() + }} + onConfirm={handleConfirm} + hideCancel={false} + cancelText="Cancel" + confirmText="Update" + title="Change login type" + confirmLoading={loading} + description={ + + + After changing your login type, you will not be able to change it + again. Are you sure you want to proceed and change your login type? + + { + if (event.key === "Enter") { + handleConfirm() + } + }} + error={Boolean(error)} + helperText={ + error + ? getErrorMessage(error, "Your password is incorrect") + : undefined + } + name="confirm-password" + id="confirm-password" + value={password} + onChange={(e) => setPassword(e.currentTarget.value)} + label="Confirm your password" + type="password" + /> + + } + /> + ) +} diff --git a/site/src/testHelpers/entities.ts b/site/src/testHelpers/entities.ts index 19db62cfec90f..870ddcf077b11 100644 --- a/site/src/testHelpers/entities.ts +++ b/site/src/testHelpers/entities.ts @@ -1021,6 +1021,14 @@ export const MockAuthMethods: TypesGen.AuthMethods = { password: { enabled: true }, github: { enabled: false }, oidc: { enabled: false, signInText: "", iconUrl: "" }, + convert_to_oidc_enabled: true, +} + +export const MockAuthMethodsWithPasswordType: TypesGen.AuthMethods = { + ...MockAuthMethods, + me_login_type: "password", + github: { enabled: true }, + oidc: { enabled: true, signInText: "", iconUrl: "" }, } export const MockGitSSHKey: TypesGen.GitSSHKey = { From 68977f5d0b089a770f11cac6c1be1c239c3fa92c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 28 Jun 2023 12:36:21 -0400 Subject: [PATCH 37/58] create a new route for user login type --- coderd/coderd.go | 5 +---- coderd/userauth.go | 16 +--------------- coderd/users.go | 31 +++++++++++++++++++++++++++++++ codersdk/users.go | 15 ++++++++------- 4 files changed, 41 insertions(+), 26 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 1de880ff5048f..48663ef21e83a 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -593,10 +593,6 @@ func New(options *Options) *API { r.Get("/first", api.firstUser) r.Post("/first", api.postFirstUser) r.Route("/authmethods", func(r chi.Router) { - // The API Key allows this method to return the auth method - // for the logged-in user. This information is useful for the - // caller. If not authenticated, this information is omitted. - r.Use(apiKeyMiddlewareOptional) r.Get("/", api.userAuthMethods) }) @@ -642,6 +638,7 @@ func New(options *Options) *API { r.Use(httpmw.ExtractUserParam(options.Database, false)) r.Delete("/", api.deleteUser) r.Get("/", api.userByName) + r.Get("/login-type", api.userLoginType) r.Put("/profile", api.putUserProfile) r.Route("/status", func(r chi.Router) { r.Put("/suspend", api.putSuspendUserAccount()) diff --git a/coderd/userauth.go b/coderd/userauth.go index 92e93474a8b51..edafeaa63c476 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -407,19 +407,6 @@ type GithubOAuth2Config struct { // @Success 200 {object} codersdk.AuthMethods // @Router /users/authmethods [get] func (api *API) userAuthMethods(rw http.ResponseWriter, r *http.Request) { - key, ok := httpmw.APIKeyOptional(r) - var currentUserLoginType codersdk.LoginType - if ok { - user, err := api.Database.GetUserByID(r.Context(), key.UserID) - if err != nil { - httpapi.Write(r.Context(), rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error.", - Detail: err.Error(), - }) - return - } - currentUserLoginType = codersdk.LoginType(user.LoginType) - } var signInText string var iconURL string @@ -431,8 +418,7 @@ func (api *API) userAuthMethods(rw http.ResponseWriter, r *http.Request) { } httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.AuthMethods{ - UserAuthenticationType: currentUserLoginType, - ConvertToOIDCEnabled: api.Options.DeploymentValues.EnableOauthAccountConversion.Value(), + ConvertToOIDCEnabled: api.Options.DeploymentValues.EnableOauthAccountConversion.Value(), Password: codersdk.AuthMethod{ Enabled: !api.DeploymentValues.DisablePasswordAuth.Value(), }, diff --git a/coderd/users.go b/coderd/users.go index f9c55380c1dc0..12d7ab1bcac9d 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -501,6 +501,37 @@ func (api *API) userByName(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusOK, db2sdk.User(user, organizationIDs)) } +// Returns the user's login type. This only works if the api key for authorization +// and the requested user match. Eg: 'me' +// +// @Summary Get user login type +// @ID get-user-login-type +// @Security CoderSessionToken +// @Produce json +// @Tags Users +// @Param user path string true "User ID, name, or me" +// @Success 200 {object} codersdk.UserAuth +// @Router /users/{user}/login-type [get] +func (api *API) userLoginType(rw http.ResponseWriter, r *http.Request) { + var ( + ctx = r.Context() + user = httpmw.UserParam(r) + key = httpmw.APIKey(r) + ) + + if key.UserID != user.ID { + // Currently this is only valid for querying yourself. + httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ + Message: "You are not authorized to view this user's login type.", + }) + return + } + + httpapi.Write(ctx, rw, http.StatusOK, codersdk.UserAuth{ + LoginType: codersdk.LoginType(user.LoginType), + }) +} + // @Summary Update user profile // @ID update-user-profile // @Security CoderSessionToken diff --git a/codersdk/users.go b/codersdk/users.go index 497db3dc659d2..6e29e71d5caac 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -123,19 +123,20 @@ type CreateOrganizationRequest struct { // AuthMethods contains authentication method information like whether they are enabled or not or custom text, etc. type AuthMethods struct { - // UserAuthenticationType returns the authentication method for the given - // caller if the request is an authenticated request. Otherwise it is empty. - UserAuthenticationType LoginType `json:"me_login_type,omitempty"` - ConvertToOIDCEnabled bool `json:"convert_to_oidc_enabled"` - Password AuthMethod `json:"password"` - Github AuthMethod `json:"github"` - OIDC OIDCAuthMethod `json:"oidc"` + ConvertToOIDCEnabled bool `json:"convert_to_oidc_enabled"` + Password AuthMethod `json:"password"` + Github AuthMethod `json:"github"` + OIDC OIDCAuthMethod `json:"oidc"` } type AuthMethod struct { Enabled bool `json:"enabled"` } +type UserAuth struct { + LoginType LoginType `json:"login_type"` +} + type OIDCAuthMethod struct { AuthMethod SignInText string `json:"signInText"` From c8905b651d512bcfa441b63a9299f233f2b53903 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 28 Jun 2023 13:38:22 -0400 Subject: [PATCH 38/58] New route for login type --- coderd/apidoc/docs.go | 50 ++++++++++++++++--- coderd/apidoc/swagger.json | 46 ++++++++++++++--- coderd/users.go | 4 +- codersdk/users.go | 2 +- docs/api/schemas.md | 28 ++++++++--- docs/api/users.md | 38 +++++++++++++- site/src/api/api.ts | 7 +++ site/src/api/typesGenerated.ts | 6 ++- .../SecurityPage/SecurityPage.tsx | 11 ++-- .../SecurityPage/SecurityPageView.stories.tsx | 3 ++ .../SecurityPage/SingleSignOnSection.tsx | 12 +++-- site/src/testHelpers/entities.ts | 1 - 12 files changed, 170 insertions(+), 38 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 00aa60296e174..1205b1465cfa9 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -3527,6 +3527,40 @@ const docTemplate = `{ } } }, + "/users/{user}/login-type": { + "get": { + "security": [ + { + "CoderSessionToken": [] + } + ], + "produces": [ + "application/json" + ], + "tags": [ + "Users" + ], + "summary": "Get user login type", + "operationId": "get-user-login-type", + "parameters": [ + { + "type": "string", + "description": "User ID, name, or me", + "name": "user", + "in": "path", + "required": true + } + ], + "responses": { + "200": { + "description": "OK", + "schema": { + "$ref": "#/definitions/codersdk.UserLoginType" + } + } + } + } + }, "/users/{user}/organizations": { "get": { "security": [ @@ -6551,14 +6585,6 @@ const docTemplate = `{ "github": { "$ref": "#/definitions/codersdk.AuthMethod" }, - "me_login_type": { - "description": "UserAuthenticationType returns the authentication method for the given\ncaller if the request is an authenticated request. Otherwise it is empty.", - "allOf": [ - { - "$ref": "#/definitions/codersdk.LoginType" - } - ] - }, "oidc": { "$ref": "#/definitions/codersdk.OIDCAuthMethod" }, @@ -9221,6 +9247,14 @@ const docTemplate = `{ } } }, + "codersdk.UserLoginType": { + "type": "object", + "properties": { + "login_type": { + "$ref": "#/definitions/codersdk.LoginType" + } + } + }, "codersdk.UserStatus": { "type": "string", "enum": [ diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 3100bd8748620..9d6e82a8435c4 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -3103,6 +3103,36 @@ } } }, + "/users/{user}/login-type": { + "get": { + "security": [ + { + "CoderSessionToken": [] + } + ], + "produces": ["application/json"], + "tags": ["Users"], + "summary": "Get user login type", + "operationId": "get-user-login-type", + "parameters": [ + { + "type": "string", + "description": "User ID, name, or me", + "name": "user", + "in": "path", + "required": true + } + ], + "responses": { + "200": { + "description": "OK", + "schema": { + "$ref": "#/definitions/codersdk.UserLoginType" + } + } + } + } + }, "/users/{user}/organizations": { "get": { "security": [ @@ -5830,14 +5860,6 @@ "github": { "$ref": "#/definitions/codersdk.AuthMethod" }, - "me_login_type": { - "description": "UserAuthenticationType returns the authentication method for the given\ncaller if the request is an authenticated request. Otherwise it is empty.", - "allOf": [ - { - "$ref": "#/definitions/codersdk.LoginType" - } - ] - }, "oidc": { "$ref": "#/definitions/codersdk.OIDCAuthMethod" }, @@ -8315,6 +8337,14 @@ } } }, + "codersdk.UserLoginType": { + "type": "object", + "properties": { + "login_type": { + "$ref": "#/definitions/codersdk.LoginType" + } + } + }, "codersdk.UserStatus": { "type": "string", "enum": ["active", "suspended"], diff --git a/coderd/users.go b/coderd/users.go index 12d7ab1bcac9d..fbf0386d6989e 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -510,7 +510,7 @@ func (api *API) userByName(rw http.ResponseWriter, r *http.Request) { // @Produce json // @Tags Users // @Param user path string true "User ID, name, or me" -// @Success 200 {object} codersdk.UserAuth +// @Success 200 {object} codersdk.UserLoginType // @Router /users/{user}/login-type [get] func (api *API) userLoginType(rw http.ResponseWriter, r *http.Request) { var ( @@ -527,7 +527,7 @@ func (api *API) userLoginType(rw http.ResponseWriter, r *http.Request) { return } - httpapi.Write(ctx, rw, http.StatusOK, codersdk.UserAuth{ + httpapi.Write(ctx, rw, http.StatusOK, codersdk.UserLoginType{ LoginType: codersdk.LoginType(user.LoginType), }) } diff --git a/codersdk/users.go b/codersdk/users.go index 6e29e71d5caac..b8039efdc6182 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -133,7 +133,7 @@ type AuthMethod struct { Enabled bool `json:"enabled"` } -type UserAuth struct { +type UserLoginType struct { LoginType LoginType `json:"login_type"` } diff --git a/docs/api/schemas.md b/docs/api/schemas.md index f27373f6bf7a6..a02244ec7b6c7 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -1113,7 +1113,6 @@ "github": { "enabled": true }, - "me_login_type": "password", "oidc": { "enabled": true, "iconUrl": "string", @@ -1127,13 +1126,12 @@ ### Properties -| Name | Type | Required | Restrictions | Description | -| ------------------------- | -------------------------------------------------- | -------- | ------------ | --------------------------------------------------------------------------------------------------------------------------------------- | -| `convert_to_oidc_enabled` | boolean | false | | | -| `github` | [codersdk.AuthMethod](#codersdkauthmethod) | false | | | -| `me_login_type` | [codersdk.LoginType](#codersdklogintype) | false | | Me login type returns the authentication method for the given caller if the request is an authenticated request. Otherwise it is empty. | -| `oidc` | [codersdk.OIDCAuthMethod](#codersdkoidcauthmethod) | false | | | -| `password` | [codersdk.AuthMethod](#codersdkauthmethod) | false | | | +| Name | Type | Required | Restrictions | Description | +| ------------------------- | -------------------------------------------------- | -------- | ------------ | ----------- | +| `convert_to_oidc_enabled` | boolean | false | | | +| `github` | [codersdk.AuthMethod](#codersdkauthmethod) | false | | | +| `oidc` | [codersdk.OIDCAuthMethod](#codersdkoidcauthmethod) | false | | | +| `password` | [codersdk.AuthMethod](#codersdkauthmethod) | false | | | ## codersdk.AuthorizationCheck @@ -4429,6 +4427,20 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in | `status` | `active` | | `status` | `suspended` | +## codersdk.UserLoginType + +```json +{ + "login_type": "password" +} +``` + +### Properties + +| Name | Type | Required | Restrictions | Description | +| ------------ | ---------------------------------------- | -------- | ------------ | ----------- | +| `login_type` | [codersdk.LoginType](#codersdklogintype) | false | | | + ## codersdk.UserStatus ```json diff --git a/docs/api/users.md b/docs/api/users.md index 667080b581ee4..1206d42c2e260 100644 --- a/docs/api/users.md +++ b/docs/api/users.md @@ -144,7 +144,6 @@ curl -X GET http://coder-server:8080/api/v2/users/authmethods \ "github": { "enabled": true }, - "me_login_type": "password", "oidc": { "enabled": true, "iconUrl": "string", @@ -794,6 +793,43 @@ curl -X DELETE http://coder-server:8080/api/v2/users/{user}/keys/{keyid} \ To perform this operation, you must be authenticated. [Learn more](authentication.md). +## Get user login type + +### Code samples + +```shell +# Example request using curl +curl -X GET http://coder-server:8080/api/v2/users/{user}/login-type \ + -H 'Accept: application/json' \ + -H 'Coder-Session-Token: API_KEY' +``` + +`GET /users/{user}/login-type` + +### Parameters + +| Name | In | Type | Required | Description | +| ------ | ---- | ------ | -------- | -------------------- | +| `user` | path | string | true | User ID, name, or me | + +### Example responses + +> 200 Response + +```json +{ + "login_type": "password" +} +``` + +### Responses + +| Status | Meaning | Description | Schema | +| ------ | ------------------------------------------------------- | ----------- | ---------------------------------------------------------- | +| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | OK | [codersdk.UserLoginType](schemas.md#codersdkuserlogintype) | + +To perform this operation, you must be authenticated. [Learn more](authentication.md). + ## Get organizations by user ### Code samples diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 0dff38cff4786..7f5f50428bd14 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -142,6 +142,13 @@ export const getAuthMethods = async (): Promise => { return response.data } +export const getUserLoginType = async (): Promise => { + const response = await axios.get( + "/api/v2/users/me/login-type", + ) + return response.data +} + export const checkAuthorization = async ( params: TypesGen.AuthorizationRequest, ): Promise => { diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index e6fb8c53aadeb..eb2948f936d1f 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -104,7 +104,6 @@ export interface AuthMethod { // From codersdk/users.go export interface AuthMethods { - readonly me_login_type?: LoginType readonly convert_to_oidc_enabled: boolean readonly password: AuthMethod readonly github: AuthMethod @@ -1048,6 +1047,11 @@ export interface User { readonly avatar_url: string } +// From codersdk/users.go +export interface UserLoginType { + readonly login_type: LoginType +} + // From codersdk/users.go export interface UserRoles { readonly roles: string[] diff --git a/site/src/pages/UserSettingsPage/SecurityPage/SecurityPage.tsx b/site/src/pages/UserSettingsPage/SecurityPage/SecurityPage.tsx index 45361684efec9..6a0e9853f6e30 100644 --- a/site/src/pages/UserSettingsPage/SecurityPage/SecurityPage.tsx +++ b/site/src/pages/UserSettingsPage/SecurityPage/SecurityPage.tsx @@ -5,7 +5,7 @@ import { userSecuritySettingsMachine } from "xServices/userSecuritySettings/user import { Section } from "../../../components/SettingsLayout/Section" import { SecurityForm } from "../../../components/SettingsSecurityForm/SettingsSecurityForm" import { useQuery } from "@tanstack/react-query" -import { getAuthMethods } from "api/api" +import { getAuthMethods, getUserLoginType } from "api/api" import { SingleSignOnSection, useSingleSignOnSection, @@ -28,9 +28,13 @@ export const SecurityPage: FC = () => { queryKey: ["authMethods"], queryFn: getAuthMethods, }) + const { data: userLoginType } = useQuery({ + queryKey: ["loginType"], + queryFn: getUserLoginType, + }) const singleSignOnSection = useSingleSignOnSection() - if (!authMethods) { + if (!authMethods || !userLoginType) { return } @@ -38,7 +42,7 @@ export const SecurityPage: FC = () => { { @@ -54,6 +58,7 @@ export const SecurityPage: FC = () => { ? { section: { authMethods, + userLoginType, ...singleSignOnSection, }, } diff --git a/site/src/pages/UserSettingsPage/SecurityPage/SecurityPageView.stories.tsx b/site/src/pages/UserSettingsPage/SecurityPage/SecurityPageView.stories.tsx index 09a34d5ac6f84..f0f71abf2f170 100644 --- a/site/src/pages/UserSettingsPage/SecurityPage/SecurityPageView.stories.tsx +++ b/site/src/pages/UserSettingsPage/SecurityPage/SecurityPageView.stories.tsx @@ -19,6 +19,9 @@ const defaultArgs: ComponentProps = { }, oidc: { section: { + userLoginType: { + login_type: "password", + }, authMethods: MockAuthMethods, closeConfirmation: action("closeConfirmation"), confirm: action("confirm"), diff --git a/site/src/pages/UserSettingsPage/SecurityPage/SingleSignOnSection.tsx b/site/src/pages/UserSettingsPage/SecurityPage/SingleSignOnSection.tsx index 9e10c328ba183..f89828cc59192 100644 --- a/site/src/pages/UserSettingsPage/SecurityPage/SingleSignOnSection.tsx +++ b/site/src/pages/UserSettingsPage/SecurityPage/SingleSignOnSection.tsx @@ -10,7 +10,7 @@ import { useLocation } from "react-router-dom" import { retrieveRedirect } from "utils/redirect" import Typography from "@mui/material/Typography" import { convertToOAUTH } from "api/api" -import { AuthMethods, LoginType } from "api/typesGenerated" +import { AuthMethods, LoginType, UserLoginType } from "api/typesGenerated" import Skeleton from "@mui/material/Skeleton" import { Stack } from "components/Stack/Stack" import { useMutation } from "@tanstack/react-query" @@ -77,10 +77,12 @@ export const useSingleSignOnSection = () => { type SingleSignOnSectionProps = ReturnType & { authMethods: AuthMethods + userLoginType: UserLoginType } export const SingleSignOnSection = ({ authMethods, + userLoginType, openConfirmation, closeConfirmation, confirm, @@ -96,8 +98,8 @@ export const SingleSignOnSection = ({ description="Authenticate in Coder using one-click" > - {authMethods ? ( - authMethods.me_login_type === "password" ? ( + {authMethods && userLoginType ? ( + userLoginType.login_type === "password" ? ( <> {authMethods.github.enabled && (