diff --git a/api/access_config.py b/api/access_config.py index b7be842e..2d0d1e30 100644 --- a/api/access_config.py +++ b/api/access_config.py @@ -1,7 +1,7 @@ import json import logging import os -from typing import Any +from typing import Any, Optional logger = logging.getLogger(__name__) @@ -9,6 +9,8 @@ BACKEND = "BACKEND" NAME_VALIDATION_PATTERN = "NAME_VALIDATION_PATTERN" NAME_VALIDATION_ERROR = "NAME_VALIDATION_ERROR" +REASON_TEMPLATE = "REASON_TEMPLATE" +REASON_TEMPLATE_REQUIRED = "REASON_TEMPLATE_REQUIRED" class UndefinedConfigKeyError(Exception): @@ -27,9 +29,17 @@ def __init__(self, error: str): class AccessConfig: - def __init__(self, name_pattern: str, name_validation_error: str): + def __init__( + self, + name_pattern: str, + name_validation_error: str, + reason_template: Optional[str] = None, + reason_template_required: Optional[list[str]] = None, + ): self.name_pattern = name_pattern self.name_validation_error = name_validation_error + self.reason_template = reason_template + self.reason_template_required = reason_template_required def _get_config_value(config: dict[str, Any], key: str) -> Any: @@ -77,9 +87,15 @@ def _load_access_config() -> AccessConfig: name_pattern = _get_config_value(config, NAME_VALIDATION_PATTERN) name_validation_error = _get_config_value(config, NAME_VALIDATION_ERROR) + # Reason template and required fields are optional + reason_template = config.get(REASON_TEMPLATE) + reason_template_required = config.get(REASON_TEMPLATE_REQUIRED) + return AccessConfig( name_pattern=name_pattern, name_validation_error=name_validation_error, + reason_template=reason_template, + reason_template_required=reason_template_required, ) diff --git a/api/operations/constraints/check_for_reason.py b/api/operations/constraints/check_for_reason.py index fab0ead5..c5e0b8be 100644 --- a/api/operations/constraints/check_for_reason.py +++ b/api/operations/constraints/check_for_reason.py @@ -5,6 +5,7 @@ selectinload, ) +from api.access_config import get_access_config from api.extensions import db from api.models import AppGroup, OktaGroup, OktaGroupTagMap, RoleGroup, RoleGroupMap, Tag from api.models.tag import coalesce_constraints @@ -44,7 +45,21 @@ def __init__( @staticmethod def invalid_reason(reason: Optional[str]) -> bool: - return reason is None or reason.strip() == "" + if reason is None or reason.strip() == "": + return True + + # Check if the reason is just the template without modification + access_config = get_access_config() + if access_config.reason_template and reason.strip() == access_config.reason_template.strip(): + return True + + # Check if the reason is missing required fields from the template + if access_config.reason_template_required and reason: + for required_field in access_config.reason_template_required: + if required_field not in reason: + return True + + return False def execute_for_group(self) -> Tuple[bool, str]: if self.invalid_reason(self.reason): diff --git a/api/views/schemas/access_requests.py b/api/views/schemas/access_requests.py index bd346644..f0ade1b2 100644 --- a/api/views/schemas/access_requests.py +++ b/api/views/schemas/access_requests.py @@ -1,7 +1,9 @@ from datetime import datetime, timezone from typing import Any, Dict, Optional -from marshmallow import Schema, ValidationError, fields, post_load, validate +from marshmallow import Schema, ValidationError, fields, post_load, validate, validates + +from api.access_config import get_access_config class CreateAccessRequestSchema(Schema): @@ -9,6 +11,28 @@ class CreateAccessRequestSchema(Schema): group_owner = fields.Boolean(load_default=False, load_only=True) reason = fields.String(validate=validate.Length(max=1024), load_only=True) + @validates("reason") + def validate_reason(self, reason: str) -> None: + access_config = get_access_config() + + # Check if reason is empty or only whitespace + if not reason or not reason.strip(): + raise ValidationError("Reason is required") + + # Check if reason is the same as the template + if access_config.reason_template and reason.strip() == access_config.reason_template.strip(): + raise ValidationError( + "Please fill out the template with your specific information instead of submitting the template as-is." + ) + + # Check if required template fields are present + if access_config.reason_template_required and reason: + for required_field in access_config.reason_template_required: + if required_field not in reason: + raise ValidationError( + f"The following required field is missing from your reason: {required_field}. Please fill it out before submitting your access request." + ) + @staticmethod def must_be_in_the_future(data: Optional[datetime]) -> None: if data and data < datetime.now(): diff --git a/src/config/accessConfig.ts b/src/config/accessConfig.ts index e533d7a7..84479d16 100644 --- a/src/config/accessConfig.ts +++ b/src/config/accessConfig.ts @@ -3,6 +3,8 @@ export interface AccessConfig { DEFAULT_ACCESS_TIME: string; NAME_VALIDATION_PATTERN: string; NAME_VALIDATION_ERROR: string; + REASON_TEMPLATE: string; + REASON_TEMPLATE_REQUIRED: string[]; } // use the globally-injected ACCESS_CONFIG from src/globals.d.ts, typed to AccessConfig interface diff --git a/src/config/loadAccessConfig.js b/src/config/loadAccessConfig.js index 1826b35a..4f100565 100644 --- a/src/config/loadAccessConfig.js +++ b/src/config/loadAccessConfig.js @@ -11,6 +11,8 @@ const DEFAULT_ACCESS_TIME = 'DEFAULT_ACCESS_TIME'; const FRONTEND = 'FRONTEND'; const NAME_VALIDATION_PATTERN = 'NAME_VALIDATION_PATTERN'; const NAME_VALIDATION_ERROR = 'NAME_VALIDATION_ERROR'; +const REASON_TEMPLATE = 'REASON_TEMPLATE'; +const REASON_TEMPLATE_REQUIRED = 'REASON_TEMPLATE_REQUIRED'; class UndefinedConfigError extends Error { constructor(key, obj) { @@ -59,6 +61,24 @@ function validate_override_config(overrideConfig) { `If ${NAME_VALIDATION_PATTERN} is present, ${NAME_VALIDATION_ERROR} must also be overridden.`, ); } + + if (REASON_TEMPLATE in overrideConfig && !(REASON_TEMPLATE_REQUIRED in overrideConfig)) { + throw new AccessConfigValidationError( + `If ${REASON_TEMPLATE} is present, ${REASON_TEMPLATE_REQUIRED} must also be overridden.`, + ); + } + // ensure that REASON_TEMPLATE has all required fields in REASON_TEMPLATE_REQUIRED + if (REASON_TEMPLATE in overrideConfig && REASON_TEMPLATE_REQUIRED in overrideConfig) { + const requiredFields = getConfig(overrideConfig, REASON_TEMPLATE_REQUIRED); + const reasonTemplate = getConfig(overrideConfig, REASON_TEMPLATE); + for (const field of requiredFields) { + if (!(field in reasonTemplate)) { + throw new AccessConfigValidationError( + `Field '${field}' is required in ${REASON_TEMPLATE} when ${REASON_TEMPLATE_REQUIRED} is set.`, + ); + } + } + } } function loadOverrideConfig(accessConfig) { diff --git a/src/pages/requests/Create.tsx b/src/pages/requests/Create.tsx index 11035965..ee6ee201 100644 --- a/src/pages/requests/Create.tsx +++ b/src/pages/requests/Create.tsx @@ -309,6 +309,7 @@ function CreateRequestContainer(props: CreateRequestContainerProps) { group: props.group, until: accessConfig.DEFAULT_ACCESS_TIME, ownerOrMember: props.owner != null ? (props.owner ? 'owner' : 'member') : undefined, + reason: accessConfig.REASON_TEMPLATE || '', }} onSuccess={(formData) => submit(formData)}> @@ -434,7 +435,35 @@ function CreateRequestContainer(props: CreateRequestContainerProps) { name="reason" multiline rows={4} - validation={{maxLength: 1024}} + placeholder={accessConfig.REASON_TEMPLATE} + validation={{ + required: 'Reason is required', + maxLength: 1024, + validate: (value: string) => { + // Check if reason is empty or only whitespace + if (!value || value.trim().length === 0) { + return 'Reason is required'; + } + + // Check if reason is the same as the template + if (accessConfig.REASON_TEMPLATE && value.trim() === accessConfig.REASON_TEMPLATE.trim()) { + return 'Please fill out the template with your specific information instead of submitting the template as-is.'; + } + + // Check if required template fields are present + if (accessConfig.REASON_TEMPLATE_REQUIRED && value) { + const missingFields = accessConfig.REASON_TEMPLATE_REQUIRED.filter( + (field: string) => !value.includes(field), + ); + + if (missingFields.length > 0) { + return `The following required fields are missing from your reason: ${missingFields.join(', ')}`; + } + } + + return true; + }, + }} parseError={(error) => { if (error?.message != '') { return error?.message ?? ''; diff --git a/src/pages/role_requests/Create.tsx b/src/pages/role_requests/Create.tsx index 39b8c597..15b0845c 100644 --- a/src/pages/role_requests/Create.tsx +++ b/src/pages/role_requests/Create.tsx @@ -2,6 +2,7 @@ import * as React from 'react'; import dayjs, {Dayjs} from 'dayjs'; import IsSameOrBefore from 'dayjs/plugin/isSameOrBefore'; import {useNavigate} from 'react-router-dom'; +import accessConfig from '../../config/accessConfig'; import Button from '@mui/material/Button'; import Dialog from '@mui/material/Dialog'; import DialogActions from '@mui/material/DialogActions'; @@ -462,6 +463,7 @@ function CreateRequestContainer(props: CreateRequestContainerProps) { name="reason" multiline rows={4} + placeholder={accessConfig.REASON_TEMPLATE} validation={{maxLength: 1024}} parseError={(error) => { if (error?.message != '') { diff --git a/tests/test_reason_template.py b/tests/test_reason_template.py new file mode 100644 index 00000000..10da419e --- /dev/null +++ b/tests/test_reason_template.py @@ -0,0 +1,95 @@ +from typing import Any + +from flask import url_for +from flask.testing import FlaskClient +from flask_sqlalchemy import SQLAlchemy +from pytest_mock import MockerFixture + +from api.models import ( + OktaGroup, + OktaUser, +) +from api.operations.constraints.check_for_reason import CheckForReason + + +def test_invalid_reason_with_template(mocker: MockerFixture) -> None: + """Test that the template itself and templates with placeholders are considered invalid reasons.""" + + # Mock the access_config to return a specific template + mock_access_config = mocker.patch("api.operations.constraints.check_for_reason.get_access_config") + mock_config = mocker.MagicMock() + mock_config.reason_template = ( + "Project: [Project Name]\nTicket: [Ticket ID]\nJustification: [Why is this access needed?]" + ) + mock_config.reason_template_required = ["Project", "Ticket", "Justification"] + mock_access_config.return_value = mock_config + + # Test cases + # 1. Empty reason + assert CheckForReason.invalid_reason(None) is True + assert CheckForReason.invalid_reason("") is True + assert CheckForReason.invalid_reason(" ") is True + + # 2. Template as-is (unchanged) + template = "Project: [Project Name]\nTicket: [Ticket ID]\nJustification: [Why is this access needed?]" + assert CheckForReason.invalid_reason(template) is True + + # 3. Template with missing fields + template = "Project: [Project Name]\nTicket: [Ticket ID]" + assert CheckForReason.invalid_reason(template) is True + + # 4. Template with all placeholders filled should be valid + filled_template = "Project: My Project\nTicket: TICKET-123\nJustification: I need access to deploy code" + assert CheckForReason.invalid_reason(filled_template) is False + + # 5. Completely different invalid reason + valid_reason = "I need access for the new project launch" + assert CheckForReason.invalid_reason(valid_reason) is True + + +def test_reason_validation_in_request_endpoint( + client: FlaskClient, + db: SQLAlchemy, + mocker: MockerFixture, + okta_group: OktaGroup, + user: OktaUser, +) -> None: + """Test that the API endpoints reject reasons that just contain the template.""" + + # Mock the access_config to return a specific template + mock_access_config = mocker.patch("api.views.schemas.access_requests.get_access_config") + mock_config = mocker.MagicMock() + mock_config.reason_template = ( + "Project: [Project Name]\nTicket: [Ticket ID]\nJustification: [Why is this access needed?]" + ) + mock_config.reason_template_required = ["Project", "Ticket", "Justification"] + mock_access_config.return_value = mock_config + + # Set up the group and user + db.session.add(okta_group) + db.session.add(user) + db.session.commit() + + # Try creating an access request with the template as reason + template = "Project: [Project Name]\nTicket: [Ticket ID]" + + data: dict[str, Any] = { + "group_id": okta_group.id, + "group_owner": False, + "reason": template, + } + + # Create the access request + access_request_url = url_for("api-access-requests.access_requests") + rep = client.post(access_request_url, json=data) + + # Should get rejected because the reason it is missing required information + assert rep.status_code == 400 + + # Try again with a filled template + filled_template = "Project: My Project\nTicket: TICKET-123\nJustification: I need access to deploy code" + data["reason"] = filled_template + + rep = client.post(access_request_url, json=data) + # Should succeed with a properly filled template + assert rep.status_code == 201