From 06dba4fb13eed675cc08ff8c00fe3e3650468c3b Mon Sep 17 00:00:00 2001 From: sechmachine <97589681+sechmachine727@users.noreply.github.com> Date: Sat, 15 Aug 2026 03:13:12 +0700 Subject: [PATCH] fix(platform): address round 2 review findings --- docs/tests/unit/vietnam-business-clock.md | 72 ++++++++++++++++++ docs/tests/web/account-uniqueness-feedback.md | 75 +++++++++++++++++++ docs/tests/web/platform-onboarding-forms.md | 9 ++- docs/tests/web/smtp-error-sanitization.md | 75 +++++++++++++++++++ .../config/TimeConfiguration.java | 7 +- .../account/controller/AccountController.java | 33 +++++++- .../controller/SmtpController.java | 11 ++- .../resources/templates/accounts/new.html | 1 + .../config/TimeConfigurationTest.java | 21 ++++++ .../controller/AccountWebIntegrationTest.java | 31 ++++++++ .../SmtpOnboardingWebIntegrationTest.java | 7 +- 11 files changed, 329 insertions(+), 13 deletions(-) create mode 100644 docs/tests/unit/vietnam-business-clock.md create mode 100644 docs/tests/web/account-uniqueness-feedback.md create mode 100644 docs/tests/web/smtp-error-sanitization.md create mode 100644 src/test/java/com/lab/labtimesheet/config/TimeConfigurationTest.java diff --git a/docs/tests/unit/vietnam-business-clock.md b/docs/tests/unit/vietnam-business-clock.md new file mode 100644 index 0000000..4ae1dd6 --- /dev/null +++ b/docs/tests/unit/vietnam-business-clock.md @@ -0,0 +1,72 @@ +# Test Evidence: Vietnam business-date clock boundary + +- **Test type:** Unit +- **Requirement IDs:** `ACC-019`, `ACC-020` +- **Scenario IDs:** `AC-ACC-010` (business-date boundary only) +- **Test class/method:** `com.lab.labtimesheet.config.TimeConfigurationTest#utcInstantAtVietnamMidnightUsesTheNewLocalBusinessDate` +- **Implementation commit:** `pending` + +## Protected behavior + +The production application clock uses `Asia/Ho_Chi_Minh`, so account lifecycle decisions based on `LocalDate.now` +advance at Vietnam midnight rather than seven hours later at UTC midnight. + +## Test method + +The test obtains the real production clock configuration, fixes its configured zone at the UTC instant +`2026-08-14T17:00:00Z`, and derives the local business date. No Spring context or database is needed because the +contract under test is the clock bean's zone. + +## Hand-derived expected result + +Vietnam is UTC+07:00, so `2026-08-14T17:00:00Z` is `2026-08-15T00:00:00+07:00` and the business date is +`2026-08-15`. + +## RED + +**Command** + +```text +export JAVA_HOME=/opt/homebrew/opt/openjdk@25 +export PATH="$JAVA_HOME/bin:$PATH" +./mvnw -Dtest=TimeConfigurationTest test +``` + +**Observed result** + +```text +Tests run: 1, Failures: 1, Errors: 0, Skipped: 0 +Expected 2026-08-15 but was 2026-08-14 because the production clock used UTC. +BUILD FAILURE +``` + +## GREEN + +**Command** + +```text +./mvnw -Dtest=TimeConfigurationTest test +``` + +**Observed result** + +```text +Tests run: 1, Failures: 0, Errors: 0, Skipped: 0 +BUILD SUCCESS +``` + +## Affected suite + +**Command and result** + +```text +./mvnw -Dtest=TimeConfigurationTest,BootstrapIntegrationTest,SmtpOnboardingWebIntegrationTest,AccountActivationIntegrationTest,AccountWebIntegrationTest,BootstrapOnboardingWebIntegrationTest,JavaMailSmtpProbeTest,SecurityResponseIntegrationTest test +Tests run: 22, Failures: 0, Errors: 0, Skipped: 0 +BUILD SUCCESS +PostgreSQL: 18.4 +``` + +## External-test boundaries + +This verifies the production clock zone and its midnight boundary. It does not exercise later scheduler behavior or +attendance-policy timezone versioning. diff --git a/docs/tests/web/account-uniqueness-feedback.md b/docs/tests/web/account-uniqueness-feedback.md new file mode 100644 index 0000000..9f53d19 --- /dev/null +++ b/docs/tests/web/account-uniqueness-feedback.md @@ -0,0 +1,75 @@ +# Test Evidence: Constraint-specific account uniqueness feedback + +- **Test type:** Web +- **Requirement IDs:** `ACC-019`, `DB-003` +- **Scenario IDs:** `AC-ACC-005` (Intern creation uniqueness boundary) +- **Test class/method:** `com.lab.labtimesheet.feature.account.controller.AccountWebIntegrationTest#duplicateNormalizedStudentCodeIsReportedOnStudentCodeRatherThanEmail` +- **Implementation commit:** `pending` + +## Protected behavior + +A case- and whitespace-normalized duplicate Intern student code is reported on the student-code field. A distinct +email is not falsely labeled as duplicate, and unknown uniqueness constraints fall back to a non-specific conflict. + +## Test method + +MockMvc creates one Intern through the authenticated CSRF-protected production form and then submits a second Intern +with a distinct email and the same student code in different case with surrounding whitespace. PostgreSQL 18.4 +enforces the real Flyway expression index; the controller maps Hibernate's known constraint name to the form field. + +## Hand-derived expected result + +The second request returns HTTP 200 on `accounts/new`, retains the safe display name, shows the student-code conflict, +and does not claim that the distinct email already exists. + +## RED + +**Command** + +```text +export JAVA_HOME=/opt/homebrew/opt/openjdk@25 +export PATH="$JAVA_HOME/bin:$PATH" +export DOCKER_HOST=unix:///Users/sechmachine/.orbstack/run/docker.sock +./mvnw -Dtest=AccountWebIntegrationTest#duplicateNormalizedStudentCodeIsReportedOnStudentCodeRatherThanEmail test +``` + +**Observed result** + +```text +Tests run: 1, Failures: 1, Errors: 0, Skipped: 0 +PostgreSQL reported uq_intern_profiles_student_code_ci, but the form displayed "this email already exists". +BUILD FAILURE +PostgreSQL: 18.4 +``` + +## GREEN + +**Command** + +```text +./mvnw -Dtest=AccountWebIntegrationTest#duplicateNormalizedStudentCodeIsReportedOnStudentCodeRatherThanEmail test +``` + +**Observed result** + +```text +Tests run: 1, Failures: 0, Errors: 0, Skipped: 0 +BUILD SUCCESS +PostgreSQL: 18.4 +``` + +## Affected suite + +**Command and result** + +```text +./mvnw -Dtest=TimeConfigurationTest,BootstrapIntegrationTest,SmtpOnboardingWebIntegrationTest,AccountActivationIntegrationTest,AccountWebIntegrationTest,BootstrapOnboardingWebIntegrationTest,JavaMailSmtpProbeTest,SecurityResponseIntegrationTest test +Tests run: 22, Failures: 0, Errors: 0, Skipped: 0 +BUILD SUCCESS +PostgreSQL: 18.4 +``` + +## External-test boundaries + +The test covers the two Platform-owned normalized identity constraints. It does not enumerate later-iteration feature +constraints or perform a real-browser accessibility pass. diff --git a/docs/tests/web/platform-onboarding-forms.md b/docs/tests/web/platform-onboarding-forms.md index 2caea33..3d8b693 100644 --- a/docs/tests/web/platform-onboarding-forms.md +++ b/docs/tests/web/platform-onboarding-forms.md @@ -18,14 +18,15 @@ non-secret values and show actionable errors. All state-changing browser operati MockMvc drives the production controllers, Bean Validation, Thymeleaf rendering, Spring Security filter chain, JPA services, and PostgreSQL 18.4. SMTP is replaced only at its network adapter. The tests inspect rendered status, buttons, warnings, validation messages, password non-retention, CSRF denial, ordered deferral navigation, and the -failed-probe response while verifying that activation remains unavailable. +failed-probe response while verifying that activation remains unavailable and raw adapter diagnostics are absent. ## Hand-derived expected result Successful bootstrap lands on `/admin/smtp?onboarding`. A saved draft shows Test but not Activate; a successful test shows Activate; activation clears the restricted warning. Deferral exposes warnings one through five in order, Back and Configure on every screen, and Finish only on screen five. Invalid data returns HTTP 200 with field/global errors -and no submitted password. A failed SMTP probe displays its safe error and leaves the draft untested. +and no submitted password. A failed SMTP probe displays fixed operator guidance and leaves the draft untested without +rendering the adapter's diagnostic. ## RED @@ -89,5 +90,5 @@ PostgreSQL: 18.4 ## External-test boundaries The SMTP adapter is in-memory here, so this does not prove external Mailpit/server interoperability. MockMvc is not a -real browser or accessibility run. The test exposes only the adapter's safe failure message fixture and never a raw -password, integration secret, or activation bearer token. +real browser or accessibility run. The test uses a non-secret diagnostic fixture only to prove that raw adapter text +is absent; it never exposes a password, integration secret, or activation bearer token. diff --git a/docs/tests/web/smtp-error-sanitization.md b/docs/tests/web/smtp-error-sanitization.md new file mode 100644 index 0000000..832bcc2 --- /dev/null +++ b/docs/tests/web/smtp-error-sanitization.md @@ -0,0 +1,75 @@ +# Test Evidence: Sanitized SMTP failure feedback + +- **Test type:** Web +- **Requirement IDs:** `INT-005`, `INT-008` +- **Scenario IDs:** `AC-INT-002` (failed-draft browser boundary) +- **Test class/method:** `com.lab.labtimesheet.feature.integration.controller.SmtpOnboardingWebIntegrationTest#failedSmtpTestRendersActionableFeedbackWithoutActivatingTheDraft` +- **Implementation commit:** `pending` + +## Protected behavior + +An SMTP test failure renders fixed actionable guidance but never renders the external adapter's arbitrary diagnostic. +The failed draft remains untested and cannot be activated. + +## Test method + +MockMvc saves a valid SMTP draft, configures the in-memory network adapter to throw a distinctive non-secret raw +diagnostic, and submits the authenticated CSRF-protected test action. It checks the production controller and +Thymeleaf response for the fixed message, absence of the raw diagnostic, and absence of the activation action. + +## Hand-derived expected result + +The response is HTTP 200 on `smtp/form`, contains the fixed operator message, omits the adapter diagnostic, and does +not offer Activate SMTP. + +## RED + +**Command** + +```text +export JAVA_HOME=/opt/homebrew/opt/openjdk@25 +export PATH="$JAVA_HOME/bin:$PATH" +export DOCKER_HOST=unix:///Users/sechmachine/.orbstack/run/docker.sock +./mvnw -Dtest=SmtpOnboardingWebIntegrationTest#failedSmtpTestRendersActionableFeedbackWithoutActivatingTheDraft test +``` + +**Observed result** + +```text +Tests run: 1, Failures: 1, Errors: 0, Skipped: 0 +The fixed guidance was absent and the rendered smtpActionError contained the adapter's distinctive diagnostic. +BUILD FAILURE +PostgreSQL: 18.4 +``` + +## GREEN + +**Command** + +```text +./mvnw -Dtest=SmtpOnboardingWebIntegrationTest#failedSmtpTestRendersActionableFeedbackWithoutActivatingTheDraft test +``` + +**Observed result** + +```text +Tests run: 1, Failures: 0, Errors: 0, Skipped: 0 +BUILD SUCCESS +PostgreSQL: 18.4 +``` + +## Affected suite + +**Command and result** + +```text +./mvnw -Dtest=TimeConfigurationTest,BootstrapIntegrationTest,SmtpOnboardingWebIntegrationTest,AccountActivationIntegrationTest,AccountWebIntegrationTest,BootstrapOnboardingWebIntegrationTest,JavaMailSmtpProbeTest,SecurityResponseIntegrationTest test +Tests run: 22, Failures: 0, Errors: 0, Skipped: 0 +BUILD SUCCESS +PostgreSQL: 18.4 +``` + +## External-test boundaries + +The SMTP adapter is in-memory, so this does not prove live server interoperability. The diagnostic is a deterministic +non-secret fixture; no password, credential, or activation token is logged or recorded. diff --git a/src/main/java/com/lab/labtimesheet/config/TimeConfiguration.java b/src/main/java/com/lab/labtimesheet/config/TimeConfiguration.java index a942085..426a443 100644 --- a/src/main/java/com/lab/labtimesheet/config/TimeConfiguration.java +++ b/src/main/java/com/lab/labtimesheet/config/TimeConfiguration.java @@ -1,15 +1,18 @@ package com.lab.labtimesheet.config; import java.time.Clock; +import java.time.ZoneId; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; -/** Provides the injectable UTC clock used for server-authoritative business time. */ +/** Provides the injectable Vietnam-zone clock used for server-authoritative business dates and time. */ @Configuration(proxyBeanMethods = false) class TimeConfiguration { + private static final ZoneId BUSINESS_ZONE = ZoneId.of("Asia/Ho_Chi_Minh"); + @Bean Clock applicationClock() { - return Clock.systemUTC(); + return Clock.system(BUSINESS_ZONE); } } diff --git a/src/main/java/com/lab/labtimesheet/feature/account/controller/AccountController.java b/src/main/java/com/lab/labtimesheet/feature/account/controller/AccountController.java index 3039e37..7f1eb31 100644 --- a/src/main/java/com/lab/labtimesheet/feature/account/controller/AccountController.java +++ b/src/main/java/com/lab/labtimesheet/feature/account/controller/AccountController.java @@ -6,6 +6,7 @@ import com.lab.labtimesheet.feature.account.model.dto.ActivationForm; import com.lab.labtimesheet.feature.account.model.dto.CreateAccountForm; import com.lab.labtimesheet.feature.account.service.AccountService; import jakarta.validation.Valid; +import org.hibernate.exception.ConstraintViolationException; import org.springframework.dao.DataIntegrityViolationException; import org.springframework.stereotype.Controller; import org.springframework.ui.Model; @@ -14,7 +15,10 @@ import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.ModelAttribute; import org.springframework.web.bind.annotation.PostMapping; -/** Handles Admin account creation and single-use account activation browser flows. */ +/** + * Handles Admin account creation and single-use account activation browser flows. Known database uniqueness + * constraints are mapped to their owning form fields without exposing persistence diagnostics. + */ @Controller class AccountController { private final AccountService accounts; @@ -43,7 +47,7 @@ class AccountController { ? "redirect:/admin/accounts/new?created" : "redirect:/admin/accounts/new?deliveryFailed"; } catch (DataIntegrityViolationException duplicate) { - bindingResult.rejectValue("email", "account.email.duplicate", "An account with this email already exists"); + rejectUniquenessViolation(bindingResult, duplicate); return "accounts/new"; } catch (IllegalArgumentException | IllegalStateException exception) { bindingResult.reject("account.invalid", exception.getMessage()); @@ -76,4 +80,29 @@ class AccountController { form.clearPasswords(); return "accounts/activate"; } + + private static void rejectUniquenessViolation(BindingResult bindingResult, + DataIntegrityViolationException violation) { + String constraintName = constraintName(violation); + if ("uq_app_users_email_ci".equals(constraintName)) { + bindingResult.rejectValue( + "email", "account.email.duplicate", "An account with this email already exists"); + } else if ("uq_intern_profiles_student_code_ci".equals(constraintName)) { + bindingResult.rejectValue("studentCode", "account.studentCode.duplicate", + "An Intern with this student code already exists"); + } else { + bindingResult.reject("account.unique", "Account details conflict with an existing account"); + } + } + + private static String constraintName(Throwable failure) { + Throwable current = failure; + while (current != null) { + if (current instanceof ConstraintViolationException violation) { + return violation.getConstraintName(); + } + current = current.getCause(); + } + return null; + } } diff --git a/src/main/java/com/lab/labtimesheet/feature/integration/controller/SmtpController.java b/src/main/java/com/lab/labtimesheet/feature/integration/controller/SmtpController.java index d44a800..79954a5 100644 --- a/src/main/java/com/lab/labtimesheet/feature/integration/controller/SmtpController.java +++ b/src/main/java/com/lab/labtimesheet/feature/integration/controller/SmtpController.java @@ -19,11 +19,16 @@ import org.springframework.web.bind.annotation.RequestMapping; /** * Runs the Admin SMTP draft, connection-test, activation, and ordered setup-deferral browser workflows. - * Cleartext passwords remain request-local and are cleared before any error view is rendered. + * Cleartext passwords remain request-local and are cleared before any error view is rendered. Failures crossing the + * SMTP adapter boundary are represented by fixed operator guidance rather than raw provider diagnostics. */ @Controller @RequestMapping("/admin/smtp") class SmtpController { + private static final String TEST_FAILURE_MESSAGE = + "SMTP test failed. Verify the draft settings and server availability, then try again."; + private static final String ACTIVATION_FAILURE_MESSAGE = + "SMTP activation failed. Test the current draft again before activating it."; private static final String DEFERRAL_STEP = SmtpController.class.getName() + ".deferralStep"; private static final List DEFERRAL_WARNINGS = List.of( "Account onboarding is disabled until SMTP is active.", @@ -72,7 +77,7 @@ class SmtpController { smtp.testDraft(action.getDraftId(), adminId(principal), principal.getName()); return "redirect:/admin/smtp?tested"; } catch (IllegalArgumentException | IllegalStateException failure) { - bindingResult.reject("smtp.test.failed", failure.getMessage()); + bindingResult.reject("smtp.test.failed", TEST_FAILURE_MESSAGE); return renderActionError(model, bindingResult); } } @@ -87,7 +92,7 @@ class SmtpController { smtp.activate(action.getDraftId(), adminId(principal)); return "redirect:/admin/smtp?activated"; } catch (IllegalArgumentException | IllegalStateException failure) { - bindingResult.reject("smtp.activate.failed", failure.getMessage()); + bindingResult.reject("smtp.activate.failed", ACTIVATION_FAILURE_MESSAGE); return renderActionError(model, bindingResult); } } diff --git a/src/main/resources/templates/accounts/new.html b/src/main/resources/templates/accounts/new.html index 2208861..a17015e 100644 --- a/src/main/resources/templates/accounts/new.html +++ b/src/main/resources/templates/accounts/new.html @@ -26,6 +26,7 @@
Intern details +

diff --git a/src/test/java/com/lab/labtimesheet/config/TimeConfigurationTest.java b/src/test/java/com/lab/labtimesheet/config/TimeConfigurationTest.java new file mode 100644 index 0000000..7983d85 --- /dev/null +++ b/src/test/java/com/lab/labtimesheet/config/TimeConfigurationTest.java @@ -0,0 +1,21 @@ +package com.lab.labtimesheet.config; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.time.Clock; +import java.time.Instant; +import java.time.LocalDate; + +import org.junit.jupiter.api.Test; + +class TimeConfigurationTest { + @Test + void utcInstantAtVietnamMidnightUsesTheNewLocalBusinessDate() { + Clock applicationClock = new TimeConfiguration().applicationClock(); + Instant vietnamMidnight = Instant.parse("2026-08-14T17:00:00Z"); + + LocalDate businessDate = LocalDate.now(Clock.fixed(vietnamMidnight, applicationClock.getZone())); + + assertThat(businessDate).isEqualTo(LocalDate.of(2026, 8, 15)); + } +} diff --git a/src/test/java/com/lab/labtimesheet/feature/account/controller/AccountWebIntegrationTest.java b/src/test/java/com/lab/labtimesheet/feature/account/controller/AccountWebIntegrationTest.java index 9afb100..7730dee 100644 --- a/src/test/java/com/lab/labtimesheet/feature/account/controller/AccountWebIntegrationTest.java +++ b/src/test/java/com/lab/labtimesheet/feature/account/controller/AccountWebIntegrationTest.java @@ -176,6 +176,37 @@ class AccountWebIntegrationTest { .andExpect(content().string(org.hamcrest.Matchers.containsString("Duplicate Mentor"))); } + @Test + void duplicateNormalizedStudentCodeIsReportedOnStudentCodeRatherThanEmail() throws Exception { + mockMvc.perform(post("/admin/accounts") + .with(user("admin@example.com").roles("ADMIN")) + .with(csrf()) + .param("email", "first-intern@example.com") + .param("displayName", "First Intern") + .param("role", "INTERN") + .param("studentCode", "STU-ROUND-2") + .param("internshipStart", "2026-08-01") + .param("internshipEnd", "2026-12-31")) + .andExpect(status().is3xxRedirection()); + + mockMvc.perform(post("/admin/accounts") + .with(user("admin@example.com").roles("ADMIN")) + .with(csrf()) + .param("email", "second-intern@example.com") + .param("displayName", "Second Intern") + .param("role", "INTERN") + .param("studentCode", " stu-round-2 ") + .param("internshipStart", "2026-08-01") + .param("internshipEnd", "2026-12-31")) + .andExpect(status().isOk()) + .andExpect(view().name("accounts/new")) + .andExpect(content().string(org.hamcrest.Matchers.containsString( + "An Intern with this student code already exists"))) + .andExpect(content().string(org.hamcrest.Matchers.not( + org.hamcrest.Matchers.containsString("this email already exists")))) + .andExpect(content().string(org.hamcrest.Matchers.containsString("Second Intern"))); + } + @Test void additionalAdminActivatesAndAuthenticatesWithoutChangingTheFirstAdmin() throws Exception { mockMvc.perform(post("/admin/accounts") diff --git a/src/test/java/com/lab/labtimesheet/feature/integration/controller/SmtpOnboardingWebIntegrationTest.java b/src/test/java/com/lab/labtimesheet/feature/integration/controller/SmtpOnboardingWebIntegrationTest.java index abd88a2..8e8c115 100644 --- a/src/test/java/com/lab/labtimesheet/feature/integration/controller/SmtpOnboardingWebIntegrationTest.java +++ b/src/test/java/com/lab/labtimesheet/feature/integration/controller/SmtpOnboardingWebIntegrationTest.java @@ -157,7 +157,8 @@ class SmtpOnboardingWebIntegrationTest { .with(user("admin@example.com").roles("ADMIN"))) .andReturn().getResponse().getContentAsString(); String draftId = html.replaceAll("(?s).*name=\"draftId\" value=\"([0-9]+)\".*", "$1"); - probe.failureMessage = "Connection refused by the configured SMTP server"; + String rawDiagnostic = "AUTH rejected for smtp-secret-raw-diagnostic"; + probe.failureMessage = rawDiagnostic; mockMvc.perform(post("/admin/smtp/test") .with(user("admin@example.com").roles("ADMIN")) @@ -166,7 +167,9 @@ class SmtpOnboardingWebIntegrationTest { .andExpect(status().isOk()) .andExpect(view().name("smtp/form")) .andExpect(content().string(org.hamcrest.Matchers.containsString( - "Connection refused by the configured SMTP server"))) + "SMTP test failed. Verify the draft settings and server availability, then try again."))) + .andExpect(content().string(org.hamcrest.Matchers.not( + org.hamcrest.Matchers.containsString(rawDiagnostic)))) .andExpect(content().string(org.hamcrest.Matchers.not( org.hamcrest.Matchers.containsString("Activate SMTP")))); }