fix(platform): address round 2 review findings

This commit is contained in:
sechmachine
2026-08-15 03:13:12 +07:00
parent 98688dec7e
commit 06dba4fb13
11 changed files with 329 additions and 13 deletions
@@ -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);
}
}
@@ -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;
}
}
@@ -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<String> 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);
}
}
@@ -26,6 +26,7 @@
<fieldset>
<legend>Intern details</legend>
<label>Student code <input th:field="*{studentCode}" autocomplete="off"></label>
<p th:if="${#fields.hasErrors('studentCode')}" th:errors="*{studentCode}" role="alert"></p>
<label>Internship start <input th:field="*{internshipStart}" type="date"></label>
<label>Internship end <input th:field="*{internshipEnd}" type="date"></label>
</fieldset>
@@ -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));
}
}
@@ -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")
@@ -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"))));
}