From c8f1e79b41ab99a6e95c440f497193b9b6523bdb Mon Sep 17 00:00:00 2001 From: felix021 Date: Tue, 1 Sep 2026 23:42:04 +0800 Subject: [PATCH] fix: address review findings for member overwrite - Overwrite inherits the target skill's visibility: the requested visibility must not change another owner's skill reach (PRIVATE request no longer hides a published skill or moves its pointer to an unpublished version) - Pick the first non-archived published record as overwrite target; reject with error.skill.publish.archived when only an archived record carries the slug - validateOnly mirrors publish target selection: archived check and version-exists check now apply to the overwrite target - Fix NamespaceRequest 4-arg call sites, web test fixtures (tsc), duplicate version-save capture, and super-admin membership lookup assertion - Add tests: non-member rejection, archived-target rejection, dry-run version conflict on target --- .../NamespacePortalCommandAppServiceTest.java | 2 +- .../skill/service/SkillPublishService.java | 55 +++++++-- .../service/SkillPublishServiceTest.java | 108 +++++++++++++++++- .../namespace/namespace-header.test.ts | 1 + web/src/features/review/review-paths.test.ts | 4 + web/src/pages/dashboard/my-namespaces.test.ts | 1 + 6 files changed, 156 insertions(+), 15 deletions(-) diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/NamespacePortalCommandAppServiceTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/NamespacePortalCommandAppServiceTest.java index 1f4ad1ac..928d54a1 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/NamespacePortalCommandAppServiceTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/NamespacePortalCommandAppServiceTest.java @@ -49,7 +49,7 @@ class NamespacePortalCommandAppServiceTest { @Test void createNamespace_requiresPlatformAdminRole() { - NamespaceRequest request = new NamespaceRequest("team-alpha", "Team Alpha", null); + NamespaceRequest request = new NamespaceRequest("team-alpha", "Team Alpha", null, null); PlatformPrincipal principal = new PlatformPrincipal( "user-1", "user-1", "user-1@example.com", "", "github", Set.of("USER") ); diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java index 4a3b4541..a78d83ea 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java @@ -51,6 +51,7 @@ import java.util.HexFormat; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.zip.ZipEntry; import java.util.zip.ZipOutputStream; @@ -234,6 +235,11 @@ public class SkillPublishService { if (resolvedSlug != null && errors.isEmpty()) { List existingSkills = skillRepository.findByNamespaceIdAndSlug(namespace.getId(), resolvedSlug); boolean overwriteAllowed = namespace.isAllowMemberOverwrite() && member.isPresent(); + // Mirror the publish target selection: first non-archived other-owner skill with a + // PUBLISHED version becomes the overwrite target, so version-exists and archived + // checks below apply to the record the publish would actually land on. + Skill overwriteTarget = null; + boolean onlyArchivedTarget = false; for (Skill existing : existingSkills) { if (existing.getOwnerId().equals(publisherId)) { if (existing.getStatus() == SkillStatus.ARCHIVED) { @@ -249,12 +255,27 @@ public class SkillPublishService { boolean hasPublished = !skillVersionRepository .findBySkillIdAndStatus(existing.getId(), SkillVersionStatus.PUBLISHED) .isEmpty(); - if (hasPublished && !overwriteAllowed) { - errors.add("Name conflict: slug \"" + resolvedSlug + "\" is already published by another user"); - break; + if (hasPublished) { + if (!overwriteAllowed) { + errors.add("Name conflict: slug \"" + resolvedSlug + "\" is already published by another user"); + break; + } + if (existing.getStatus() == SkillStatus.ARCHIVED) { + onlyArchivedTarget = true; + } else if (overwriteTarget == null) { + overwriteTarget = existing; + } } } } + if (overwriteTarget == null && onlyArchivedTarget) { + errors.add("Cannot publish to archived skill: " + resolvedSlug); + } else if (overwriteTarget != null && resolvedVersion != null) { + var targetVersion = skillVersionRepository.findBySkillIdAndVersion(overwriteTarget.getId(), resolvedVersion); + if (targetVersion.isPresent() && targetVersion.get().getStatus() == SkillVersionStatus.PUBLISHED) { + errors.add("Version already published: " + resolvedVersion); + } + } } // Warnings make valid=false: real publish rejects them when confirmWarnings=false, @@ -407,8 +428,11 @@ public class SkillPublishService { // When the namespace opts into allowMemberOverwrite and the publisher is a member, the // published skill owned by another user becomes the publish target instead: the new // version attaches to it so the (namespace, slug) coordinate keeps its identity and - // original owner (SkillVersion.createdBy still records the actual publisher). + // original owner (SkillVersion.createdBy still records the actual publisher). Archived + // records are never selected as target; when only an archived record carries the + // coordinate the publish is rejected below. Skill overwriteTarget = null; + boolean onlyArchivedTarget = false; for (Skill existing : existingSkills) { if (!existing.getOwnerId().equals(publisherId)) { boolean hasPublished = !skillVersionRepository @@ -423,17 +447,26 @@ public class SkillPublishService { throw new DomainBadRequestException("error.skill.publish.nameConflict", skillSlug); } } - if (overwriteTarget == null) { + if (existing.getStatus() == SkillStatus.ARCHIVED) { + onlyArchivedTarget = true; + } else if (overwriteTarget == null) { overwriteTarget = existing; } } } } + if (overwriteTarget == null && onlyArchivedTarget) { + throw new DomainBadRequestException("error.skill.publish.archived", skillSlug); + } // Find or create skill for current user Skill skill; + SkillVisibility effectiveVisibility = visibility; if (overwriteTarget != null) { skill = overwriteTarget; + // Overwriting updates someone else's coordinate: the requested visibility must not + // change the target skill's reach, so the target's current visibility wins. + effectiveVisibility = skill.getVisibility(); } else { skill = skillRepository.findByNamespaceIdAndSlugAndOwnerId(namespace.getId(), skillSlug, publisherId) .orElseGet(() -> { @@ -481,12 +514,12 @@ public class SkillPublishService { // 8. Create SkillVersion SkillVersion version = new SkillVersion(skill.getId(), metadata.version(), publisherId); - version.setRequestedVisibility(visibility); + version.setRequestedVisibility(effectiveVisibility); boolean autoPublish = forceAutoPublish || isSuperAdmin; if (autoPublish) { version.setStatus(SkillVersionStatus.PUBLISHED); version.setPublishedAt(currentTime()); - } else if (visibility == SkillVisibility.PRIVATE) { + } else if (effectiveVisibility == SkillVisibility.PRIVATE) { // PRIVATE skill goes to UPLOADED status, no review task created version.setStatus(SkillVersionStatus.UPLOADED); version.setPublishedAt(currentTime()); @@ -577,7 +610,7 @@ public class SkillPublishService { skillVersionRepository.save(version); // Create review task for PUBLIC/NAMESPACE_ONLY (not PRIVATE) - if (!autoPublish && visibility != SkillVisibility.PRIVATE) { + if (!autoPublish && effectiveVisibility != SkillVisibility.PRIVATE) { ReviewTask reviewTask = new ReviewTask( version.getId(), skill.getId(), namespace.getId(), version.getVersion(), publisherId); ReviewTask savedReviewTask = reviewTaskRepository.save(reviewTask); @@ -598,10 +631,10 @@ public class SkillPublishService { // 12. Update skill metadata and move the published pointer for auto-publish flows skill.setDisplayName(metadata.name()); skill.setSummary(metadata.description()); - if (autoPublish || visibility == SkillVisibility.PRIVATE) { + if (autoPublish || effectiveVisibility == SkillVisibility.PRIVATE) { // Update latestVersionId for autoPublish or PRIVATE skill (UPLOADED status) skill.setLatestVersionId(version.getId()); - skill.setVisibility(visibility); + skill.setVisibility(effectiveVisibility); } skill.setUpdatedBy(publisherId); skillRepository.save(skill); @@ -713,7 +746,7 @@ public class SkillPublishService { * member. With the setting off (the default) the owner-isolation behavior is unchanged for * everyone, including OWNER/ADMIN and SUPER_ADMIN. */ - private boolean canOverwriteOtherOwners(Namespace namespace, java.util.Optional membership) { + private boolean canOverwriteOtherOwners(Namespace namespace, Optional membership) { return namespace.isAllowMemberOverwrite() && membership.isPresent(); } diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java index f50e45df..ea7a8a25 100644 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java @@ -965,7 +965,8 @@ class SkillPublishServiceTest { ); assertEquals(SkillVersionStatus.PUBLISHED, result.version().getStatus()); - verify(namespaceMemberRepository, never()).findByNamespaceIdAndUserId(any(), any()); + // Membership is still looked up (it feeds the allowMemberOverwrite relaxation), + // but emptiness must be fine for SUPER_ADMIN. } @Test @@ -1429,7 +1430,7 @@ class SkillPublishServiceTest { when(skillVersionRepository.save(any(SkillVersion.class))).thenAnswer(invocation -> { SkillVersion saved = invocation.getArgument(0); if (saved.getId() == null) setId(saved, 10L); - savedVersions.add(saved); + if (!savedVersions.contains(saved)) savedVersions.add(saved); return saved; }); List savedSkills = new java.util.ArrayList<>(); @@ -1443,15 +1444,19 @@ class SkillPublishServiceTest { ); // Overwrite lands on the original owner's skill record: ownership preserved, - // actual publisher recorded on the version. + // actual publisher recorded on the version. The member-updated PUBLIC skill goes + // through review (no auto-publish), so visibility and the published pointer stay. assertNotNull(result); assertEquals("test-skill", result.slug()); + assertEquals(SkillVersionStatus.PENDING_REVIEW, savedVersions.get(0).getStatus()); assertEquals(1, savedVersions.size()); assertEquals(1L, savedVersions.get(0).getSkillId()); assertEquals(publisherId, savedVersions.get(0).getCreatedBy()); assertEquals(1, savedSkills.size()); assertEquals("user-100", savedSkills.get(0).getOwnerId()); assertEquals(publisherId, savedSkills.get(0).getUpdatedBy()); + assertEquals(SkillVisibility.PUBLIC, savedSkills.get(0).getVisibility()); + assertNull(savedSkills.get(0).getLatestVersionId()); } @Test @@ -1493,8 +1498,105 @@ class SkillPublishServiceTest { namespaceSlug, entries, publisherId, SkillVisibility.PRIVATE, Set.of() ); + // The overwrite inherits the target's visibility: even though the publisher asked for + // PRIVATE, the skill was already PRIVATE, so main's private-skill semantics apply and + // nothing about the coordinate's reach changes. assertNotNull(result); assertEquals("test-skill", result.slug()); + assertEquals(SkillVersionStatus.UPLOADED, result.version().getStatus()); + assertEquals("user-100", existingSkill.getOwnerId()); + assertEquals(SkillVisibility.PRIVATE, existingSkill.getVisibility()); + assertEquals(result.version().getId(), existingSkill.getLatestVersionId()); + } + + @Test + void testPublishFromEntries_ShouldRejectNonMemberEvenWhenNamespaceAllows() throws Exception { + String namespaceSlug = "test-ns"; + String publisherId = "user-200"; + String skillMdContent = "---\nname: test-skill\ndescription: Test\nversion: 1.0.0\n---\nBody"; + + PackageEntry skillMd = new PackageEntry("SKILL.md", skillMdContent.getBytes(), skillMdContent.length(), "text/markdown"); + List entries = List.of(skillMd); + + Namespace namespace = new Namespace(namespaceSlug, "Test NS", "user-1"); + setId(namespace, 1L); + namespace.setAllowMemberOverwrite(true); + + when(namespaceRepository.findBySlug(namespaceSlug)).thenReturn(Optional.of(namespace)); + when(namespaceMemberRepository.findByNamespaceIdAndUserId(any(), eq(publisherId))).thenReturn(Optional.empty()); + + DomainBadRequestException ex = assertThrows(DomainBadRequestException.class, () -> service.publishFromEntries( + namespaceSlug, entries, publisherId, SkillVisibility.PUBLIC, Set.of() + )); + assertEquals("error.skill.publish.publisher.notMember", ex.messageCode()); + } + + @Test + void testPublishFromEntries_ShouldRejectWhenOnlyArchivedSkillCarriesSlug() throws Exception { + String namespaceSlug = "test-ns"; + String publisherId = "user-200"; + String skillMdContent = "---\nname: test-skill\ndescription: Test\nversion: 1.0.0\n---\nBody"; + + PackageEntry skillMd = new PackageEntry("SKILL.md", skillMdContent.getBytes(), skillMdContent.length(), "text/markdown"); + List entries = List.of(skillMd); + + Namespace namespace = new Namespace(namespaceSlug, "Test NS", "user-1"); + setId(namespace, 1L); + namespace.setAllowMemberOverwrite(true); + NamespaceMember member = mock(NamespaceMember.class); + SkillMetadata metadata = new SkillMetadata("test-skill", "Test", "1.0.0", "Body", Map.of()); + + Skill existingSkill = new Skill(1L, "test-skill", "user-100", SkillVisibility.PUBLIC); + setId(existingSkill, 1L); + existingSkill.setStatus(SkillStatus.ARCHIVED); + SkillVersion publishedVersion = new SkillVersion(1L, "0.1.0", "user-100"); + publishedVersion.setStatus(SkillVersionStatus.PUBLISHED); + + when(namespaceRepository.findBySlug(namespaceSlug)).thenReturn(Optional.of(namespace)); + when(namespaceMemberRepository.findByNamespaceIdAndUserId(any(), eq(publisherId))).thenReturn(Optional.of(member)); + when(skillPackageValidator.validate(entries)).thenReturn(ValidationResult.pass()); + when(skillMetadataParser.parse(skillMdContent)).thenReturn(metadata); + when(prePublishValidator.validate(any())).thenReturn(ValidationResult.pass()); + when(skillRepository.findByNamespaceIdAndSlug(any(), eq("test-skill"))).thenReturn(List.of(existingSkill)); + when(skillVersionRepository.findBySkillIdAndStatus(1L, SkillVersionStatus.PUBLISHED)).thenReturn(List.of(publishedVersion)); + + DomainBadRequestException ex = assertThrows(DomainBadRequestException.class, () -> service.publishFromEntries( + namespaceSlug, entries, publisherId, SkillVisibility.PUBLIC, Set.of() + )); + assertEquals("error.skill.publish.archived", ex.messageCode()); + } + + @Test + void testValidateOnly_ShouldReportVersionConflictOnOverwriteTarget() throws Exception { + String namespaceSlug = "test-ns"; + String publisherId = "user-200"; + List entries = skillEntries("test-skill", "1.0.0"); + Namespace namespace = new Namespace(namespaceSlug, "Test NS", "user-1"); + setId(namespace, 1L); + namespace.setAllowMemberOverwrite(true); + NamespaceMember member = mock(NamespaceMember.class); + SkillMetadata metadata = new SkillMetadata("test-skill", "Test", "1.0.0", "Body", Map.of()); + + Skill existingSkill = new Skill(1L, "test-skill", "user-100", SkillVisibility.PUBLIC); + setId(existingSkill, 1L); + SkillVersion publishedVersion = new SkillVersion(1L, "1.0.0", "user-100"); + publishedVersion.setStatus(SkillVersionStatus.PUBLISHED); + + when(namespaceRepository.findBySlug(namespaceSlug)).thenReturn(Optional.of(namespace)); + when(namespaceMemberRepository.findByNamespaceIdAndUserId(any(), eq(publisherId))).thenReturn(Optional.of(member)); + when(skillPackageValidator.validate(entries)).thenReturn(ValidationResult.pass()); + when(skillMetadataParser.parse(anyString())).thenReturn(metadata); + when(prePublishValidator.validate(any())).thenReturn(ValidationResult.pass()); + when(skillRepository.findByNamespaceIdAndSlug(any(), eq("test-skill"))).thenReturn(List.of(existingSkill)); + when(skillVersionRepository.findBySkillIdAndStatus(1L, SkillVersionStatus.PUBLISHED)).thenReturn(List.of(publishedVersion)); + when(skillVersionRepository.findBySkillIdAndVersion(eq(1L), eq("1.0.0"))).thenReturn(Optional.of(publishedVersion)); + + SkillPublishService.DryRunResult result = service.validateOnly( + namespaceSlug, entries, publisherId, SkillVisibility.PUBLIC, Set.of() + ); + + assertFalse(result.valid()); + assertTrue(result.errors().stream().anyMatch(e -> e.contains("Version already published"))); } @Test diff --git a/web/src/features/namespace/namespace-header.test.ts b/web/src/features/namespace/namespace-header.test.ts index 3b081ff6..2bceea91 100644 --- a/web/src/features/namespace/namespace-header.test.ts +++ b/web/src/features/namespace/namespace-header.test.ts @@ -32,6 +32,7 @@ describe('NamespaceHeader', () => { status: 'ACTIVE', avatarUrl: 'https://example.com/avatar.png', description: 'Shared namespace for all skills', + allowMemberOverwrite: false, createdAt: '2026-03-23T00:00:00.000Z', } diff --git a/web/src/features/review/review-paths.test.ts b/web/src/features/review/review-paths.test.ts index 0a9dc531..28cd73da 100644 --- a/web/src/features/review/review-paths.test.ts +++ b/web/src/features/review/review-paths.test.ts @@ -46,6 +46,7 @@ describe('review-paths', () => { canArchive: false, canRestore: false, canDelete: false, + allowMemberOverwrite: false, currentUserRole: 'ADMIN', createdAt: '', }, @@ -61,6 +62,7 @@ describe('review-paths', () => { canArchive: false, canRestore: false, canDelete: false, + allowMemberOverwrite: false, currentUserRole: 'OWNER', createdAt: '', }, @@ -81,6 +83,7 @@ describe('review-paths', () => { canArchive: false, canRestore: false, canDelete: false, + allowMemberOverwrite: false, currentUserRole: 'MEMBER', createdAt: '', }, @@ -102,6 +105,7 @@ describe('review-paths', () => { canArchive: false, canRestore: false, canDelete: false, + allowMemberOverwrite: false, currentUserRole: 'ADMIN', createdAt: '', }, diff --git a/web/src/pages/dashboard/my-namespaces.test.ts b/web/src/pages/dashboard/my-namespaces.test.ts index f3044a6c..3ffa2cfc 100644 --- a/web/src/pages/dashboard/my-namespaces.test.ts +++ b/web/src/pages/dashboard/my-namespaces.test.ts @@ -95,6 +95,7 @@ function buildNamespace(overrides: Partial = {}): ManagedNames description: 'namespace', type: 'TEAM', status: 'ACTIVE', + allowMemberOverwrite: false, createdAt: '2026-05-07T00:00:00Z', immutable: false, canFreeze: false,