Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions api/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
6 changes: 4 additions & 2 deletions api/views/schemas/access_requests.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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:
Expand Down
43 changes: 43 additions & 0 deletions api/views/schemas/core_schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Comment on lines +108 to +109
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When REQUIRE_REASONS is enabled and the client explicitly sends JSON null for reason, this branch calls self.fail("required"), which produces Marshmallow’s generic “Missing data for required field.” message. That’s inconsistent with the other reason validation errors and with the intended “non-empty, max 1024” rule. Consider raising a consistent ValidationError for the None/null case (e.g., the same message used for empty/whitespace).

Suggested change
if value is None or value == "":
return "" if not require_reasons else self.fail("required")
if value is None:
if require_reasons:
# Ensure null/None gets the same validation message as empty/whitespace
raise ValidationError("Reason must be between 1 and 1024 characters")
return ""

Copilot uses AI. Check for mistakes.

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)
Expand Down
6 changes: 4 additions & 2 deletions api/views/schemas/role_requests.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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:
Expand Down
1 change: 1 addition & 0 deletions src/config/accessConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,4 @@ export default accessConfig;

export const appName = APP_NAME;
export const requireDescriptions = REQUIRE_DESCRIPTIONS;
export const requireReasons = REQUIRE_REASONS;
1 change: 1 addition & 0 deletions src/globals.d.ts
Original file line number Diff line number Diff line change
@@ -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;
Expand Down
4 changes: 2 additions & 2 deletions src/pages/groups/AddRoles.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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 != '') {
Expand Down
4 changes: 2 additions & 2 deletions src/pages/groups/AddUsers.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ import {
requiredReason,
requiredReasonGroups,
} from '../../helpers';
import accessConfig from '../../config/accessConfig';
import accessConfig, {requireReasons} from '../../config/accessConfig';

dayjs.extend(IsSameOrBefore);

Expand Down Expand Up @@ -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 != '') {
Expand Down
4 changes: 2 additions & 2 deletions src/pages/groups/BulkRenewal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 != '') {
Expand Down
3 changes: 2 additions & 1 deletion src/pages/requests/Create.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -442,6 +442,7 @@ function CreateRequestContainer(props: CreateRequestContainerProps) {
name="reason"
multiline
rows={4}
required={requireReasons}
validation={{maxLength: 1024}}
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UI marks this field required when requireReasons is true, but the only client-side validation here is maxLength. A whitespace-only value (e.g., " ") will typically satisfy “required” on the client but is rejected by the backend (which strips whitespace when REQUIRE_REASONS is enabled), causing a confusing submit-time 400. Consider adding a client-side validation rule that enforces at least one non-whitespace character (or trimming the value before submit) when requireReasons is enabled.

Suggested change
validation={{maxLength: 1024}}
validation={{
maxLength: 1024,
validate: (value) => {
if (!requireReasons) {
return true;
}
if (typeof value !== 'string') {
return 'Reason is required';
}
return value.trim().length > 0 || 'Reason is required';
},
}}

Copilot uses AI. Check for mistakes.
parseError={(error) => {
if (error?.message != '') {
Expand Down
3 changes: 2 additions & 1 deletion src/pages/requests/Read.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -700,6 +700,7 @@ export default function ReadRequest() {
name="reason"
multiline
rows={4}
required={requireReasons || reason}
validation={{maxLength: 1024}}
parseError={(error) => {
if (error?.message != '') {
Expand Down
2 changes: 2 additions & 0 deletions src/pages/role_requests/Create.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -462,6 +463,7 @@ function CreateRequestContainer(props: CreateRequestContainerProps) {
name="reason"
multiline
rows={4}
required={requireReasons}
validation={{maxLength: 1024}}
parseError={(error) => {
if (error?.message != '') {
Expand Down
2 changes: 2 additions & 0 deletions src/pages/role_requests/Read.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -787,6 +788,7 @@ export default function ReadRoleRequest() {
name="reason"
multiline
rows={4}
required={requireReasons || reason}
validation={{maxLength: 1024}}
parseError={(error) => {
if (error?.message != '') {
Expand Down
4 changes: 2 additions & 2 deletions src/pages/roles/AddGroups.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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 != '') {
Expand Down
4 changes: 2 additions & 2 deletions src/pages/roles/BulkRenewal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 != '') {
Expand Down
14 changes: 11 additions & 3 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
39 changes: 39 additions & 0 deletions tests/test_access_request.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import pytest
from datetime import datetime, timedelta
from typing import Any

Expand Down Expand Up @@ -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
Comment on lines +813 to +848
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test covers create-time behavior with REQUIRE_REASONS toggled, but the PR also changes resolve (PUT) schemas to require/validate reason when enabled. Adding a parametrized resolve-path assertion (e.g., PUT with missing/blank reason should 400 when enabled and succeed when disabled) would prevent regressions in the resolve flow.

Copilot uses AI. Check for mistakes.
Loading