mirror of
https://github.com/iflytek/skillhub.git
synced 2026-09-05 08:05:56 +00:00
test(security): verify retry row locking
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
This commit is contained in:
parent
697bb952a4
commit
6770be22c5
5 changed files with 150 additions and 12 deletions
|
|
@ -56,19 +56,18 @@ public class SecurityScanRetryAppService {
|
|||
.orElseThrow(() -> new DomainBadRequestException("error.skill.notFound", skillId));
|
||||
authorize(skill, userId, platformRoles, namespaceRoles);
|
||||
|
||||
SkillVersion observedVersion = skillVersionRepository.findById(versionId)
|
||||
.filter(candidate -> candidate.getSkillId().equals(skillId))
|
||||
SkillVersionStatus observedStatus = skillVersionRepository.findStatusByIdAndSkillId(versionId, skillId)
|
||||
.orElseThrow(() -> new DomainBadRequestException("error.skill.version.notFound", versionId));
|
||||
if (observedVersion.getStatus() != SkillVersionStatus.SCAN_FAILED
|
||||
&& observedVersion.getStatus() != SkillVersionStatus.SCANNING) {
|
||||
throw new DomainBadRequestException("error.security.scan.retry.status", observedVersion.getStatus());
|
||||
if (observedStatus != SkillVersionStatus.SCAN_FAILED
|
||||
&& observedStatus != SkillVersionStatus.SCANNING) {
|
||||
throw new DomainBadRequestException("error.security.scan.retry.status", observedStatus);
|
||||
}
|
||||
if (!securityScanService.isEnabled()) {
|
||||
throw new DomainBadRequestException("error.security.scan.retry.disabled");
|
||||
}
|
||||
|
||||
String bundleKey = bundleKey(skillId, versionId);
|
||||
if (observedVersion.getStatus() == SkillVersionStatus.SCAN_FAILED
|
||||
if (observedStatus == SkillVersionStatus.SCAN_FAILED
|
||||
&& !objectStorageService.exists(bundleKey)) {
|
||||
throw new DomainBadRequestException("error.security.scan.retry.bundleMissing");
|
||||
}
|
||||
|
|
|
|||
|
|
@ -64,7 +64,8 @@ class SecurityScanRetryAppServiceTest {
|
|||
|
||||
@Test
|
||||
void retry_asOwnerCreatesNewAttemptAndAuditLog() {
|
||||
given(skillVersionRepository.findById(42L)).willReturn(Optional.of(version));
|
||||
given(skillVersionRepository.findStatusByIdAndSkillId(42L, 8L))
|
||||
.willReturn(Optional.of(SkillVersionStatus.SCAN_FAILED));
|
||||
given(skillVersionRepository.findByIdForUpdate(42L)).willReturn(Optional.of(version));
|
||||
given(securityScanService.isEnabled()).willReturn(true);
|
||||
given(objectStorageService.exists("packages/8/42/bundle.zip")).willReturn(true);
|
||||
|
|
@ -84,7 +85,8 @@ class SecurityScanRetryAppServiceTest {
|
|||
|
||||
@Test
|
||||
void retry_allowsNamespaceAdminAndPlatformSecurityAdmin() {
|
||||
given(skillVersionRepository.findById(42L)).willReturn(Optional.of(version));
|
||||
given(skillVersionRepository.findStatusByIdAndSkillId(42L, 8L))
|
||||
.willReturn(Optional.of(SkillVersionStatus.SCAN_FAILED));
|
||||
given(skillVersionRepository.findByIdForUpdate(42L)).willReturn(Optional.of(version));
|
||||
given(securityScanService.isEnabled()).willReturn(true);
|
||||
given(objectStorageService.exists("packages/8/42/bundle.zip")).willReturn(true);
|
||||
|
|
@ -107,13 +109,14 @@ class SecurityScanRetryAppServiceTest {
|
|||
.isInstanceOf(DomainForbiddenException.class);
|
||||
|
||||
verify(skillVersionRepository, never()).findByIdForUpdate(any());
|
||||
verify(skillVersionRepository, never()).findById(any());
|
||||
verify(skillVersionRepository, never()).findStatusByIdAndSkillId(any(), any());
|
||||
}
|
||||
|
||||
@Test
|
||||
void retry_rejectsNonFailedVersion() {
|
||||
version.setStatus(SkillVersionStatus.PENDING_REVIEW);
|
||||
given(skillVersionRepository.findById(42L)).willReturn(Optional.of(version));
|
||||
given(skillVersionRepository.findStatusByIdAndSkillId(42L, 8L))
|
||||
.willReturn(Optional.of(SkillVersionStatus.PENDING_REVIEW));
|
||||
|
||||
assertThatThrownBy(() -> service.retry(
|
||||
8L, 42L, "owner-1", Set.of(), Map.of(), new AuditRequestContext(null, null)))
|
||||
|
|
@ -125,7 +128,8 @@ class SecurityScanRetryAppServiceTest {
|
|||
|
||||
@Test
|
||||
void retry_rejectsMissingStoredBundle() {
|
||||
given(skillVersionRepository.findById(42L)).willReturn(Optional.of(version));
|
||||
given(skillVersionRepository.findStatusByIdAndSkillId(42L, 8L))
|
||||
.willReturn(Optional.of(SkillVersionStatus.SCAN_FAILED));
|
||||
given(securityScanService.isEnabled()).willReturn(true);
|
||||
|
||||
assertThatThrownBy(() -> service.retry(
|
||||
|
|
@ -139,7 +143,8 @@ class SecurityScanRetryAppServiceTest {
|
|||
@Test
|
||||
void retry_whenAttemptAlreadyStartedReturnsCurrentStateWithoutDuplicateTask() {
|
||||
version.setStatus(SkillVersionStatus.SCANNING);
|
||||
given(skillVersionRepository.findById(42L)).willReturn(Optional.of(version));
|
||||
given(skillVersionRepository.findStatusByIdAndSkillId(42L, 8L))
|
||||
.willReturn(Optional.of(SkillVersionStatus.SCANNING));
|
||||
given(skillVersionRepository.findByIdForUpdate(42L)).willReturn(Optional.of(version));
|
||||
given(securityScanService.isEnabled()).willReturn(true);
|
||||
given(securityAuditRepository.findLatestActiveByVersionIdAndScannerType(42L, ScannerType.SKILL_SCANNER))
|
||||
|
|
|
|||
|
|
@ -0,0 +1,125 @@
|
|||
package com.iflytek.skillhub.service;
|
||||
|
||||
import com.iflytek.skillhub.domain.namespace.Namespace;
|
||||
import com.iflytek.skillhub.domain.skill.Skill;
|
||||
import com.iflytek.skillhub.domain.skill.SkillVersion;
|
||||
import com.iflytek.skillhub.domain.skill.SkillVersionStatus;
|
||||
import com.iflytek.skillhub.domain.skill.SkillVisibility;
|
||||
import com.iflytek.skillhub.domain.user.UserAccount;
|
||||
import com.iflytek.skillhub.infra.jpa.SkillVersionJpaRepository;
|
||||
import jakarta.persistence.EntityManager;
|
||||
import jakarta.persistence.PersistenceContext;
|
||||
import java.util.concurrent.CountDownLatch;
|
||||
import java.util.concurrent.Executors;
|
||||
import java.util.concurrent.TimeUnit;
|
||||
import org.junit.jupiter.api.Test;
|
||||
import org.springframework.beans.factory.annotation.Autowired;
|
||||
import org.springframework.boot.test.autoconfigure.jdbc.AutoConfigureTestDatabase;
|
||||
import org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTest;
|
||||
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.annotation.Propagation;
|
||||
import org.springframework.transaction.annotation.Transactional;
|
||||
import org.springframework.transaction.support.TransactionTemplate;
|
||||
import org.testcontainers.containers.PostgreSQLContainer;
|
||||
import org.testcontainers.junit.jupiter.Container;
|
||||
import org.testcontainers.junit.jupiter.Testcontainers;
|
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
|
||||
@DataJpaTest
|
||||
@AutoConfigureTestDatabase(replace = AutoConfigureTestDatabase.Replace.NONE)
|
||||
@ActiveProfiles("test")
|
||||
@Testcontainers
|
||||
class SecurityScanRetryLockingTest {
|
||||
|
||||
@Container
|
||||
private static final PostgreSQLContainer<?> POSTGRES =
|
||||
new PostgreSQLContainer<>("postgres:16-alpine");
|
||||
|
||||
@DynamicPropertySource
|
||||
static void configurePostgres(DynamicPropertyRegistry registry) {
|
||||
registry.add("spring.datasource.url", POSTGRES::getJdbcUrl);
|
||||
registry.add("spring.datasource.username", POSTGRES::getUsername);
|
||||
registry.add("spring.datasource.password", POSTGRES::getPassword);
|
||||
registry.add("spring.datasource.driver-class-name", () -> "org.postgresql.Driver");
|
||||
registry.add("spring.jpa.database-platform", () -> "org.hibernate.dialect.PostgreSQLDialect");
|
||||
}
|
||||
|
||||
@Autowired
|
||||
private SkillVersionJpaRepository skillVersionRepository;
|
||||
|
||||
@Autowired
|
||||
private PlatformTransactionManager transactionManager;
|
||||
|
||||
@PersistenceContext
|
||||
private EntityManager entityManager;
|
||||
|
||||
@Test
|
||||
@Transactional(propagation = Propagation.NOT_SUPPORTED)
|
||||
void lockReadSeesStateCommittedWhileWaitingInsteadOfCachedPreflightEntity() throws Exception {
|
||||
Fixture fixture = persistFailedVersion();
|
||||
Long versionId = fixture.versionId();
|
||||
CountDownLatch firstLocked = new CountDownLatch(1);
|
||||
CountDownLatch secondAboutToLock = new CountDownLatch(1);
|
||||
CountDownLatch releaseFirst = new CountDownLatch(1);
|
||||
TransactionTemplate transactions = new TransactionTemplate(transactionManager);
|
||||
|
||||
try (var executor = Executors.newVirtualThreadPerTaskExecutor()) {
|
||||
var first = executor.submit(() -> transactions.executeWithoutResult(status -> {
|
||||
SkillVersion version = skillVersionRepository.findByIdForUpdate(versionId).orElseThrow();
|
||||
version.setStatus(SkillVersionStatus.SCANNING);
|
||||
firstLocked.countDown();
|
||||
await(releaseFirst);
|
||||
}));
|
||||
|
||||
assertThat(firstLocked.await(10, TimeUnit.SECONDS)).isTrue();
|
||||
var second = executor.submit(() -> transactions.execute(status -> {
|
||||
assertThat(skillVersionRepository.findStatusByIdAndSkillId(versionId, fixture.skillId()))
|
||||
.contains(SkillVersionStatus.SCAN_FAILED);
|
||||
secondAboutToLock.countDown();
|
||||
return skillVersionRepository.findByIdForUpdate(versionId).orElseThrow().getStatus();
|
||||
}));
|
||||
|
||||
assertThat(secondAboutToLock.await(10, TimeUnit.SECONDS)).isTrue();
|
||||
releaseFirst.countDown();
|
||||
first.get();
|
||||
assertThat(second.get()).isEqualTo(SkillVersionStatus.SCANNING);
|
||||
}
|
||||
}
|
||||
|
||||
private Fixture persistFailedVersion() {
|
||||
TransactionTemplate transaction = new TransactionTemplate(transactionManager);
|
||||
return transaction.execute(status -> {
|
||||
UserAccount user = new UserAccount("retry-lock-user", "Retry Lock User", null, null);
|
||||
entityManager.persist(user);
|
||||
Namespace namespace = new Namespace("retry-lock", "Retry Lock", user.getId());
|
||||
entityManager.persist(namespace);
|
||||
entityManager.flush();
|
||||
Skill skill = new Skill(namespace.getId(), "retry-lock", user.getId(), SkillVisibility.PRIVATE);
|
||||
entityManager.persist(skill);
|
||||
entityManager.flush();
|
||||
SkillVersion version = new SkillVersion(skill.getId(), "1.0.0", user.getId());
|
||||
version.setStatus(SkillVersionStatus.SCAN_FAILED);
|
||||
entityManager.persist(version);
|
||||
entityManager.flush();
|
||||
return new Fixture(version.getId(), skill.getId());
|
||||
});
|
||||
}
|
||||
|
||||
private void await(CountDownLatch latch) {
|
||||
try {
|
||||
if (!latch.await(10, TimeUnit.SECONDS)) {
|
||||
throw new IllegalStateException("Timed out waiting for concurrent retry test");
|
||||
}
|
||||
} catch (InterruptedException e) {
|
||||
Thread.currentThread().interrupt();
|
||||
throw new IllegalStateException("Concurrent retry test interrupted", e);
|
||||
}
|
||||
}
|
||||
|
||||
private record Fixture(Long versionId, Long skillId) {
|
||||
}
|
||||
}
|
||||
|
|
@ -11,6 +11,11 @@ public interface SkillVersionRepository {
|
|||
default Optional<SkillVersion> findByIdForUpdate(Long id) {
|
||||
return findById(id);
|
||||
}
|
||||
default Optional<SkillVersionStatus> findStatusByIdAndSkillId(Long id, Long skillId) {
|
||||
return findById(id)
|
||||
.filter(version -> version.getSkillId().equals(skillId))
|
||||
.map(SkillVersion::getStatus);
|
||||
}
|
||||
List<SkillVersion> findByIdIn(List<Long> ids);
|
||||
List<SkillVersion> findBySkillIdIn(List<Long> skillIds);
|
||||
List<SkillVersion> findBySkillIdInAndStatus(List<Long> skillIds, SkillVersionStatus status);
|
||||
|
|
|
|||
|
|
@ -26,6 +26,10 @@ public interface SkillVersionJpaRepository extends JpaRepository<SkillVersion, L
|
|||
@Override
|
||||
@Query(value = "SELECT * FROM skill_version WHERE id = :id FOR UPDATE", nativeQuery = true)
|
||||
Optional<SkillVersion> findByIdForUpdate(@Param("id") Long id);
|
||||
|
||||
@Override
|
||||
@Query("SELECT version.status FROM SkillVersion version WHERE version.id = :id AND version.skillId = :skillId")
|
||||
Optional<SkillVersionStatus> findStatusByIdAndSkillId(@Param("id") Long id, @Param("skillId") Long skillId);
|
||||
List<SkillVersion> findByIdIn(List<Long> ids);
|
||||
List<SkillVersion> findBySkillId(Long skillId);
|
||||
List<SkillVersion> findBySkillIdIn(List<Long> skillIds);
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue