From c0bcc50b28fc89e5c48f6e515ce249243ba459b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Tue, 23 May 2023 15:56:34 +0100 Subject: [PATCH] fix: add confirmation to disable password login (#3829) https://linear.app/unleash/issue/2-1071/prevent-users-from-disabling-password-authentication-when-there-are-no Improves the behavior of disabling password based login by adding some relevant information and a confirmation dialog with a warning. This felt better than trying to disable the toggle, by still allowing the end users to make the decision, except now it should be a properly informed decision with confirmation. ![image](https://github.com/Unleash/unleash/assets/14320932/2ca754d8-cfa2-4fda-984d-0c34b89750f3) - **Password based administrators**: Admin accounts that have a password set; - **Other administrators**: Other admin users that do not have a password. May be SSO, but may also be users that did not set a password yet; - **Admin service accounts**: Service accounts that have the admin root role. Depending on how you're using the SA this may not necessarily mean locking yourself out of an admin account, especially if you secured its token beforehand; - **Admin API tokens**: Similar to the above. If you secured an admin API token beforehand, you still have access to all features through the API; Each one of them link to the respective page inside Unleash (e.g. users page, service accounts page, tokens page...); If you try to disable and press "save", and only in that scenario, you are presented with the following confirmation dialog: ![image](https://github.com/Unleash/unleash/assets/14320932/5ad6d105-ad47-4d31-a1df-04737aed4e00) --- .../admin/auth/OidcAuth/OidcAuth.tsx | 2 +- .../admin/auth/PasswordAuth/PasswordAuth.tsx | 49 ++++++++++++++++- .../auth/PasswordAuth/PasswordAuthDialog.tsx | 53 +++++++++++++++++++ .../admin/auth/SamlAuth/SamlAuth.tsx | 2 +- .../getters/useAdminCount/useAdminCount.ts | 29 ++++++++++ src/lib/db/account-store.ts | 28 ++++++++++ src/lib/openapi/index.ts | 2 + src/lib/openapi/spec/admin-count-schema.ts | 28 ++++++++++ src/lib/openapi/spec/index.ts | 1 + src/lib/routes/admin-api/user-admin.ts | 35 ++++++++++++ src/lib/services/account-service.ts | 5 ++ src/lib/types/stores/account-store.ts | 7 +++ .../__snapshots__/openapi.e2e.test.ts.snap | 44 +++++++++++++++ src/test/fixtures/fake-account-store.ts | 5 ++ 14 files changed, 287 insertions(+), 3 deletions(-) create mode 100644 frontend/src/component/admin/auth/PasswordAuth/PasswordAuthDialog.tsx create mode 100644 frontend/src/hooks/api/getters/useAdminCount/useAdminCount.ts create mode 100644 src/lib/openapi/spec/admin-count-schema.ts diff --git a/frontend/src/component/admin/auth/OidcAuth/OidcAuth.tsx b/frontend/src/component/admin/auth/OidcAuth/OidcAuth.tsx index 1e8bb0d494..7ed804466d 100644 --- a/frontend/src/component/admin/auth/OidcAuth/OidcAuth.tsx +++ b/frontend/src/component/admin/auth/OidcAuth/OidcAuth.tsx @@ -94,7 +94,7 @@ export const OidcAuth = () => { return ( - + Please read the{' '} diff --git a/frontend/src/component/admin/auth/PasswordAuth/PasswordAuth.tsx b/frontend/src/component/admin/auth/PasswordAuth/PasswordAuth.tsx index 2aef86c5cd..07361bdd7c 100644 --- a/frontend/src/component/admin/auth/PasswordAuth/PasswordAuth.tsx +++ b/frontend/src/component/admin/auth/PasswordAuth/PasswordAuth.tsx @@ -10,15 +10,22 @@ import useAuthSettingsApi, { } from 'hooks/api/actions/useAuthSettingsApi/useAuthSettingsApi'; import useToast from 'hooks/useToast'; import { formatUnknownError } from 'utils/formatUnknownError'; +import { useAdminCount } from 'hooks/api/getters/useAdminCount/useAdminCount'; +import { Link } from 'react-router-dom'; +import { useApiTokens } from 'hooks/api/getters/useApiTokens/useApiTokens'; +import { PasswordAuthDialog } from './PasswordAuthDialog'; export const PasswordAuth = () => { const { setToastData, setToastApiError } = useToast(); - const { config } = useAuthSettings('simple'); + const { config, refetch } = useAuthSettings('simple'); const [disablePasswordAuth, setDisablePasswordAuth] = useState(false); const { updateSettings, errors, loading } = useAuthSettingsApi('simple'); const { hasAccess } = useContext(AccessContext); + const [confirmationOpen, setConfirmationOpen] = useState(false); + const { data: adminCount } = useAdminCount(); + const { tokens } = useApiTokens(); useEffect(() => { setDisablePasswordAuth(!!config.disabled); @@ -39,11 +46,20 @@ export const PasswordAuth = () => { const onSubmit = async (event: React.SyntheticEvent) => { event.preventDefault(); + if (!config.disabled && disablePasswordAuth) { + setConfirmationOpen(true); + } else { + onConfirm(); + } + }; + + const onConfirm = async () => { try { const settings: ISimpleAuthSettings = { disabled: disablePasswordAuth, }; await updateSettings(settings); + refetch(); setToastData({ title: 'Successfully saved', text: 'Password authentication settings stored.', @@ -56,9 +72,30 @@ export const PasswordAuth = () => { setDisablePasswordAuth(config.disabled); } }; + return (
+ + Overview of administrators on your Unleash instance: +
+
+ Password based administrators: {' '} + {adminCount?.password} +
+ Other administrators: {' '} + {adminCount?.noPassword} +
+ Admin service accounts: {' '} + + {adminCount?.service} + +
+ Admin API tokens: {' '} + + {tokens.filter(({ type }) => type === 'admin').length} + + Password based login @@ -97,6 +134,16 @@ export const PasswordAuth = () => {

+ { + setConfirmationOpen(false); + onConfirm(); + }} + adminCount={adminCount!} + tokens={tokens} + />
); diff --git a/frontend/src/component/admin/auth/PasswordAuth/PasswordAuthDialog.tsx b/frontend/src/component/admin/auth/PasswordAuth/PasswordAuthDialog.tsx new file mode 100644 index 0000000000..99544c4526 --- /dev/null +++ b/frontend/src/component/admin/auth/PasswordAuth/PasswordAuthDialog.tsx @@ -0,0 +1,53 @@ +import { Alert, Typography } from '@mui/material'; +import { Dialogue } from 'component/common/Dialogue/Dialogue'; +import { IAdminCount } from 'hooks/api/getters/useAdminCount/useAdminCount'; +import { IApiToken } from 'hooks/api/getters/useApiTokens/useApiTokens'; + +interface IPasswordAuthDialogProps { + open: boolean; + setOpen: (open: boolean) => void; + onClick: () => void; + adminCount: IAdminCount; + tokens: IApiToken[]; +} + +export const PasswordAuthDialog = ({ + open, + setOpen, + onClick, + adminCount, + tokens, +}: IPasswordAuthDialogProps) => ( + { + setOpen(false); + }} + onClick={onClick} + title="Disable password based login?" + primaryButtonText="Disable password based login" + secondaryButtonText="Cancel" + > + + Warning! Disabling password based login may lock + you out of the system permanently if you do not have any alternative + admin credentials (such as an admin SSO account or admin API token) + secured beforehand. +
+
+ Password based administrators: {' '} + {adminCount?.password} +
+ Other administrators: {adminCount?.noPassword} +
+ Admin service accounts: {adminCount?.service} +
+ Admin API tokens: {' '} + {tokens.filter(({ type }) => type === 'admin').length} + + + You are about to disable password based login. Are you sure you want + to proceed? + + +); diff --git a/frontend/src/component/admin/auth/SamlAuth/SamlAuth.tsx b/frontend/src/component/admin/auth/SamlAuth/SamlAuth.tsx index dcaaa5c6e5..a7a4a8a58d 100644 --- a/frontend/src/component/admin/auth/SamlAuth/SamlAuth.tsx +++ b/frontend/src/component/admin/auth/SamlAuth/SamlAuth.tsx @@ -85,7 +85,7 @@ export const SamlAuth = () => { return ( - + Please read the{' '} diff --git a/frontend/src/hooks/api/getters/useAdminCount/useAdminCount.ts b/frontend/src/hooks/api/getters/useAdminCount/useAdminCount.ts new file mode 100644 index 0000000000..75bb1f9d75 --- /dev/null +++ b/frontend/src/hooks/api/getters/useAdminCount/useAdminCount.ts @@ -0,0 +1,29 @@ +import useSWR from 'swr'; +import { formatApiPath } from 'utils/formatPath'; +import handleErrorResponses from '../httpErrorResponseHandler'; + +export interface IAdminCount { + password: number; + noPassword: number; + service: number; +} + +export const useAdminCount = () => { + const { data, error, mutate } = useSWR( + formatApiPath(`api/admin/user-admin/admin-count`), + fetcher + ); + + return { + data, + loading: !error && !data, + refetch: () => mutate(), + error, + }; +}; + +const fetcher = (path: string) => { + return fetch(path) + .then(handleErrorResponses('Admin count')) + .then(res => res.json()); +}; diff --git a/src/lib/db/account-store.ts b/src/lib/db/account-store.ts index d8de4dba18..658a459f81 100644 --- a/src/lib/db/account-store.ts +++ b/src/lib/db/account-store.ts @@ -3,6 +3,7 @@ import User from '../types/user'; import NotFoundError from '../error/notfound-error'; import { IUserLookup } from '../types/stores/user-store'; +import { IAdminCount } from '../types/stores/account-store'; import { IAccountStore } from '../types'; import { Db } from './db'; @@ -169,4 +170,31 @@ export class AccountStore implements IAccountStore { this.logger.error('Could not update lastSeen, error: ', err); } } + + async getAdminCount(): Promise { + const adminCount = await this.activeAccounts() + .join('role_user as ru', 'users.id', 'ru.user_id') + .where( + 'ru.role_id', + '=', + this.db.raw('(SELECT id FROM roles WHERE name = ?)', ['Admin']), + ) + .select( + this.db.raw( + 'COUNT(CASE WHEN users.password_hash IS NOT NULL AND users.is_service = false THEN 1 END)::integer AS password', + ), + this.db.raw( + 'COUNT(CASE WHEN users.password_hash IS NULL AND users.is_service = false THEN 1 END)::integer AS no_password', + ), + this.db.raw( + 'COUNT(CASE WHEN users.is_service = true THEN 1 END)::integer AS service', + ), + ); + + return { + password: adminCount[0].password, + noPassword: adminCount[0].no_password, + service: adminCount[0].service, + }; + } } diff --git a/src/lib/openapi/index.ts b/src/lib/openapi/index.ts index ad87046618..b919c91c56 100644 --- a/src/lib/openapi/index.ts +++ b/src/lib/openapi/index.ts @@ -5,6 +5,7 @@ import { addonSchema, addonsSchema, addonTypeSchema, + adminCountSchema, adminFeaturesQuerySchema, apiTokenSchema, apiTokensSchema, @@ -182,6 +183,7 @@ interface OpenAPIV3DocumentWithServers extends OpenAPIV3.Document { // All schemas in `openapi/spec` should be listed here. export const schemas: UnleashSchemas = { + adminCountSchema, adminFeaturesQuerySchema, addonParameterSchema, addonSchema, diff --git a/src/lib/openapi/spec/admin-count-schema.ts b/src/lib/openapi/spec/admin-count-schema.ts new file mode 100644 index 0000000000..7861f37281 --- /dev/null +++ b/src/lib/openapi/spec/admin-count-schema.ts @@ -0,0 +1,28 @@ +import { FromSchema } from 'json-schema-to-ts'; + +export const adminCountSchema = { + $id: '#/components/schemas/adminCountSchema', + type: 'object', + additionalProperties: false, + description: 'Contains total admin counts for an Unleash instance.', + required: ['password', 'noPassword', 'service'], + properties: { + password: { + type: 'number', + description: 'Total number of admins that have a password set.', + }, + noPassword: { + type: 'number', + description: + 'Total number of admins that do not have a password set. May be SSO, but may also be users that did not set a password yet.', + }, + service: { + type: 'number', + description: + 'Total number of service accounts that have the admin root role.', + }, + }, + components: {}, +} as const; + +export type AdminCountSchema = FromSchema; diff --git a/src/lib/openapi/spec/index.ts b/src/lib/openapi/spec/index.ts index 3cb523723f..e5793bcee8 100644 --- a/src/lib/openapi/spec/index.ts +++ b/src/lib/openapi/spec/index.ts @@ -136,3 +136,4 @@ export * from './upsert-segment-schema'; export * from './batch-features-schema'; export * from './token-string-list-schema'; export * from './bulk-toggle-features-schema'; +export * from './admin-count-schema'; diff --git a/src/lib/routes/admin-api/user-admin.ts b/src/lib/routes/admin-api/user-admin.ts index 611041c571..2a8dec5c33 100644 --- a/src/lib/routes/admin-api/user-admin.ts +++ b/src/lib/routes/admin-api/user-admin.ts @@ -41,6 +41,10 @@ import { IGroup } from '../../types/group'; import { IFlagResolver } from '../../types/experimental'; import rateLimit from 'express-rate-limit'; import { minutesToMilliseconds } from 'date-fns'; +import { + AdminCountSchema, + adminCountSchema, +} from '../../openapi/spec/admin-count-schema'; export default class UserAdminController extends Controller { private flagResolver: IFlagResolver; @@ -192,6 +196,22 @@ export default class UserAdminController extends Controller { ], }); + this.route({ + method: 'get', + path: '/admin-count', + handler: this.getAdminCount, + permission: ADMIN, + middleware: [ + openApiService.validPath({ + tags: ['Users'], + operationId: 'getAdminCount', + responses: { + 200: createResponseSchema('adminCountSchema'), + }, + }), + ], + }); + this.route({ method: 'post', path: '', @@ -498,4 +518,19 @@ export default class UserAdminController extends Controller { await this.userService.changePassword(+id, password); res.status(200).send(); } + + async getAdminCount( + req: Request, + res: Response, + ): Promise { + console.log('user-admin controller'); + const adminCount = await this.accountService.getAdminCount(); + + this.openApiService.respondWithValidation( + 200, + res, + adminCountSchema.$id, + adminCount, + ); + } } diff --git a/src/lib/services/account-service.ts b/src/lib/services/account-service.ts index 8204493f99..9dc145c869 100644 --- a/src/lib/services/account-service.ts +++ b/src/lib/services/account-service.ts @@ -5,6 +5,7 @@ import { IAccountStore, IUnleashStores } from '../types/stores'; import { minutesToMilliseconds } from 'date-fns'; import { AccessService } from './access-service'; import { RoleName } from '../types/model'; +import { IAdminCount } from 'lib/types/stores/account-store'; interface IUserWithRole extends IUser { rootRole: number; @@ -52,6 +53,10 @@ export class AccountService { return this.store.getAccountByPersonalAccessToken(secret); } + async getAdminCount(): Promise { + return this.store.getAdminCount(); + } + async updateLastSeen(): Promise { if (this.lastSeenSecrets.size > 0) { const toStore = [...this.lastSeenSecrets]; diff --git a/src/lib/types/stores/account-store.ts b/src/lib/types/stores/account-store.ts index 4501109915..cfb754efac 100644 --- a/src/lib/types/stores/account-store.ts +++ b/src/lib/types/stores/account-store.ts @@ -7,6 +7,12 @@ export interface IUserLookup { email?: string; } +export interface IAdminCount { + password: number; + noPassword: number; + service: number; +} + export interface IAccountStore extends Store { hasAccount(idQuery: IUserLookup): Promise; search(query: string): Promise; @@ -15,4 +21,5 @@ export interface IAccountStore extends Store { count(): Promise; getAccountByPersonalAccessToken(secret: string): Promise; markSeenAt(secrets: string[]): Promise; + getAdminCount(): Promise; } diff --git a/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap b/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap index 081499c35a..1c53163698 100644 --- a/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap +++ b/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap @@ -608,6 +608,30 @@ The provider you choose for your addon dictates what properties the \`parameters ], "type": "object", }, + "adminCountSchema": { + "additionalProperties": false, + "description": "Contains total admin counts for an Unleash instance.", + "properties": { + "noPassword": { + "description": "Total number of admins that do not have a password set. May be SSO, but may also be users that did not set a password yet.", + "type": "number", + }, + "password": { + "description": "Total number of admins that have a password set.", + "type": "number", + }, + "service": { + "description": "Total number of service accounts that have the admin root role.", + "type": "number", + }, + }, + "required": [ + "password", + "noPassword", + "service", + ], + "type": "object", + }, "adminFeaturesQuerySchema": { "additionalProperties": false, "properties": { @@ -12522,6 +12546,26 @@ If the provided project does not exist, the list of events will be empty.", ], }, }, + "/api/admin/user-admin/admin-count": { + "get": { + "operationId": "getAdminCount", + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/adminCountSchema", + }, + }, + }, + "description": "adminCountSchema", + }, + }, + "tags": [ + "Users", + ], + }, + }, "/api/admin/user-admin/reset-password": { "post": { "operationId": "resetUserPassword", diff --git a/src/test/fixtures/fake-account-store.ts b/src/test/fixtures/fake-account-store.ts index 284a797c0c..3b0c99471b 100644 --- a/src/test/fixtures/fake-account-store.ts +++ b/src/test/fixtures/fake-account-store.ts @@ -3,6 +3,7 @@ import { // ICreateUser, IUserLookup, IAccountStore, + IAdminCount, } from '../../lib/types/stores/account-store'; export class FakeAccountStore implements IAccountStore { @@ -93,4 +94,8 @@ export class FakeAccountStore implements IAccountStore { async markSeenAt(secrets: string[]): Promise { throw new Error('Not implemented'); } + + async getAdminCount(): Promise { + throw new Error('Not implemented'); + } }