diff --git a/docs/verification/issue-640.md b/docs/verification/issue-640.md index fb1ebc0d..3fd2370b 100644 --- a/docs/verification/issue-640.md +++ b/docs/verification/issue-640.md @@ -38,7 +38,7 @@ They did not connect to or modify the shared test-environment database. | Scenario | Observable result | |---|---| -| Fresh migration | Flyway V44 applied successfully and created `identity_provider_state` | +| Fresh migration | Flyway V45 applied successfully and created `identity_provider_state` after the reserved V44 compliance index | | Fixed GitHub authority vector | `oauth2-github`, `https://github.com`, fingerprint `b2a93d58465e3de9e8b6cd127ba18425ae0f80c49c85f18f76086832923ca619`, state `READY` | | Concurrent first pin | Two application instances converged to one READY row with the same fingerprint; no unique-constraint error | | Legacy OAuth binding | Existing `identity_binding` row remained byte-for-byte equivalent while the provider moved through first pin to READY | @@ -48,8 +48,8 @@ They did not connect to or modify the shared test-environment database. | Stale READY mismatch window | Recovery returned 409, persisted `AUTHORITY_MISMATCH`, retained the pinned authority/fingerprint, and wrote no recovery audit | | Transaction rollback | A forced audit insert failure returned 500; the provider state update rolled back and no audit record was added | | Unknown provider routes | Authorization and callback routes returned 403 without an upstream redirect | -| V43 to V44 upgrade | A database initialized by `v0.2.15` upgraded successfully and retained its legacy OAuth binding | -| Mixed-version and rollback | Current and `v0.2.15` servers were simultaneously healthy against the V44 database; the old provider endpoint returned 200 | +| V43 to V45 upgrade | A database initialized by `v0.2.15` upgraded successfully through reserved V44 and retained its legacy OAuth binding | +| Mixed-version and rollback | Current and `v0.2.15` servers were simultaneously healthy against the V45 database; the old provider endpoint returned 200 | | Redis session compatibility | A local session created by `v0.2.15` was accepted by the current server for the same user | ## Remaining integration gate @@ -60,7 +60,7 @@ the merge commit, deploy them to the shared test environment, and verify: 1. health, login catalog, and local-password login through the configured test domain; 2. unknown provider authorization/callback rejection; -3. V44 migration and READY provider state in the shared database; +3. reserved V44 compatibility migration, V45 identity migration, and READY provider state in the shared database; 4. existing Redis sessions and OAuth bindings; 5. recovery authorization and audit behavior; 6. logs contain no credentials or unexpected identity errors. diff --git a/scripts/tests/identity-binding-v2-postgres-test.sh b/scripts/tests/identity-binding-v2-postgres-test.sh index 5b81074c..30f0d698 100755 --- a/scripts/tests/identity-binding-v2-postgres-test.sh +++ b/scripts/tests/identity-binding-v2-postgres-test.sh @@ -10,15 +10,29 @@ POSTGRES_PASSWORD="identity-v2-test-password" POSTGRES_DB="identity_v2" MAVEN_CACHE_DIR="${MAVEN_CACHE_DIR:-${HOME}/.m2}" +log() { + printf '[identity-binding-v2] %s\n' "$*" +} + cleanup() { + exit_code="$?" + if [[ "${exit_code}" -ne 0 ]]; then + log "failed with exit code ${exit_code}" + docker ps -a \ + --filter "label=skillhub.test.run=${RUN_ID}" \ + --format 'resource={{.Names}} status={{.Status}}' || true + docker logs "${POSTGRES_CONTAINER}" 2>&1 || true + fi docker rm -f "${POSTGRES_CONTAINER}" >/dev/null 2>&1 || true docker network rm "${NETWORK}" >/dev/null 2>&1 || true } trap cleanup EXIT +log "creating isolated Docker network ${NETWORK}" docker network create \ --label "skillhub.test.run=${RUN_ID}" \ "${NETWORK}" >/dev/null +log "starting isolated PostgreSQL container ${POSTGRES_CONTAINER}" docker run -d \ --name "${POSTGRES_CONTAINER}" \ --label "skillhub.test.run=${RUN_ID}" \ @@ -31,28 +45,43 @@ docker run -d \ -p 127.0.0.1::5432 \ postgres:16-alpine >/dev/null +log "waiting for PostgreSQL readiness" +postgres_ready="false" for _ in $(seq 1 60); do - if docker exec "${POSTGRES_CONTAINER}" \ + pid_one_comm="$( + docker exec "${POSTGRES_CONTAINER}" \ + cat /proc/1/comm 2>/dev/null || true + )" + if [[ "${pid_one_comm}" == "postgres" ]] \ + && docker exec "${POSTGRES_CONTAINER}" \ pg_isready -U "${POSTGRES_USER}" -d "${POSTGRES_DB}" \ >/dev/null 2>&1; then + postgres_ready="true" break fi sleep 1 done -docker exec "${POSTGRES_CONTAINER}" \ - pg_isready -U "${POSTGRES_USER}" -d "${POSTGRES_DB}" \ - >/dev/null +if [[ "${postgres_ready}" != "true" ]]; then + log "PostgreSQL did not become ready after entrypoint initialization" + exit 1 +fi +log "PostgreSQL is ready" run_test() { test_class="$1" flyway_target="${2:-}" java_version="" if command -v java >/dev/null 2>&1; then - java_version="$(java -version 2>&1 | head -n 1)" + java_version="$(java -version 2>&1)" fi if [[ "${java_version}" == *'"21.'* ]]; then host_port="$(docker port "${POSTGRES_CONTAINER}" 5432/tcp \ | sed -n 's/.*://p')" + if [[ -z "${host_port}" ]]; then + log "Docker did not publish a PostgreSQL host port" + return 1 + fi + log "running ${test_class} with host Java 21" ( cd "${REPO_ROOT}/server" IDENTITY_BINDING_V2_POSTGRES_URL="jdbc:postgresql://127.0.0.1:${host_port}/${POSTGRES_DB}" \ @@ -70,6 +99,7 @@ run_test() { return fi + log "running ${test_class} with containerized Java 21" mkdir -p "${MAVEN_CACHE_DIR}" docker run --rm \ --name "${RUN_ID}-java" \ @@ -98,7 +128,7 @@ run_test() { } run_test IdentityBindingV2MigrationPostgresTest -run_test IdentityBindingV2ContractPostgresTest 46 +run_test IdentityBindingV2ContractPostgresTest 47 run_test UserProfileFieldSourceMigrationPostgresTest -run_test IdentityBindingV2PostgresIntegrationTest 47 -run_test IdentityProfileProvisioningPostgresIntegrationTest 47 +run_test IdentityBindingV2PostgresIntegrationTest 48 +run_test IdentityProfileProvisioningPostgresIntegrationTest 48 diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/AsyncConfig.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/AsyncConfig.java index 7feb8955..2b0efdba 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/AsyncConfig.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/config/AsyncConfig.java @@ -1,14 +1,16 @@ package com.iflytek.skillhub.config; +import java.util.Map; +import java.util.concurrent.Executor; +import java.util.concurrent.ThreadPoolExecutor; +import org.slf4j.MDC; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.core.task.TaskDecorator; import org.springframework.scheduling.annotation.EnableAsync; import org.springframework.scheduling.annotation.EnableScheduling; import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor; -import java.util.concurrent.Executor; -import java.util.concurrent.ThreadPoolExecutor; - /** * Enables asynchronous event handling and other background execution features used by the * application module. @@ -26,7 +28,30 @@ public class AsyncConfig { executor.setQueueCapacity(100); executor.setThreadNamePrefix("skillhub-event-"); executor.setRejectedExecutionHandler(new ThreadPoolExecutor.CallerRunsPolicy()); + executor.setTaskDecorator(mdcTaskDecorator()); executor.initialize(); return executor; } + + private TaskDecorator mdcTaskDecorator() { + return task -> { + Map callerContext = MDC.getCopyOfContextMap(); + return () -> { + Map executorContext = MDC.getCopyOfContextMap(); + try { + restoreMdc(callerContext); + task.run(); + } finally { + restoreMdc(executorContext); + } + }; + }; + } + + private void restoreMdc(Map context) { + MDC.clear(); + if (context != null) { + MDC.setContextMap(context); + } + } } diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/metrics/SkillHubMetrics.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/metrics/SkillHubMetrics.java index abbd05dd..17627c2a 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/metrics/SkillHubMetrics.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/metrics/SkillHubMetrics.java @@ -60,4 +60,8 @@ public class SkillHubMetrics { "operation", operation ).increment(); } + + public void incrementSearchRebuildFailure() { + meterRegistry.counter("skillhub.search.rebuild.failure").increment(); + } } diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/AdminUserAppService.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/AdminUserAppService.java index 38a0a6ba..d3dce6f5 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/AdminUserAppService.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/AdminUserAppService.java @@ -133,7 +133,7 @@ public class AdminUserAppService { String status, String actorUserId, AuditRequestContext auditContext) { - UserAccount user = loadUser(userId); + UserAccount user = loadUserForUpdate(userId); rejectSystemAccountMutation(user); UserStatus nextStatus = parseManageableStatus(status); UserStatus previousStatus = user.getStatus(); @@ -258,6 +258,13 @@ public class AdminUserAppService { .orElseThrow(() -> new DomainNotFoundException("error.admin.user.notFound", userId)); } + private UserAccount loadUserForUpdate(String userId) { + return userAccountRepository.findByIdForUpdate(userId) + .orElseThrow(() -> new DomainNotFoundException( + "error.admin.user.notFound", + userId)); + } + private void rejectSystemAccountMutation(UserAccount user) { if (user.isSystemAccount()) { throw new DomainForbiddenException("error.admin.user.systemAccount.immutable"); diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/LabelSearchSyncService.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/LabelSearchSyncService.java index 0dc9ce41..0a3ab591 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/LabelSearchSyncService.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/LabelSearchSyncService.java @@ -1,5 +1,6 @@ package com.iflytek.skillhub.service; +import com.iflytek.skillhub.metrics.SkillHubMetrics; import com.iflytek.skillhub.search.SearchRebuildService; import java.util.List; import org.slf4j.Logger; @@ -15,9 +16,12 @@ public class LabelSearchSyncService { private static final Logger log = LoggerFactory.getLogger(LabelSearchSyncService.class); private final SearchRebuildService searchRebuildService; + private final SkillHubMetrics metrics; - public LabelSearchSyncService(SearchRebuildService searchRebuildService) { + public LabelSearchSyncService(SearchRebuildService searchRebuildService, + SkillHubMetrics metrics) { this.searchRebuildService = searchRebuildService; + this.metrics = metrics; } @Async("skillhubEventExecutor") @@ -25,6 +29,7 @@ public class LabelSearchSyncService { try { searchRebuildService.rebuildBySkill(skillId); } catch (RuntimeException ex) { + metrics.incrementSearchRebuildFailure(); log.error("Failed to rebuild search document for skill {}", skillId, ex); } } @@ -41,6 +46,7 @@ public class LabelSearchSyncService { try { searchRebuildService.rebuildBySkill(skillId); } catch (RuntimeException ex) { + metrics.incrementSearchRebuildFailure(); log.error("Failed to rebuild search document for skill {} after label change", skillId, ex); } } diff --git a/server/skillhub-app/src/main/resources/db/migration/V44__skill_version_compliance_index.sql b/server/skillhub-app/src/main/resources/db/migration/V44__skill_version_compliance_index.sql new file mode 100644 index 00000000..e69d8458 --- /dev/null +++ b/server/skillhub-app/src/main/resources/db/migration/V44__skill_version_compliance_index.sql @@ -0,0 +1,3 @@ +CREATE INDEX IF NOT EXISTS idx_skill_version_compliance_mappings +ON skill_version +USING GIN ((parsed_metadata_json -> 'frontmatter' -> 'x-astron-compliance')); diff --git a/server/skillhub-app/src/main/resources/db/migration/V44__identity_provider_authority_state.sql b/server/skillhub-app/src/main/resources/db/migration/V45__identity_provider_authority_state.sql similarity index 100% rename from server/skillhub-app/src/main/resources/db/migration/V44__identity_provider_authority_state.sql rename to server/skillhub-app/src/main/resources/db/migration/V45__identity_provider_authority_state.sql diff --git a/server/skillhub-app/src/main/resources/db/migration/V45__identity_binding_v2_expand.sql b/server/skillhub-app/src/main/resources/db/migration/V46__identity_binding_v2_expand.sql similarity index 100% rename from server/skillhub-app/src/main/resources/db/migration/V45__identity_binding_v2_expand.sql rename to server/skillhub-app/src/main/resources/db/migration/V46__identity_binding_v2_expand.sql diff --git a/server/skillhub-app/src/main/resources/db/migration/V46__identity_binding_v2_contract_gate.sql b/server/skillhub-app/src/main/resources/db/migration/V47__identity_binding_v2_contract_gate.sql similarity index 98% rename from server/skillhub-app/src/main/resources/db/migration/V46__identity_binding_v2_contract_gate.sql rename to server/skillhub-app/src/main/resources/db/migration/V47__identity_binding_v2_contract_gate.sql index 211f6dd8..b3f28a30 100644 --- a/server/skillhub-app/src/main/resources/db/migration/V46__identity_binding_v2_contract_gate.sql +++ b/server/skillhub-app/src/main/resources/db/migration/V47__identity_binding_v2_contract_gate.sql @@ -1,7 +1,7 @@ -- Binding V2 contract gate. -- -- Deploy this migration only after every pre-Binding-V2 application instance --- has exited. Unlike the V45 expand migration, this gate rejects transactions +-- has exited. Unlike the V46 expand migration, this gate rejects transactions -- that leave an ACTIVE binding without exactly one ACTIVE primary subject. DO $$ diff --git a/server/skillhub-app/src/main/resources/db/migration/V47__user_profile_field_source.sql b/server/skillhub-app/src/main/resources/db/migration/V48__user_profile_field_source.sql similarity index 100% rename from server/skillhub-app/src/main/resources/db/migration/V47__user_profile_field_source.sql rename to server/skillhub-app/src/main/resources/db/migration/V48__user_profile_field_source.sql diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/identity/IdentityBindingV2ContractPostgresTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/identity/IdentityBindingV2ContractPostgresTest.java index e03807a8..a12c1f93 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/identity/IdentityBindingV2ContractPostgresTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/identity/IdentityBindingV2ContractPostgresTest.java @@ -37,7 +37,7 @@ class IdentityBindingV2ContractPostgresTest { .schemas(PREFLIGHT_SCHEMA) .defaultSchema(PREFLIGHT_SCHEMA) .createSchemas(true) - .target(MigrationVersion.fromVersion("45")) + .target(MigrationVersion.fromVersion("46")) .load() .migrate(); @@ -95,7 +95,7 @@ class IdentityBindingV2ContractPostgresTest { WHERE success = TRUE ORDER BY installed_rank DESC LIMIT 1 - """)).isEqualTo("45"); + """)).isEqualTo("46"); assertThat(singleLong( statement, """ @@ -126,7 +126,7 @@ class IdentityBindingV2ContractPostgresTest { database.username(), database.password()) .locations("classpath:db/migration") - .target(MigrationVersion.fromVersion("46")) + .target(MigrationVersion.fromVersion("47")) .load() .migrate(); diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/identity/IdentityBindingV2MigrationPostgresTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/identity/IdentityBindingV2MigrationPostgresTest.java index 6f1f1192..ad39bb2f 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/identity/IdentityBindingV2MigrationPostgresTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/identity/IdentityBindingV2MigrationPostgresTest.java @@ -41,7 +41,7 @@ class IdentityBindingV2MigrationPostgresTest { Flyway.configure() .dataSource(url, username, password) .locations("classpath:db/migration") - .target(MigrationVersion.fromVersion("44")) + .target(MigrationVersion.fromVersion("45")) .load() .migrate(); @@ -91,7 +91,7 @@ class IdentityBindingV2MigrationPostgresTest { Flyway.configure() .dataSource(url, username, password) .locations("classpath:db/migration") - .target(MigrationVersion.fromVersion("45")) + .target(MigrationVersion.fromVersion("46")) .load() .migrate(); @@ -237,7 +237,7 @@ class IdentityBindingV2MigrationPostgresTest { .schemas(PREFLIGHT_SCHEMA) .defaultSchema(PREFLIGHT_SCHEMA) .createSchemas(true) - .target(MigrationVersion.fromVersion("44")) + .target(MigrationVersion.fromVersion("45")) .load() .migrate(); @@ -315,7 +315,7 @@ class IdentityBindingV2MigrationPostgresTest { .schemas(PREFLIGHT_SCHEMA) .defaultSchema(PREFLIGHT_SCHEMA) .createSchemas(true) - .target(MigrationVersion.fromVersion("45")) + .target(MigrationVersion.fromVersion("46")) .load() .migrate()); @@ -343,7 +343,7 @@ class IdentityBindingV2MigrationPostgresTest { WHERE success = TRUE ORDER BY installed_rank DESC LIMIT 1 - """)).isEqualTo("44"); + """)).isEqualTo("45"); assertThat(singleLong( statement, """ diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/identity/IdentityProfileProvisioningPostgresIntegrationTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/identity/IdentityProfileProvisioningPostgresIntegrationTest.java index e9ff79f9..39f9b7c1 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/identity/IdentityProfileProvisioningPostgresIntegrationTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/identity/IdentityProfileProvisioningPostgresIntegrationTest.java @@ -24,6 +24,8 @@ import org.springframework.test.annotation.DirtiesContext; import org.springframework.test.context.ActiveProfiles; import org.springframework.test.context.DynamicPropertyRegistry; import org.springframework.test.context.DynamicPropertySource; +import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.support.TransactionTemplate; @SpringBootTest @ActiveProfiles("test") @@ -48,6 +50,12 @@ class IdentityProfileProvisioningPostgresIntegrationTest { @Autowired private AdminUserAppService adminUserAppService; + @Autowired + private IdentitySecurityAuditWriter securityAuditWriter; + + @Autowired + private PlatformTransactionManager transactionManager; + @Autowired private JdbcTemplate jdbcTemplate; @@ -333,6 +341,13 @@ class IdentityProfileProvisioningPostgresIntegrationTest { 'ACTIVE', CURRENT_TIMESTAMP, CURRENT_TIMESTAMP + ), ( + 'existing-collision-user-2', + 'Existing User 2', + 'collision@example.com', + 'ACTIVE', + CURRENT_TIMESTAMP, + CURRENT_TIMESTAMP ) """); @@ -357,7 +372,42 @@ class IdentityProfileProvisioningPostgresIntegrationTest { provider)).isZero(); assertThat(count( "SELECT COUNT(*) FROM user_account WHERE email = ?", - "collision@example.com")).isEqualTo(1L); + "collision@example.com")).isEqualTo(2L); + } + + @Test + void deniedSecurityAuditSurvivesCallerTransactionRollback() { + TransactionTemplate transactionTemplate = + new TransactionTemplate(transactionManager); + + assertThatThrownBy(() -> + transactionTemplate.executeWithoutResult(status -> { + securityAuditWriter.recordDenied( + "profile-auto", + "oidc", + IdentityFailureCode.ACCESS_DENIED, + new IdentityLoginContext( + "identity-denial-audit-rollback", + "127.0.0.1", + "identity-profile-test")); + throw new IllegalStateException( + "force caller rollback"); + })) + .isInstanceOf(IllegalStateException.class) + .hasMessage("force caller rollback"); + + assertThat(count( + """ + SELECT COUNT(*) + FROM audit_log + WHERE action = 'IDENTITY_LOGIN_DENIED' + AND request_id = + 'identity-denial-audit-rollback' + AND detail_json ->> 'providerCode' = + 'profile-auto' + AND detail_json ->> 'reason' = + 'ACCESS_DENIED' + """)).isEqualTo(1L); } @Test diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/identity/UserProfileFieldSourceMigrationPostgresTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/identity/UserProfileFieldSourceMigrationPostgresTest.java index 7fb876b7..b448f9b6 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/identity/UserProfileFieldSourceMigrationPostgresTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/auth/identity/UserProfileFieldSourceMigrationPostgresTest.java @@ -37,7 +37,7 @@ class UserProfileFieldSourceMigrationPostgresTest { .schemas(SCHEMA) .defaultSchema(SCHEMA) .createSchemas(true) - .target(MigrationVersion.fromVersion("46")) + .target(MigrationVersion.fromVersion("47")) .load() .migrate(); @@ -86,7 +86,7 @@ class UserProfileFieldSourceMigrationPostgresTest { .schemas(SCHEMA) .defaultSchema(SCHEMA) .createSchemas(true) - .target(MigrationVersion.fromVersion("47")) + .target(MigrationVersion.fromVersion("48")) .load() .migrate(); diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/config/AsyncConfigTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/config/AsyncConfigTest.java index a5ac41d6..9c8f4305 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/config/AsyncConfigTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/config/AsyncConfigTest.java @@ -2,9 +2,13 @@ package com.iflytek.skillhub.config; import static org.assertj.core.api.Assertions.assertThat; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; import org.junit.jupiter.api.Test; +import org.slf4j.MDC; import org.springframework.scheduling.annotation.EnableAsync; import org.springframework.scheduling.annotation.EnableScheduling; +import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor; class AsyncConfigTest { @@ -13,4 +17,26 @@ class AsyncConfigTest { assertThat(AsyncConfig.class).hasAnnotation(EnableAsync.class); assertThat(AsyncConfig.class).hasAnnotation(EnableScheduling.class); } + + @Test + void skillhubEventExecutor_propagatesAndClearsMdc() throws Exception { + ThreadPoolTaskExecutor executor = + (ThreadPoolTaskExecutor) new AsyncConfig().skillhubEventExecutor(); + try { + MDC.put("requestId", "req-597"); + CompletableFuture propagatedRequestId = new CompletableFuture<>(); + executor.execute(() -> propagatedRequestId.complete(MDC.get("requestId"))); + MDC.clear(); + + assertThat(propagatedRequestId.get(5, TimeUnit.SECONDS)).isEqualTo("req-597"); + + CompletableFuture nextRequestId = new CompletableFuture<>(); + executor.execute(() -> nextRequestId.complete(MDC.get("requestId"))); + + assertThat(nextRequestId.get(5, TimeUnit.SECONDS)).isNull(); + } finally { + MDC.clear(); + executor.shutdown(); + } + } } diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/db/FlywayMigrationGuardrailTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/db/FlywayMigrationGuardrailTest.java index eb8627af..7204d418 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/db/FlywayMigrationGuardrailTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/db/FlywayMigrationGuardrailTest.java @@ -102,11 +102,24 @@ class FlywayMigrationGuardrailTest { assertThat(migration).contains("bad_namespace.slug <> 'global'"); } + @Test + void complianceIndexMigration_mustMatchReservedV44() throws IOException { + String migration = Files.readString( + migrationPath( + "V44__skill_version_compliance_index.sql")); + + assertThat(migration).isEqualTo(""" + CREATE INDEX IF NOT EXISTS idx_skill_version_compliance_mappings + ON skill_version + USING GIN ((parsed_metadata_json -> 'frontmatter' -> 'x-astron-compliance')); + """); + } + @Test void identityBindingContractGate_mustRemainDeferred() throws IOException { String migration = Files.readString( migrationPath( - "V46__identity_binding_v2_contract_gate.sql")); + "V47__identity_binding_v2_contract_gate.sql")); assertThat(migration) .contains("CREATE CONSTRAINT TRIGGER") @@ -120,7 +133,7 @@ class FlywayMigrationGuardrailTest { throws IOException { String migration = Files.readString( migrationPath( - "V47__user_profile_field_source.sql")); + "V48__user_profile_field_source.sql")); assertThat(migration) .contains("LEGACY_LOCAL") diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/metrics/PrometheusEndpointTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/metrics/PrometheusEndpointTest.java index 38373f64..f52d66c7 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/metrics/PrometheusEndpointTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/metrics/PrometheusEndpointTest.java @@ -39,6 +39,7 @@ class PrometheusEndpointTest { skillHubMetrics.incrementUserRegister(); skillHubMetrics.recordLocalLogin(true); skillHubMetrics.incrementSkillPublish("global", "PENDING_REVIEW"); + skillHubMetrics.incrementSearchRebuildFailure(); assertThat(environment.getProperty("management.endpoints.web.exposure.include")) .doesNotContain("prometheus") @@ -54,5 +55,8 @@ class PrometheusEndpointTest { .tag("status", "PENDING_REVIEW") .counter() .count()).isEqualTo(1.0d); + assertThat(meterRegistry.get("skillhub.search.rebuild.failure") + .counter() + .count()).isEqualTo(1.0d); } } diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/AdminUserAppServiceTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/AdminUserAppServiceTest.java index f07b822b..8afa16da 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/AdminUserAppServiceTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/AdminUserAppServiceTest.java @@ -154,7 +154,7 @@ class AdminUserAppServiceTest { @Test void updateUserStatus_rejectsUnsupportedStatuses() { - when(userAccountRepository.findById("user-1")) + when(userAccountRepository.findByIdForUpdate("user-1")) .thenReturn(Optional.of(user("user-1", "alice", "alice@example.com", UserStatus.ACTIVE))); assertThrows(DomainBadRequestException.class, () -> service.updateUserStatus("user-1", "MERGED")); @@ -163,7 +163,7 @@ class AdminUserAppServiceTest { @Test void updateUserStatus_updatesPersistedStatus() { UserAccount user = user("user-1", "alice", "alice@example.com", UserStatus.ACTIVE); - when(userAccountRepository.findById("user-1")).thenReturn(Optional.of(user)); + when(userAccountRepository.findByIdForUpdate("user-1")).thenReturn(Optional.of(user)); when(userAccountRepository.save(user)).thenReturn(user); var response = service.updateUserStatus("user-1", "DISABLED"); @@ -177,7 +177,7 @@ class AdminUserAppServiceTest { @Test void updateUserStatus_activatingUserEnsuresGlobalMembership() { UserAccount user = user("user-1", "alice", "alice@example.com", UserStatus.PENDING); - when(userAccountRepository.findById("user-1")).thenReturn(Optional.of(user)); + when(userAccountRepository.findByIdForUpdate("user-1")).thenReturn(Optional.of(user)); when(userAccountRepository.save(user)).thenReturn(user); var response = service.updateUserStatus("user-1", "ACTIVE"); @@ -195,7 +195,7 @@ class AdminUserAppServiceTest { "alice", "alice@example.com", UserStatus.PENDING); - when(userAccountRepository.findById("user-1")) + when(userAccountRepository.findByIdForUpdate("user-1")) .thenReturn(Optional.of(user)); when(userAccountRepository.save(user)).thenReturn(user); @@ -226,7 +226,7 @@ class AdminUserAppServiceTest { @Test void updateUserStatus_rejectsReactivatingMergedAccount() { UserAccount user = user("user-1", "alice", "alice@example.com", UserStatus.MERGED); - when(userAccountRepository.findById("user-1")).thenReturn(Optional.of(user)); + when(userAccountRepository.findByIdForUpdate("user-1")).thenReturn(Optional.of(user)); assertThrows(DomainBadRequestException.class, () -> service.updateUserStatus("user-1", "ACTIVE")); @@ -238,7 +238,7 @@ class AdminUserAppServiceTest { @Test void updateUserStatus_rejectsSystemAccount() { - when(userAccountRepository.findById("builtin-skill-publisher")) + when(userAccountRepository.findByIdForUpdate("builtin-skill-publisher")) .thenReturn(Optional.of(systemUser())); assertThrows(DomainForbiddenException.class, @@ -249,7 +249,7 @@ class AdminUserAppServiceTest { @Test void updateUserStatus_withUnknownUser_throwsNotFound() { - when(userAccountRepository.findById("missing")).thenReturn(Optional.empty()); + when(userAccountRepository.findByIdForUpdate("missing")).thenReturn(Optional.empty()); assertThrows(DomainNotFoundException.class, () -> service.updateUserStatus("missing", "DISABLED")); } diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/LabelSearchSyncServiceTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/LabelSearchSyncServiceTest.java index c6859bed..ae0e5ac1 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/LabelSearchSyncServiceTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/LabelSearchSyncServiceTest.java @@ -1,13 +1,19 @@ package com.iflytek.skillhub.service; +import com.iflytek.skillhub.metrics.SkillHubMetrics; import com.iflytek.skillhub.search.SearchRebuildService; +import io.micrometer.core.instrument.simple.SimpleMeterRegistry; import org.junit.jupiter.api.Test; +import org.springframework.boot.test.context.runner.ApplicationContextRunner; import java.util.ArrayList; import java.util.List; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.verifyNoMoreInteractions; class LabelSearchSyncServiceTest { @@ -15,7 +21,8 @@ class LabelSearchSyncServiceTest { @Test void rebuildSkillsShouldSkipNullsAndDuplicatesWhileProcessingLargeLists() { SearchRebuildService rebuildService = mock(SearchRebuildService.class); - LabelSearchSyncService service = new LabelSearchSyncService(rebuildService); + SkillHubMetrics metrics = mock(SkillHubMetrics.class); + LabelSearchSyncService service = new LabelSearchSyncService(rebuildService, metrics); List skillIds = new ArrayList<>(); skillIds.add(null); for (long i = 1; i <= 120; i++) { @@ -30,5 +37,58 @@ class LabelSearchSyncServiceTest { verify(rebuildService).rebuildBySkill(i); } verifyNoMoreInteractions(rebuildService); + verifyNoInteractions(metrics); + } + + @Test + void rebuildSkillFailureShouldIncrementMetric() { + SearchRebuildService rebuildService = mock(SearchRebuildService.class); + doThrow(new IllegalStateException("search unavailable")) + .when(rebuildService) + .rebuildBySkill(42L); + + contextRunner(rebuildService).run(context -> { + LabelSearchSyncService service = context.getBean(LabelSearchSyncService.class); + SimpleMeterRegistry meterRegistry = context.getBean(SimpleMeterRegistry.class); + + service.rebuildSkill(42L); + + assertThat(meterRegistry.get("skillhub.search.rebuild.failure").counter().count()) + .isEqualTo(1.0d); + }); + } + + @Test + void rebuildSkillsShouldCountEachFailureAndContinue() { + SearchRebuildService rebuildService = mock(SearchRebuildService.class); + doThrow(new IllegalStateException("search unavailable")) + .when(rebuildService) + .rebuildBySkill(2L); + doThrow(new IllegalStateException("search unavailable")) + .when(rebuildService) + .rebuildBySkill(3L); + + contextRunner(rebuildService).run(context -> { + LabelSearchSyncService service = context.getBean(LabelSearchSyncService.class); + SimpleMeterRegistry meterRegistry = context.getBean(SimpleMeterRegistry.class); + + service.rebuildSkills(List.of(1L, 2L, 3L, 4L)); + + assertThat(meterRegistry.get("skillhub.search.rebuild.failure").counter().count()) + .isEqualTo(2.0d); + verify(rebuildService).rebuildBySkill(1L); + verify(rebuildService).rebuildBySkill(2L); + verify(rebuildService).rebuildBySkill(3L); + verify(rebuildService).rebuildBySkill(4L); + verifyNoMoreInteractions(rebuildService); + }); + } + + private ApplicationContextRunner contextRunner(SearchRebuildService rebuildService) { + return new ApplicationContextRunner() + .withBean(SearchRebuildService.class, () -> rebuildService) + .withBean(SimpleMeterRegistry.class) + .withBean(SkillHubMetrics.class) + .withBean(LabelSearchSyncService.class); } } diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/DefaultExternalIdentityLoginService.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/DefaultExternalIdentityLoginService.java index 94def7a7..42d8096d 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/DefaultExternalIdentityLoginService.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/DefaultExternalIdentityLoginService.java @@ -2,6 +2,8 @@ package com.iflytek.skillhub.auth.identity; import java.sql.SQLException; import java.util.Objects; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.dao.DataIntegrityViolationException; import org.springframework.stereotype.Service; @@ -9,23 +11,29 @@ import org.springframework.stereotype.Service; class DefaultExternalIdentityLoginService implements ExternalIdentityLoginService { + private static final Logger log = LoggerFactory.getLogger( + DefaultExternalIdentityLoginService.class); + private final TrustedProviderDescriptorSource descriptorSource; private final ProviderAuthorityLockService authorityLockService; private final IdentityAssertionFactory assertionFactory; private final IdentityResolutionTransaction resolutionTransaction; private final IdentityLoginMetrics metrics; + private final IdentitySecurityAuditWriter securityAuditWriter; DefaultExternalIdentityLoginService( TrustedProviderDescriptorSource descriptorSource, ProviderAuthorityLockService authorityLockService, IdentityAssertionFactory assertionFactory, IdentityResolutionTransaction resolutionTransaction, - IdentityLoginMetrics metrics) { + IdentityLoginMetrics metrics, + IdentitySecurityAuditWriter securityAuditWriter) { this.descriptorSource = descriptorSource; this.authorityLockService = authorityLockService; this.assertionFactory = assertionFactory; this.resolutionTransaction = resolutionTransaction; this.metrics = metrics; + this.securityAuditWriter = securityAuditWriter; } @Override @@ -38,10 +46,12 @@ class DefaultExternalIdentityLoginService Objects.requireNonNull(context, "context"); String metricProvider = "unresolved"; + String metricProtocol = "unresolved"; try { ProviderDescriptor descriptor = descriptorSource.require(provider); metricProvider = descriptor.providerCode(); + metricProtocol = descriptor.protocol(); authorityLockService.requirePinnedAuthority(descriptor); IdentityAssertion assertion = assertionFactory.create(descriptor, result); @@ -51,19 +61,48 @@ class DefaultExternalIdentityLoginService context); metrics.recordOutcome( descriptor.providerCode(), + descriptor.protocol(), outcome); return outcome; } catch (IdentityCoreException exception) { metrics.recordFailure( metricProvider, + metricProtocol, exception.getReasonCode()); + recordDeniedAudit( + metricProvider, + metricProtocol, + exception.getReasonCode(), + context); throw exception; } catch (RuntimeException exception) { - metrics.recordSystemError(metricProvider); + metrics.recordSystemError( + metricProvider, + metricProtocol); throw exception; } } + private void recordDeniedAudit( + String providerCode, + String protocol, + IdentityFailureCode failureCode, + IdentityLoginContext context) { + try { + securityAuditWriter.recordDenied( + providerCode, + protocol, + failureCode, + context); + } catch (RuntimeException auditFailure) { + log.error( + "Identity denial audit failed for provider '{}' and reason '{}'", + providerCode, + failureCode, + auditFailure); + } + } + private IdentityLoginOutcome resolveWithRetry( IdentityAssertion assertion, ProviderDescriptor descriptor, diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/IdentityLoginMetrics.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/IdentityLoginMetrics.java index 08a8eb2e..6886a23f 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/IdentityLoginMetrics.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/IdentityLoginMetrics.java @@ -15,6 +15,7 @@ class IdentityLoginMetrics { void recordOutcome( String providerCode, + String protocol, IdentityLoginOutcome outcome) { String result; if (outcome instanceof @@ -28,28 +29,35 @@ class IdentityLoginMetrics { } else { result = "link_required"; } - counter(providerCode, result); + counter(providerCode, protocol, result); } void recordFailure( String providerCode, + String protocol, IdentityFailureCode failureCode) { counter( providerCode, + protocol, failureCode.name().toLowerCase(Locale.ROOT)); } - void recordSystemError(String providerCode) { - counter(providerCode, "system_error"); + void recordSystemError( + String providerCode, + String protocol) { + counter(providerCode, protocol, "system_error"); } private void counter( String providerCode, + String protocol, String result) { meterRegistry.counter( "skillhub.identity.login", "provider", providerCode, + "protocol", + protocol, "result", result).increment(); } diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/IdentityResolutionTransaction.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/IdentityResolutionTransaction.java index bb921ee0..5496e010 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/IdentityResolutionTransaction.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/IdentityResolutionTransaction.java @@ -228,6 +228,11 @@ class IdentityResolutionTransaction { requireAllowed(decision); } + requireAccessAllowed( + assertion, + context, + IdentityAccessKind.RETURNING_IDENTITY, + Optional.of(user.getStatus())); if (decision == AccountLoginDecision.PENDING) { reconcileSubjects( binding, @@ -247,11 +252,6 @@ class IdentityResolutionTransaction { ACCOUNT_PENDING); } - requireAccessAllowed( - assertion, - context, - IdentityAccessKind.RETURNING_IDENTITY, - Optional.of(user.getStatus())); reconcileSubjects( binding, assertion, @@ -378,7 +378,7 @@ class IdentityResolutionTransaction { } Optional email = trustedEmail(assertion.profile()); - if (email.flatMap(userRepository::findByEmailIgnoreCase) + if (email.filter(userRepository::existsByEmailIgnoreCase) .isPresent()) { recordAudit( null, diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/IdentitySecurityAuditWriter.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/IdentitySecurityAuditWriter.java new file mode 100644 index 00000000..9174f4bb --- /dev/null +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/identity/IdentitySecurityAuditWriter.java @@ -0,0 +1,50 @@ +package com.iflytek.skillhub.auth.identity; + +import com.iflytek.skillhub.domain.audit.AuditLogService; +import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Propagation; +import org.springframework.transaction.annotation.Transactional; + +/** + * Persists security denials independently from the identity transaction that + * produced them. + */ +@Service +class IdentitySecurityAuditWriter { + + private final AuditLogService auditLogService; + + IdentitySecurityAuditWriter(AuditLogService auditLogService) { + this.auditLogService = auditLogService; + } + + @Transactional(propagation = Propagation.REQUIRES_NEW) + public void recordDenied( + String providerCode, + String protocol, + IdentityFailureCode failureCode, + IdentityLoginContext context) { + String action = switch (failureCode) { + case IDENTITY_IDENTIFIER_CONFLICT -> + "IDENTITY_CONFLICT_DETECTED"; + case PROVIDER_AUTHORITY_MISMATCH -> + "PROVIDER_AUTHORITY_MISMATCH"; + default -> "IDENTITY_LOGIN_DENIED"; + }; + auditLogService.record( + null, + action, + "IDENTITY_PROVIDER", + null, + context.requestId(), + context.clientIp(), + context.userAgent(), + "{\"providerCode\":\"" + + providerCode + + "\",\"protocol\":\"" + + protocol + + "\",\"reason\":\"" + + failureCode.name() + + "\"}"); + } +} diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/DefaultExternalIdentityLoginServiceTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/DefaultExternalIdentityLoginServiceTest.java index 30fb6b01..a4fa2b2d 100644 --- a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/DefaultExternalIdentityLoginServiceTest.java +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/DefaultExternalIdentityLoginServiceTest.java @@ -3,6 +3,7 @@ package com.iflytek.skillhub.auth.identity; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -26,6 +27,7 @@ class DefaultExternalIdentityLoginServiceTest { private ProviderAuthorityLockService authorityLockService; private IdentityResolutionTransaction resolutionTransaction; private IdentityLoginMetrics metrics; + private IdentitySecurityAuditWriter securityAuditWriter; private DefaultExternalIdentityLoginService service; @BeforeEach @@ -37,12 +39,15 @@ class DefaultExternalIdentityLoginServiceTest { resolutionTransaction = mock(IdentityResolutionTransaction.class); metrics = mock(IdentityLoginMetrics.class); + securityAuditWriter = + mock(IdentitySecurityAuditWriter.class); service = new DefaultExternalIdentityLoginService( descriptorSource, authorityLockService, new IdentityAssertionFactory(), resolutionTransaction, - metrics); + metrics, + securityAuditWriter); } @Test @@ -75,7 +80,10 @@ class DefaultExternalIdentityLoginServiceTest { any(IdentityAssertion.class), org.mockito.ArgumentMatchers.eq(descriptor), org.mockito.ArgumentMatchers.eq(context)); - order.verify(metrics).recordOutcome("github", expected); + order.verify(metrics).recordOutcome( + "github", + "oauth2-github", + expected); } @Test @@ -98,7 +106,10 @@ class DefaultExternalIdentityLoginServiceTest { IdentityLoginContext.empty()); assertThat(outcome).isSameAs(pending); - verify(metrics).recordOutcome("github", pending); + verify(metrics).recordOutcome( + "github", + "oauth2-github", + pending); } @Test @@ -126,7 +137,10 @@ class DefaultExternalIdentityLoginServiceTest { any(IdentityAssertion.class), org.mockito.ArgumentMatchers.eq(descriptor), any(IdentityLoginContext.class)); - verify(metrics).recordOutcome("github", expected); + verify(metrics).recordOutcome( + "github", + "oauth2-github", + expected); } @Test @@ -152,7 +166,44 @@ class DefaultExternalIdentityLoginServiceTest { .IDENTITY_IDENTIFIER_CONFLICT); verify(metrics).recordFailure( "github", + "oauth2-github", IdentityFailureCode.IDENTITY_IDENTIFIER_CONFLICT); + verify(securityAuditWriter).recordDenied( + "github", + "oauth2-github", + IdentityFailureCode.IDENTITY_IDENTIFIER_CONFLICT, + IdentityLoginContext.empty()); + } + + @Test + void auditFailureDoesNotReplaceIdentityDenial() { + ResolvedProviderHandle handle = + new DefaultResolvedProviderHandle("github"); + IdentityLoginContext context = + IdentityLoginContext.empty(); + when(descriptorSource.require(handle)) + .thenReturn(descriptor); + when(resolutionTransaction.resolve( + any(IdentityAssertion.class), + org.mockito.ArgumentMatchers.eq(descriptor), + org.mockito.ArgumentMatchers.eq(context))) + .thenThrow(new IdentityCoreException( + IdentityFailureCode.ACCESS_DENIED)); + doThrow(new IllegalStateException("audit unavailable")) + .when(securityAuditWriter) + .recordDenied( + "github", + "oauth2-github", + IdentityFailureCode.ACCESS_DENIED, + context); + + assertThatThrownBy(() -> service.authenticate( + handle, + result(), + context)) + .isInstanceOf(IdentityCoreException.class) + .extracting("reasonCode") + .isEqualTo(IdentityFailureCode.ACCESS_DENIED); } @Test @@ -182,7 +233,9 @@ class DefaultExternalIdentityLoginServiceTest { any(IdentityAssertion.class), org.mockito.ArgumentMatchers.eq(descriptor), any(IdentityLoginContext.class)); - verify(metrics).recordSystemError("github"); + verify(metrics).recordSystemError( + "github", + "oauth2-github"); } private static IdentityLoginOutcome authenticated() { diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/IdentityLoginMetricsTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/IdentityLoginMetricsTest.java index 4a0c34bf..1ddf3a77 100644 --- a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/IdentityLoginMetricsTest.java +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/IdentityLoginMetricsTest.java @@ -25,20 +25,24 @@ class IdentityLoginMetricsTest { metrics.recordOutcome( "github", + "oauth2-github", new IdentityLoginOutcome.Authenticated( principal, true, true)); metrics.recordOutcome( "github", + "oauth2-github", new IdentityLoginOutcome.PendingApproval( "ACCOUNT_PENDING")); metrics.recordOutcome( "github", + "oauth2-github", new IdentityLoginOutcome.LinkRequired( "EMAIL_COLLISION")); metrics.recordFailure( "github", + "oauth2-github", IdentityFailureCode.ACCESS_DENIED); assertThat(counter( @@ -62,6 +66,8 @@ class IdentityLoginMetricsTest { .tags( "provider", "github", + "protocol", + "oauth2-github", "result", result) .counter() diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/IdentityResolutionTransactionTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/IdentityResolutionTransactionTest.java index 3618afe4..862ee8e5 100644 --- a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/IdentityResolutionTransactionTest.java +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/IdentityResolutionTransactionTest.java @@ -221,12 +221,9 @@ class IdentityResolutionTransactionTest { @Test void verifiedEmailCollisionReturnsOnlyStableLinkReason() { - when(userRepository.findByEmailIgnoreCase( + when(userRepository.existsByEmailIgnoreCase( "alice@example.com")) - .thenReturn(Optional.of(user( - "usr_existing", - UserStatus.ACTIVE, - false))); + .thenReturn(true); IdentityLoginOutcome outcome = transaction.resolve( githubAssertion(Set.of()), @@ -537,6 +534,58 @@ class IdentityResolutionTransactionTest { .isEqualTo("original@example.com"); verify(userRepository, never()).save(user); verify(subjectRepository).saveAll(any()); + ArgumentCaptor accessContext = + ArgumentCaptor.forClass( + IdentityAccessContext.class); + verify(accessPolicy).evaluate(accessContext.capture()); + assertThat(accessContext.getValue().accessKind()) + .isEqualTo( + IdentityAccessKind.RETURNING_IDENTITY); + assertThat(accessContext.getValue() + .existingAccountStatus()) + .contains(UserStatus.PENDING); + } + + @Test + void deniedLoginPolicyDoesNotMutatePendingBinding() { + IdentityBinding binding = binding( + 1L, + "usr_1", + "github", + "123456"); + UserAccount user = user("usr_1", UserStatus.PENDING, false); + when(bindingRepository.findByProviderCodeAndSubject( + "github", + "123456")).thenReturn(Optional.of(binding)); + when(bindingRepository.findByIdAndStatusForUpdate( + 1L, + IdentityBindingStatus.ACTIVE)) + .thenReturn(Optional.of(binding)); + when(userRepository.findByIdForUpdate("usr_1")) + .thenReturn(Optional.of(user)); + when(accessPolicy.evaluate(any())) + .thenReturn(AccessDecision.DENY); + + assertThatThrownBy(() -> transaction.resolve( + githubAssertion(Set.of()), + githubDescriptor(ProvisioningMode.APPROVAL), + IdentityLoginContext.empty())) + .isInstanceOf(IdentityCoreException.class) + .extracting("reasonCode") + .isEqualTo(IdentityFailureCode.ACCESS_DENIED); + + verify(subjectRepository, never()) + .demoteActivePrimary(any(), any()); + verify(bindingRepository, never()).save(any()); + verify(auditLogService, never()).record( + any(), + any(), + any(), + any(), + any(), + any(), + any(), + any()); } @Test diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/IdentitySecurityAuditWriterTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/IdentitySecurityAuditWriterTest.java new file mode 100644 index 00000000..8338a2ec --- /dev/null +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/identity/IdentitySecurityAuditWriterTest.java @@ -0,0 +1,65 @@ +package com.iflytek.skillhub.auth.identity; + +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.isNull; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; + +import com.iflytek.skillhub.domain.audit.AuditLogService; +import org.junit.jupiter.api.Test; + +class IdentitySecurityAuditWriterTest { + + private final AuditLogService auditLogService = + mock(AuditLogService.class); + private final IdentitySecurityAuditWriter writer = + new IdentitySecurityAuditWriter(auditLogService); + + @Test + void mapsIdentifierConflictToDedicatedAuditAction() { + assertAuditAction( + IdentityFailureCode.IDENTITY_IDENTIFIER_CONFLICT, + "IDENTITY_CONFLICT_DETECTED"); + } + + @Test + void mapsAuthorityMismatchToDedicatedAuditAction() { + assertAuditAction( + IdentityFailureCode.PROVIDER_AUTHORITY_MISMATCH, + "PROVIDER_AUTHORITY_MISMATCH"); + } + + @Test + void mapsOtherDenialsToGenericAuditAction() { + assertAuditAction( + IdentityFailureCode.ACCESS_DENIED, + "IDENTITY_LOGIN_DENIED"); + } + + private void assertAuditAction( + IdentityFailureCode failureCode, + String expectedAction) { + writer.recordDenied( + "github", + "oauth2-github", + failureCode, + new IdentityLoginContext( + "request-1", + "127.0.0.1", + "identity-test")); + + verify(auditLogService).record( + isNull(), + eq(expectedAction), + eq("IDENTITY_PROVIDER"), + isNull(), + eq("request-1"), + eq("127.0.0.1"), + eq("identity-test"), + eq("{\"providerCode\":\"github\"," + + "\"protocol\":\"oauth2-github\"," + + "\"reason\":\"" + + failureCode.name() + + "\"}")); + } +} diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/user/UserAccountRepository.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/user/UserAccountRepository.java index 5c081c7a..46b9bb18 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/user/UserAccountRepository.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/user/UserAccountRepository.java @@ -14,6 +14,7 @@ public interface UserAccountRepository { Optional findByIdForUpdate(String id); List findByIdIn(List ids); Optional findByEmailIgnoreCase(String email); + boolean existsByEmailIgnoreCase(String email); Page search(String keyword, UserStatus status, Pageable pageable); UserAccount save(UserAccount user); } diff --git a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/UserAccountJpaRepository.java b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/UserAccountJpaRepository.java index 6c8011be..d4d1da81 100644 --- a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/UserAccountJpaRepository.java +++ b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/UserAccountJpaRepository.java @@ -3,26 +3,32 @@ package com.iflytek.skillhub.infra.jpa; import com.iflytek.skillhub.domain.user.UserAccount; import com.iflytek.skillhub.domain.user.UserAccountRepository; import com.iflytek.skillhub.domain.user.UserStatus; -import jakarta.persistence.LockModeType; import org.springframework.data.domain.Page; import org.springframework.data.domain.Pageable; import org.springframework.data.jpa.repository.JpaRepository; import org.springframework.data.jpa.repository.JpaSpecificationExecutor; -import org.springframework.data.jpa.repository.Lock; import org.springframework.data.jpa.repository.Query; import org.springframework.data.repository.query.Param; import org.springframework.stereotype.Repository; /** * JPA-backed user-account repository that provides filtered admin search over account records. + * The native {@code FOR UPDATE} query keeps row-lock behavior portable + * between PostgreSQL and the test suite's H2 PostgreSQL compatibility mode. */ @Repository public interface UserAccountJpaRepository extends JpaRepository, JpaSpecificationExecutor, UserAccountRepository { @Override - @Lock(LockModeType.PESSIMISTIC_WRITE) - @Query("SELECT u FROM UserAccount u WHERE u.id = :id") + @Query( + value = """ + SELECT * + FROM user_account + WHERE id = :id + FOR UPDATE + """, + nativeQuery = true) java.util.Optional findByIdForUpdate( @Param("id") String id);