feat: Add global REQUIRE_REASONS config option by ianuriegas · Pull Request #378 · discord/access

ianuriegas · 2026-03-03T21:25:39Z

Summary

Adds a REQUIRE_REASONS environment variable so admins can require business justifications when creating or resolving access/role requests
When enabled, reason is required and validated (non-empty, max 1024 chars) on create and resolve flows; when disabled, behavior is unchanged
Frontend and backend both respect the flag so forms and API stay in sync

Changes

API: New REQUIRE_REASONS config, context-aware reason field in core_schemas (read at validation time via current_app), and reason validation on access/role request create and resolve schemas
Frontend: requireReasons wired through Vite env, accessConfig, and globals; reason fields marked required and validated on all request pages (access/role create and resolve, bulk renewal, add users/roles/groups)
Tests: Conftest supports dict-based app parametrization; new parametrized access-request and role-request tests for required-reason behavior when enabled/disabled

Testing

tox -- tests/test_access_request.py::test_create_access_request_require_reasons tests/test_role_request.py::test_create_role_request_require_reasons

REQUIRE_REASONS=true

REQUIRE_REASONS=false

Result: Preserves original behavior; justification remains optional.

Introduce a REQUIRE_REASONS environment variable that enforces business justifications on create and resolve request flows. - Add context-aware reason field in core_schemas.py that reads config at validation time via current_app - Wire REQUIRE_REASONS through backend config, Vite, and frontend - Enforce required reasons across all request pages (access, role, bulk renewal, add users/roles/groups) - Update test fixture to support dict-based parametrization - Add role request reason-required test coverage

Copilot

Pull request overview

Adds a global REQUIRE_REASONS configuration flag to require business-justification reasons across access/role request create + resolve flows, keeping backend schema validation and frontend form requirements aligned.

Changes:

Backend: introduces REQUIRE_REASONS config and a context_aware_reason_field used by access/role request create + resolve schemas.
Frontend: wires requireReasons through Vite globals/config and marks reason inputs as required across request and bulk-renewal/add flows.
Tests: enhances app fixture parametrization and adds create-request tests for REQUIRE_REASONS enabled/disabled behavior.

Reviewed changes

Copilot reviewed 18 out of 19 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
vite.config.ts	Exposes `REQUIRE_REASONS` to the frontend via Vite define globals.
api/config.py	Adds backend env-config flag `REQUIRE_REASONS`.
api/views/schemas/core_schemas.py	Adds context-aware marshmallow field that enforces reason requirement based on app config at validation time.
api/views/schemas/access_requests.py	Uses context-aware reason field for access request create/resolve schemas.
api/views/schemas/role_requests.py	Uses context-aware reason field for role request create/resolve schemas.
src/globals.d.ts	Declares new global `REQUIRE_REASONS`.
src/config/accessConfig.ts	Exports `requireReasons` for app-wide consumption.
src/pages/requests/Create.tsx	Marks create access-request reason as required based on `requireReasons`.
src/pages/requests/Read.tsx	Marks resolve access-request reason as required when globally required or tag-required.
src/pages/role_requests/Create.tsx	Marks create role-request reason as required based on `requireReasons`.
src/pages/role_requests/Read.tsx	Marks resolve role-request reason as required when globally required or tag-required.
src/pages/groups/AddUsers.tsx	Ensures reason required if globally required.
src/pages/groups/AddRoles.tsx	Ensures reason required if globally required.
src/pages/groups/BulkRenewal.tsx	Ensures reason required if globally required.
src/pages/roles/AddGroups.tsx	Ensures reason required if globally required.
src/pages/roles/BulkRenewal.tsx	Ensures reason required if globally required.
tests/conftest.py	Allows dict-based app config parametrization for tests.
tests/test_access_request.py	Adds create-access-request test for `REQUIRE_REASONS` toggling.
tests/test_role_request.py	Adds create-role-request test for `REQUIRE_REASONS` toggling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tests/conftest.py

Copilot · 2026-03-03T21:32:30Z

api/views/schemas/core_schemas.py

+            if value is None or value == "":
+                return "" if not require_reasons else self.fail("required")


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 · 2026-03-03T21:32:31Z

src/pages/requests/Create.tsx

            multiline
            rows={4}
+            required={requireReasons}
            validation={{maxLength: 1024}}


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 · 2026-03-03T21:32:31Z

tests/test_access_request.py

+@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


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 · 2026-03-03T21:32:31Z

tests/test_role_request.py

+@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


This test validates create-time behavior, but resolve (PUT) schemas were also updated to use context_aware_reason_field. Consider adding coverage for resolving a role request with REQUIRE_REASONS enabled (blank/missing reason should 400; valid reason should succeed) to ensure the create/resolve promise in the PR description stays enforced.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings March 3, 2026 21:25

Copilot started reviewing on behalf of ianuriegas March 3, 2026 21:26 View session

Copilot AI reviewed Mar 3, 2026

View reviewed changes

Update tests/conftest.py

f506567

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add global REQUIRE_REASONS config option#378

feat: Add global REQUIRE_REASONS config option#378
ianuriegas wants to merge 2 commits intodiscord:mainfrom
ianuriegas:feat/require-reasons

ianuriegas commented Mar 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		if value is None or value == "":
		return "" if not require_reasons else self.fail("required")

-            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';
+              },
+            }}

Conversation

ianuriegas commented Mar 3, 2026

Summary

Changes

Testing

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants