Require a password when changing a user from SSO to password-based authentication (#25843)
## 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)** <img width="994" alt="Screenshot 2025-01-28 at 3 38 11 PM" src="https://github.com/user-attachments/assets/304f8def-2656-43f7-97e5-8be1fc679814" /> <img width="660" alt="Screenshot 2025-01-28 at 3 39 41 PM" src="https://github.com/user-attachments/assets/77283520-b313-4743-96df-06c55e573496" /> - [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 <jacob@fleetdm.com>
This commit is contained in:
co-authored by
Jacob Shandling
parent
616ed21a46
commit
8d9ca0eabf
@@ -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
|
||||
@@ -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<IUser>();
|
||||
const [searchString, setSearchString] = useState("");
|
||||
const [addUserErrors, setAddUserErrors] = useState<IUserFormErrors>(
|
||||
DEFAULT_USER_FORM_ERRORS
|
||||
);
|
||||
const [editUserErrors, setEditUserErrors] = useState<IUserFormErrors>(
|
||||
DEFAULT_USER_FORM_ERRORS
|
||||
);
|
||||
const [addUserErrors, setAddUserErrors] = useState<IUserFormErrors>({});
|
||||
const [editUserErrors, setEditUserErrors] = useState<IUserFormErrors>({});
|
||||
|
||||
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]
|
||||
);
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -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<string, string> = {};
|
||||
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"}
|
||||
</Button>
|
||||
|
||||
@@ -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<any>(null);
|
||||
const [addUserErrors, setAddUserErrors] = useState<IUserFormErrors>(
|
||||
DEFAULT_USER_FORM_ERRORS
|
||||
);
|
||||
const [editUserErrors, setEditUserErrors] = useState<IUserFormErrors>(
|
||||
DEFAULT_USER_FORM_ERRORS
|
||||
);
|
||||
const [addUserErrors, setAddUserErrors] = useState<IUserFormErrors>({});
|
||||
const [editUserErrors, setEditUserErrors] = useState<IUserFormErrors>({});
|
||||
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]
|
||||
);
|
||||
|
||||
@@ -377,13 +377,6 @@ export const BATTERY_TOOLTIP: Record<string, string | React.ReactNode> = {
|
||||
),
|
||||
};
|
||||
|
||||
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: {},
|
||||
|
||||
@@ -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() {
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user