diff --git a/api/config.py b/api/config.py index f368d832..36e9478d 100644 --- a/api/config.py +++ b/api/config.py @@ -92,3 +92,6 @@ def default_user_search() -> list[str]: # Require descriptions for apps, groups, and tags REQUIRE_DESCRIPTIONS = os.getenv("REQUIRE_DESCRIPTIONS", "false").lower() == "true" + +# Require reasons globally for create/resolve request flows +REQUIRE_REASONS = os.getenv("REQUIRE_REASONS", "false").lower() == "true" diff --git a/api/views/schemas/access_requests.py b/api/views/schemas/access_requests.py index bd346644..a4b7e100 100644 --- a/api/views/schemas/access_requests.py +++ b/api/views/schemas/access_requests.py @@ -3,11 +3,13 @@ from marshmallow import Schema, ValidationError, fields, post_load, validate +from api.views.schemas.core_schemas import context_aware_reason_field + class CreateAccessRequestSchema(Schema): group_id = fields.String(validate=validate.Length(equal=20), required=True, load_only=True) group_owner = fields.Boolean(load_default=False, load_only=True) - reason = fields.String(validate=validate.Length(max=1024), load_only=True) + reason = context_aware_reason_field(load_only=True) @staticmethod def must_be_in_the_future(data: Optional[datetime]) -> None: @@ -30,7 +32,7 @@ def convert_to_utc(self, item: Dict[str, Any], many: bool, **kwargs: Any) -> Dic class ResolveAccessRequestSchema(Schema): approved = fields.Boolean(required=True, load_only=True) - reason = fields.String(load_only=True, validate=validate.Length(max=1024)) + reason = context_aware_reason_field(load_only=True) @staticmethod def must_be_in_the_future(data: Optional[datetime]) -> None: diff --git a/api/views/schemas/core_schemas.py b/api/views/schemas/core_schemas.py index 31d44e3d..d92e70c0 100644 --- a/api/views/schemas/core_schemas.py +++ b/api/views/schemas/core_schemas.py @@ -76,6 +76,49 @@ def deserialize( return ContextAwareDescriptionField(allow_none=True, load_default="", dump_default="") +def context_aware_reason_field(**kwargs: Any) -> fields.Field: + """ + Returns a context-aware reason field that reads REQUIRE_REASONS + from Flask app config at validation time instead of module import time. + """ + + class ContextAwareReasonField(fields.String): + def deserialize( + self, value: Any, attr: Optional[str] = None, data: Optional[Mapping[str, Any]] = None, **kwargs: Any + ) -> Any: + # Read config at deserialization time (when processing request data) + require_reasons = current_app.config.get("REQUIRE_REASONS", False) + + # Check if field was provided in the input data + field_was_provided = data is not None and attr is not None and attr in data + + # If field wasn't provided and reasons are required, raise error + if not field_was_provided and require_reasons: + raise ValidationError("Reason is required.") + + # If field wasn't provided and reasons are not required, return empty string + if not field_was_provided: + return "" + + # Field was provided, validate it + if isinstance(value, str) and value.strip() == "" and require_reasons: + raise ValidationError("Reason must be between 1 and 1024 characters") + + # Use parent deserialization for type conversion + if value is None or value == "": + return "" if not require_reasons else self.fail("required") + + result = super().deserialize(value, attr, data, **kwargs) + + # Validate length + if result and len(result) > 1024: + raise ValidationError("Reason must be 1024 characters or less") + + return result + + return ContextAwareReasonField(allow_none=True, load_default="", dump_default="", **kwargs) + + # See https://stackoverflow.com/a/58646612 class OktaUserGroupMemberSchema(SQLAlchemyAutoSchema): group = fields.Nested(lambda: PolymorphicGroupSchema) diff --git a/api/views/schemas/role_requests.py b/api/views/schemas/role_requests.py index c471112f..d8b0c398 100644 --- a/api/views/schemas/role_requests.py +++ b/api/views/schemas/role_requests.py @@ -3,12 +3,14 @@ from marshmallow import Schema, ValidationError, fields, post_load, validate +from api.views.schemas.core_schemas import context_aware_reason_field + class CreateRoleRequestSchema(Schema): role_id = fields.String(validate=validate.Length(equal=20), required=True, load_only=True) group_id = fields.String(validate=validate.Length(equal=20), required=True, load_only=True) group_owner = fields.Boolean(load_default=False, load_only=True) - reason = fields.String(validate=validate.Length(max=1024), load_only=True) + reason = context_aware_reason_field(load_only=True) @staticmethod def must_be_in_the_future(data: Optional[datetime]) -> None: @@ -31,7 +33,7 @@ def convert_to_utc(self, item: Dict[str, Any], many: bool, **kwargs: Any) -> Dic class ResolveRoleRequestSchema(Schema): approved = fields.Boolean(required=True, load_only=True) - reason = fields.String(load_only=True, validate=validate.Length(max=1024)) + reason = context_aware_reason_field(load_only=True) @staticmethod def must_be_in_the_future(data: Optional[datetime]) -> None: diff --git a/src/config/accessConfig.ts b/src/config/accessConfig.ts index 4b7dfe19..64eb666c 100644 --- a/src/config/accessConfig.ts +++ b/src/config/accessConfig.ts @@ -13,3 +13,4 @@ export default accessConfig; export const appName = APP_NAME; export const requireDescriptions = REQUIRE_DESCRIPTIONS; +export const requireReasons = REQUIRE_REASONS; diff --git a/src/globals.d.ts b/src/globals.d.ts index 8698ddaf..c1122765 100644 --- a/src/globals.d.ts +++ b/src/globals.d.ts @@ -1,6 +1,7 @@ declare const ACCESS_CONFIG: any; declare const APP_NAME: string; declare const REQUIRE_DESCRIPTIONS: boolean; +declare const REQUIRE_REASONS: boolean; interface ImportMetaEnv { readonly VITE_API_SERVER_URL: string; diff --git a/src/pages/groups/AddRoles.tsx b/src/pages/groups/AddRoles.tsx index 886fba11..dbcaca80 100644 --- a/src/pages/groups/AddRoles.tsx +++ b/src/pages/groups/AddRoles.tsx @@ -40,7 +40,7 @@ import {PolymorphicGroup, RoleGroup, RoleMember, OktaUser} from '../../api/apiSc import {canManageGroup, isAccessAdmin, isGroupOwner} from '../../authorization'; import {minTagTime, ownerCantAddSelf, requiredReason} from '../../helpers'; import {useCurrentUser} from '../../authentication'; -import accessConfig from '../../config/accessConfig'; +import accessConfig, {requireReasons} from '../../config/accessConfig'; dayjs.extend(IsSameOrBefore); @@ -295,7 +295,7 @@ function AddRolesDialog(props: AddRolesDialogProps) { name="reason" multiline rows={4} - required={reason} + required={requireReasons || reason} validation={{maxLength: 1024}} parseError={(error) => { if (error?.message != '') { diff --git a/src/pages/groups/AddUsers.tsx b/src/pages/groups/AddUsers.tsx index b9e8194e..f8fb3f8f 100644 --- a/src/pages/groups/AddUsers.tsx +++ b/src/pages/groups/AddUsers.tsx @@ -47,7 +47,7 @@ import { requiredReason, requiredReasonGroups, } from '../../helpers'; -import accessConfig from '../../config/accessConfig'; +import accessConfig, {requireReasons} from '../../config/accessConfig'; dayjs.extend(IsSameOrBefore); @@ -278,7 +278,7 @@ function AddUsersDialog(props: AddUsersDialogProps) { name="reason" multiline rows={4} - required={reason} + required={requireReasons || reason} validation={{maxLength: 1024}} parseError={(error) => { if (error?.message != '') { diff --git a/src/pages/groups/BulkRenewal.tsx b/src/pages/groups/BulkRenewal.tsx index a5e9edc8..ae71bc28 100644 --- a/src/pages/groups/BulkRenewal.tsx +++ b/src/pages/groups/BulkRenewal.tsx @@ -34,7 +34,7 @@ import {displayUserName, minTagTimeGroups, requiredReasonGroups} from '../../hel import {usePutGroupMembersById, PutGroupMembersByIdError, PutGroupMembersByIdVariables} from '../../api/apiComponents'; import {GroupMember, OktaUserGroupMember, PolymorphicGroup, RoleGroupMap, RoleGroup} from '../../api/apiSchemas'; import BulkRenewalDataGrid from '../../components/BulkRenewalDataGrid'; -import accessConfig from '../../config/accessConfig'; +import accessConfig, {requireReasons} from '../../config/accessConfig'; interface Data { id: number; @@ -479,7 +479,7 @@ function BulkRenewalDialog(props: BulkRenewalDialogProps) { name="reason" multiline rows={1} - required={requiredReason} + required={requireReasons || requiredReason} validation={{maxLength: 1024}} parseError={(error) => { if (error?.message != '') { diff --git a/src/pages/requests/Create.tsx b/src/pages/requests/Create.tsx index 9eb8480d..d4ef9f1f 100644 --- a/src/pages/requests/Create.tsx +++ b/src/pages/requests/Create.tsx @@ -43,7 +43,7 @@ import { } from '../../api/apiSchemas'; import {canManageGroup} from '../../authorization'; import {minTagTime, minTagTimeGroups} from '../../helpers'; -import accessConfig from '../../config/accessConfig'; +import accessConfig, {requireReasons} from '../../config/accessConfig'; dayjs.extend(IsSameOrBefore); @@ -442,6 +442,7 @@ function CreateRequestContainer(props: CreateRequestContainerProps) { name="reason" multiline rows={4} + required={requireReasons} validation={{maxLength: 1024}} parseError={(error) => { if (error?.message != '') { diff --git a/src/pages/requests/Read.tsx b/src/pages/requests/Read.tsx index 1b4ed9cd..38bfea64 100644 --- a/src/pages/requests/Read.tsx +++ b/src/pages/requests/Read.tsx @@ -76,7 +76,7 @@ import { import NotFound from '../NotFound'; import ChangeTitle from '../../tab-title'; import Loading from '../../components/Loading'; -import accessConfig from '../../config/accessConfig'; +import accessConfig, {requireReasons} from '../../config/accessConfig'; import {EmptyListEntry} from '../../components/EmptyListEntry'; import AccessHistory from '../../components/AccessHistory'; @@ -700,6 +700,7 @@ export default function ReadRequest() { name="reason" multiline rows={4} + required={requireReasons || reason} validation={{maxLength: 1024}} parseError={(error) => { if (error?.message != '') { diff --git a/src/pages/role_requests/Create.tsx b/src/pages/role_requests/Create.tsx index 437fb71b..b71f0304 100644 --- a/src/pages/role_requests/Create.tsx +++ b/src/pages/role_requests/Create.tsx @@ -51,6 +51,7 @@ import {useCurrentUser} from '../../authentication'; import {canManageGroup} from '../../authorization'; import {minTagTime, minTagTimeGroups} from '../../helpers'; import {Tooltip} from '@mui/material'; +import {requireReasons} from '../../config/accessConfig'; dayjs.extend(IsSameOrBefore); @@ -462,6 +463,7 @@ function CreateRequestContainer(props: CreateRequestContainerProps) { name="reason" multiline rows={4} + required={requireReasons} validation={{maxLength: 1024}} parseError={(error) => { if (error?.message != '') { diff --git a/src/pages/role_requests/Read.tsx b/src/pages/role_requests/Read.tsx index ca91fdeb..60964910 100644 --- a/src/pages/role_requests/Read.tsx +++ b/src/pages/role_requests/Read.tsx @@ -81,6 +81,7 @@ import NotFound from '../NotFound'; import Loading from '../../components/Loading'; import ChangeTitle from '../../tab-title'; import AccessHistory from '../../components/AccessHistory'; +import {requireReasons} from '../../config/accessConfig'; dayjs.extend(RelativeTime); dayjs.extend(IsSameOrBefore); @@ -787,6 +788,7 @@ export default function ReadRoleRequest() { name="reason" multiline rows={4} + required={requireReasons || reason} validation={{maxLength: 1024}} parseError={(error) => { if (error?.message != '') { diff --git a/src/pages/roles/AddGroups.tsx b/src/pages/roles/AddGroups.tsx index d23ccbc7..4bb06471 100644 --- a/src/pages/roles/AddGroups.tsx +++ b/src/pages/roles/AddGroups.tsx @@ -43,7 +43,7 @@ import {isAccessAdmin, isGroupOwner} from '../../authorization'; import {minTagTimeGroups, requiredReasonGroups, ownerCantAddSelf} from '../../helpers'; import {useCurrentUser} from '../../authentication'; import {group} from 'console'; -import accessConfig from '../../config/accessConfig'; +import accessConfig, {requireReasons} from '../../config/accessConfig'; dayjs.extend(IsSameOrBefore); @@ -270,7 +270,7 @@ function AddGroupsDialog(props: AddGroupsDialogProps) { name="reason" multiline rows={4} - required={requiredReason} + required={requireReasons || requiredReason} validation={{maxLength: 1024}} parseError={(error) => { if (error?.message != '') { diff --git a/src/pages/roles/BulkRenewal.tsx b/src/pages/roles/BulkRenewal.tsx index f171f2e0..75d856ee 100644 --- a/src/pages/roles/BulkRenewal.tsx +++ b/src/pages/roles/BulkRenewal.tsx @@ -37,7 +37,7 @@ import {usePutRoleMembersById, PutRoleMembersByIdError, PutRoleMembersByIdVariab import {RoleMember, RoleGroupMap, OktaGroup, AppGroup} from '../../api/apiSchemas'; import {isAccessAdmin} from '../../authorization'; import BulkRenewalDataGrid from '../../components/BulkRenewalDataGrid'; -import accessConfig from '../../config/accessConfig'; +import accessConfig, {requireReasons} from '../../config/accessConfig'; interface Data { id: number; @@ -516,7 +516,7 @@ function BulkRenewalDialog(props: BulkRenewalDialogProps) { name="reason" multiline rows={1} - required={requiredReason} + required={requireReasons || requiredReason} validation={{maxLength: 1024}} parseError={(error) => { if (error?.message != '') { diff --git a/tests/conftest.py b/tests/conftest.py index e7179bf7..79b76ba5 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -36,9 +36,17 @@ def app(request: pytest.FixtureRequest) -> Flask: app = create_app(testing=True) # Check if parametrization is being used (indirect parametrization) - require_descriptions = getattr(request, "param", False) - # Set the config on the Flask app (schemas read from current_app.config at validation time) - app.config["REQUIRE_DESCRIPTIONS"] = require_descriptions + param = getattr(request, "param", False) + + if isinstance(param, dict): + app.config.update(param) + if "REQUIRE_DESCRIPTIONS" not in param: + app.config["REQUIRE_DESCRIPTIONS"] = False + if "REQUIRE_REASONS" not in param: + app.config["REQUIRE_REASONS"] = False + else: + app.config["REQUIRE_DESCRIPTIONS"] = param + app.config["REQUIRE_REASONS"] = False return app diff --git a/tests/test_access_request.py b/tests/test_access_request.py index d7f7727b..e9ed24cd 100644 --- a/tests/test_access_request.py +++ b/tests/test_access_request.py @@ -1,3 +1,4 @@ +import pytest from datetime import datetime, timedelta from typing import Any @@ -807,3 +808,41 @@ def test_auto_resolve_create_access_request_with_time_limit_constraint_tag( assert user == kwargs["requester"] assert len(kwargs["group_tags"]) == 1 assert tag in kwargs["group_tags"] + + +@pytest.mark.parametrize("app", [{"REQUIRE_REASONS": True}, {"REQUIRE_REASONS": False}], indirect=True) +def test_create_access_request_require_reasons( + app: Flask, client: FlaskClient, db: SQLAlchemy, okta_group: OktaGroup +) -> None: + require_reasons = app.config.get("REQUIRE_REASONS", False) + access_requests_url = url_for("api-access-requests.access_requests") + + db.session.add(okta_group) + db.session.commit() + + if require_reasons: + data = { + "group_id": okta_group.id, + "group_owner": False, + "reason": "", + } + rep = client.post(access_requests_url, json=data) + assert rep.status_code == 400 + assert "Reason must be between 1 and 1024 characters" in str(rep.get_json()) + + data["reason"] = " " + rep = client.post(access_requests_url, json=data) + assert rep.status_code == 400 + assert "Reason must be between 1 and 1024 characters" in str(rep.get_json()) + + data["reason"] = "Valid reason" + rep = client.post(access_requests_url, json=data) + assert rep.status_code == 201 + else: + data = { + "group_id": okta_group.id, + "group_owner": False, + "reason": "", + } + rep = client.post(access_requests_url, json=data) + assert rep.status_code == 201 diff --git a/tests/test_role_request.py b/tests/test_role_request.py index 2b34f6a3..7ab0ff00 100644 --- a/tests/test_role_request.py +++ b/tests/test_role_request.py @@ -1,3 +1,4 @@ +import pytest from datetime import datetime, timedelta, timezone from typing import Any @@ -1271,3 +1272,54 @@ def test_role_request_approval_via_direct_add( assert len(data["owners"]) == 1 assert len(data["members"]) == 0 assert data["owners"][0] == user.id + + +@pytest.mark.parametrize("app", [{"REQUIRE_REASONS": True}, {"REQUIRE_REASONS": False}], indirect=True) +def test_create_role_request_require_reasons( + app: Flask, client: FlaskClient, db: SQLAlchemy, role_group: RoleGroup, okta_group: OktaGroup, user: OktaUser +) -> None: + require_reasons = app.config.get("REQUIRE_REASONS", False) + role_requests_url = url_for("api-role-requests.role_requests") + + db.session.add(user) + db.session.add(okta_group) + db.session.add(role_group) + db.session.commit() + + ModifyGroupUsers( + group=role_group, + members_to_add=[], + owners_to_add=[user.id], + sync_to_okta=False + ).execute() + + app.config["CURRENT_OKTA_USER_EMAIL"] = user.email + + if require_reasons: + data = { + "role_id": role_group.id, + "group_id": okta_group.id, + "group_owner": False, + "reason": "", + } + rep = client.post(role_requests_url, json=data) + assert rep.status_code == 400 + assert "Reason must be between 1 and 1024 characters" in str(rep.get_json()) + + data["reason"] = " " + rep = client.post(role_requests_url, json=data) + assert rep.status_code == 400 + assert "Reason must be between 1 and 1024 characters" in str(rep.get_json()) + + data["reason"] = "Valid reason" + rep = client.post(role_requests_url, json=data) + assert rep.status_code == 201 + else: + data = { + "role_id": role_group.id, + "group_id": okta_group.id, + "group_owner": False, + "reason": "", + } + rep = client.post(role_requests_url, json=data) + assert rep.status_code == 201 diff --git a/vite.config.ts b/vite.config.ts index 12fd2840..c7adbd9e 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -41,6 +41,7 @@ export default defineConfig(({mode}) => { ACCESS_CONFIG: accessConfig, APP_NAME: JSON.stringify(env.APP_NAME || 'Access'), REQUIRE_DESCRIPTIONS: env.REQUIRE_DESCRIPTIONS?.toLowerCase() === 'true', + REQUIRE_REASONS: env.REQUIRE_REASONS?.toLowerCase() === 'true', }, server: { port: 3000,