From 8d9ca0eabf89b6063cd1c2ac6252ee74ae2c04f3 Mon Sep 17 00:00:00 2001 From: jacobshandling <61553566+jacobshandling@users.noreply.github.com> Date: Thu, 30 Jan 2025 09:32:11 -0800 Subject: [PATCH] Require a password when changing a user from SSO to password-based authentication (#25843) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## For #24754 - Backend: - Return an error when a PATCH attempts to update a user's authentication from SSO to password but doesn't include a password - Add checks to integration test. - Frontend: - Form error when attempting to switch a user who is currently SSO-authed to password without a password - Refactor upstream inherited errors to allow for disabling the form submission button when errors are present - Other improvements to user form validation **[UI Demo](https://drive.google.com/file/d/1-BIzCpqu0zjYHf7zxiZL_7kVoE2sLwtx/view?usp=sharing)** **[API Demo](https://drive.google.com/file/d/19lQ7Pvfmq3MwEjHw0_r9IoxVuNaSNwGb/view?usp=sharing)** Screenshot 2025-01-28 at 3 38 11 PM Screenshot 2025-01-28 at 3 39 41 PM - [x] Changes file added for user-visible changes in `changes/` - [x] Added/updated automated tests - [ ] A detailed QA plan exists on the associated ticket (if it isn't there, work with the product group's QA engineer to add it) - [x] Manual QA for all new/changed functionality --------- Co-authored-by: Jacob Shandling --- changes/24754-require-pw-for-pw-auth | 4 ++ .../UsersPage/UsersPage.tsx | 11 +--- .../components/UserForm/UserForm.tests.tsx | 3 +- .../components/UserForm/UserForm.tsx | 61 +++++++++++++++---- .../components/UsersTable/UsersTable.tsx | 13 ++-- frontend/utilities/constants.tsx | 7 --- server/service/integration_core_test.go | 25 ++++++++ server/service/users.go | 3 + 8 files changed, 89 insertions(+), 38 deletions(-) create mode 100644 changes/24754-require-pw-for-pw-auth diff --git a/changes/24754-require-pw-for-pw-auth b/changes/24754-require-pw-for-pw-auth new file mode 100644 index 0000000000..d6228caf8a --- /dev/null +++ b/changes/24754-require-pw-for-pw-auth @@ -0,0 +1,4 @@ +- Update user form validation to require a password be present when switching a user from + SSO to password authentication +- Refactor upstream error logic to allow disabling submit button when form errors are present +- Add similar check for password presence on server, update integration test accordingly diff --git a/frontend/pages/admin/TeamManagementPage/TeamDetailsWrapper/UsersPage/UsersPage.tsx b/frontend/pages/admin/TeamManagementPage/TeamDetailsWrapper/UsersPage/UsersPage.tsx index ca3e1a1731..0a56b4f0c1 100644 --- a/frontend/pages/admin/TeamManagementPage/TeamDetailsWrapper/UsersPage/UsersPage.tsx +++ b/frontend/pages/admin/TeamManagementPage/TeamDetailsWrapper/UsersPage/UsersPage.tsx @@ -13,7 +13,6 @@ import PATHS from "router/paths"; import usersAPI from "services/entities/users"; import inviteAPI from "services/entities/invites"; import teamsAPI, { ILoadTeamsResponse } from "services/entities/teams"; -import { DEFAULT_USER_FORM_ERRORS } from "utilities/constants"; import TableContainer from "components/TableContainer"; import TableDataError from "components/DataError"; @@ -69,12 +68,8 @@ const UsersPage = ({ location, router }: ITeamSubnavProps): JSX.Element => { const [isUpdatingUsers, setIsUpdatingUsers] = useState(false); const [userEditing, setUserEditing] = useState(); const [searchString, setSearchString] = useState(""); - const [addUserErrors, setAddUserErrors] = useState( - DEFAULT_USER_FORM_ERRORS - ); - const [editUserErrors, setEditUserErrors] = useState( - DEFAULT_USER_FORM_ERRORS - ); + const [addUserErrors, setAddUserErrors] = useState({}); + const [editUserErrors, setEditUserErrors] = useState({}); const toggleAddUserModal = useCallback(() => { setShowAddUserModal(!showAddUserModal); @@ -129,7 +124,7 @@ const UsersPage = ({ location, router }: ITeamSubnavProps): JSX.Element => { (user?: IUser) => { setShowEditUserModal(!showEditUserModal); user ? setUserEditing(user) : setUserEditing(undefined); - setEditUserErrors(DEFAULT_USER_FORM_ERRORS); + setEditUserErrors({}); }, [showEditUserModal, setShowEditUserModal, setUserEditing] ); diff --git a/frontend/pages/admin/UserManagementPage/components/UserForm/UserForm.tests.tsx b/frontend/pages/admin/UserManagementPage/components/UserForm/UserForm.tests.tsx index 5c11d2e2ef..6700148b6e 100644 --- a/frontend/pages/admin/UserManagementPage/components/UserForm/UserForm.tests.tsx +++ b/frontend/pages/admin/UserManagementPage/components/UserForm/UserForm.tests.tsx @@ -2,7 +2,6 @@ import React from "react"; import { render, screen } from "@testing-library/react"; import { noop } from "lodash"; import { renderWithSetup, createMockRouter } from "test/test-utils"; -import { DEFAULT_USER_FORM_ERRORS } from "utilities/constants"; import UserForm from "./UserForm"; // Note: Happy path is tested e2e so these integration tests are only edge cases @@ -18,7 +17,7 @@ describe("UserForm - component", () => { canUseSso: false, isNewUser: true, router: createMockRouter(), - ancestorErrors: DEFAULT_USER_FORM_ERRORS, + ancestorErrors: {}, }; it("displays error messages for invalid inputs", async () => { diff --git a/frontend/pages/admin/UserManagementPage/components/UserForm/UserForm.tsx b/frontend/pages/admin/UserManagementPage/components/UserForm/UserForm.tsx index 0f283e948d..cb5dd38f2f 100644 --- a/frontend/pages/admin/UserManagementPage/components/UserForm/UserForm.tsx +++ b/frontend/pages/admin/UserManagementPage/components/UserForm/UserForm.tsx @@ -88,8 +88,9 @@ interface IUserFormProps { const validate = ( formData: IUserFormData, canUseSso: boolean, - isNewUser?: boolean, - isSsoEnabled?: boolean + isNewUser: boolean, + isSsoEnabled: boolean, + initiallyPasswordAuth: boolean ) => { const newErrors: IUserFormErrors = {}; @@ -110,8 +111,13 @@ const validate = ( // force to password auth if SSO is disabled globally but was enabled on the form const isExistingUserForcedToPasswordAuth = !canUseSso && isSsoEnabled; - // password required when creating a user with SSO disabled, though not when inviting a user - if (isNewAdminCreatedUserWithoutSSO || isExistingUserForcedToPasswordAuth) { + // password required when creating a user with SSO disabled and when changing a user from SSO to + // password authentication, though not when inviting a user + if ( + isNewAdminCreatedUserWithoutSSO || + isExistingUserForcedToPasswordAuth || + (!initiallyPasswordAuth && !sso_enabled) + ) { if (password !== null && !validPassword(password)) { newErrors.password = "Password must meet the criteria below"; } @@ -199,13 +205,19 @@ const UserForm = ({ const onInputChange = ({ name, value }: IFormField) => { const newFormData = { ...formData, [name]: value }; setFormData(newFormData); - const newErrs = validate(newFormData, canUseSso, isNewUser, isSsoEnabled); + const newErrs = validate( + newFormData, + canUseSso, + isNewUser, + !!isSsoEnabled, + initiallyPasswordAuth + ); // only set errors that are updates of existing errors // new errors are only set onBlur or submit const errsToSet: Record = {}; Object.keys(formErrors).forEach((k) => { // @ts-ignore - if (formErrors[k] && newErrs[k]) { + if (newErrs[k]) { // @ts-ignore errsToSet[k] = newErrs[k]; } @@ -214,7 +226,15 @@ const UserForm = ({ }; const onInputBlur = () => { - setFormErrors(validate(formData, canUseSso, isNewUser, isSsoEnabled)); + setFormErrors( + validate( + formData, + canUseSso, + isNewUser, + !!isSsoEnabled, + initiallyPasswordAuth + ) + ); }; // Used to show entire dropdown when a dropdown menu is open in scrollable component of a modal @@ -261,10 +281,20 @@ const UserForm = ({ }; const onSsoChange = (value: boolean): void => { - setFormData({ - ...formData, - sso_enabled: value, - }); + const newFormData = { ...formData, sso_enabled: value }; + setFormData(newFormData); + if (value) { + // clears password error when enabling sso, allowing submission even if password is invalid + setFormErrors( + validate( + newFormData, + canUseSso, + isNewUser, + !!isSsoEnabled, + initiallyPasswordAuth + ) + ); + } }; const onSelectedTeamChange = (teams: ITeam[]): void => { @@ -318,7 +348,13 @@ const UserForm = ({ renderFlash("error", `Please select at least one team for this user.`); return; } - const errs = validate(formData, canUseSso, isNewUser, isSsoEnabled); + const errs = validate( + formData, + canUseSso, + isNewUser, + !!isSsoEnabled, + initiallyPasswordAuth + ); if (Object.keys(errs).length > 0) { setFormErrors(errs); return; @@ -699,6 +735,7 @@ const UserForm = ({ className={`${isNewUser ? "add" : "save"}-loading `} isLoading={isUpdatingUsers} + disabled={Object.keys(formErrors).length > 0} > {isNewUser ? "Add" : "Save"} diff --git a/frontend/pages/admin/UserManagementPage/components/UsersTable/UsersTable.tsx b/frontend/pages/admin/UserManagementPage/components/UsersTable/UsersTable.tsx index 2d97a7137f..84f1440b44 100644 --- a/frontend/pages/admin/UserManagementPage/components/UsersTable/UsersTable.tsx +++ b/frontend/pages/admin/UserManagementPage/components/UsersTable/UsersTable.tsx @@ -15,7 +15,6 @@ import teamsAPI, { ILoadTeamsResponse } from "services/entities/teams"; import usersAPI from "services/entities/users"; import invitesAPI from "services/entities/invites"; -import { DEFAULT_USER_FORM_ERRORS } from "utilities/constants"; import TableContainer from "components/TableContainer"; import { ITableQueryData } from "components/TableContainer/TableContainer"; import TableCount from "components/TableContainer/TableCount"; @@ -51,12 +50,8 @@ const UsersTable = ({ router }: IUsersTableProps): JSX.Element => { const [showResetSessionsModal, setShowResetSessionsModal] = useState(false); const [isUpdatingUsers, setIsUpdatingUsers] = useState(false); const [userEditing, setUserEditing] = useState(null); - const [addUserErrors, setAddUserErrors] = useState( - DEFAULT_USER_FORM_ERRORS - ); - const [editUserErrors, setEditUserErrors] = useState( - DEFAULT_USER_FORM_ERRORS - ); + const [addUserErrors, setAddUserErrors] = useState({}); + const [editUserErrors, setEditUserErrors] = useState({}); const [querySearchText, setQuerySearchText] = useState(""); // API CALLS @@ -112,7 +107,7 @@ const UsersTable = ({ router }: IUsersTableProps): JSX.Element => { // clear errors on close if (!showAddUserModal) { - setAddUserErrors(DEFAULT_USER_FORM_ERRORS); + setAddUserErrors({}); } }, [showAddUserModal, setShowAddUserModal]); @@ -128,7 +123,7 @@ const UsersTable = ({ router }: IUsersTableProps): JSX.Element => { (user?: IUser | IInvite) => { setShowEditUserModal(!showEditUserModal); setUserEditing(!showEditUserModal ? user : null); - setEditUserErrors(DEFAULT_USER_FORM_ERRORS); + setEditUserErrors({}); }, [showEditUserModal, setShowEditUserModal, setUserEditing] ); diff --git a/frontend/utilities/constants.tsx b/frontend/utilities/constants.tsx index 5f091b0bfe..874cf8c730 100644 --- a/frontend/utilities/constants.tsx +++ b/frontend/utilities/constants.tsx @@ -377,13 +377,6 @@ export const BATTERY_TOOLTIP: Record = { ), }; -export const DEFAULT_USER_FORM_ERRORS = { - email: null, - name: null, - password: null, - sso_enabled: null, -}; - /** Must pass agent options config as empty object */ export const EMPTY_AGENT_OPTIONS = { config: {}, diff --git a/server/service/integration_core_test.go b/server/service/integration_core_test.go index c7fa9e838b..42e2c3029a 100644 --- a/server/service/integration_core_test.go +++ b/server/service/integration_core_test.go @@ -8429,6 +8429,31 @@ func (s *integrationTestSuite) TestModifyUser() { require.NoError(t, json.NewDecoder(resp.Body).Decode(&loginResp)) resp.Body.Close() require.Equal(t, u.ID, loginResp.User.ID) + + // as an admin, create a new user with SSO authentication enabled + params = fleet.UserPayload{ + Name: ptr.String("moduser1"), + Email: ptr.String("moduser1@example.com"), + SSOInvite: ptr.Bool(true), + GlobalRole: ptr.String(fleet.RoleObserver), + AdminForcedPasswordReset: ptr.Bool(false), + } + s.DoJSON("POST", "/api/latest/fleet/users/admin", params, http.StatusOK, &createResp) + require.NotZero(t, createResp.User.ID) + u = *createResp.User + + // as an admin, try to disable sso for that user without providing a password + res := s.Do("PATCH", fmt.Sprintf("/api/latest/fleet/users/%d", u.ID), fleet.UserPayload{ + SSOEnabled: ptr.Bool(false), + }, http.StatusUnprocessableEntity) + errMsg := extractServerErrorText(res.Body) + require.Contains(t, errMsg, "a new password must be provided when disabling SSO") + + // as an admin, try to disable sso for that user while providing a password + s.Do("PATCH", fmt.Sprintf("/api/latest/fleet/users/%d", u.ID), fleet.UserPayload{ + SSOEnabled: ptr.Bool(false), + NewPassword: ptr.String("Password123#"), + }, http.StatusOK) } func (s *integrationTestSuite) TestGetHostLastOpenedAt() { diff --git a/server/service/users.go b/server/service/users.go index b807266313..ab4da962b8 100644 --- a/server/service/users.go +++ b/server/service/users.go @@ -480,6 +480,9 @@ func (svc *Service) ModifyUser(ctx context.Context, userID uint, p fleet.UserPay } if p.SSOEnabled != nil { + if !*p.SSOEnabled && p.NewPassword == nil { + return nil, fleet.NewInvalidArgumentError("missing password", "a new password must be provided when disabling SSO") + } user.SSOEnabled = *p.SSOEnabled }