scriptoriumadmin/skillhub
1
0
Fork 0
mirror of https://github.com/iflytek/skillhub.git synced 2026-09-08 22:21:09 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

fix: address review findings for member overwrite

Browse source 
- 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
This commit is contained in:
felix021 2026-09-01 23:42:04 +08:00
parent ab7e02bda0
commit c8f1e79b41
6 changed files with 156 additions and 15 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
server/skillhub-app/src/test/java/com/iflytek/skillhub/service/NamespacePortalCommandAppServiceTest.java
View file

@ -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")
);

55
server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java
View file

@ -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<Skill> 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<NamespaceMember> membership) {
private boolean canOverwriteOtherOwners(Namespace namespace, Optional<NamespaceMember> membership) {
return namespace.isAllowMemberOverwrite() && membership.isPresent();
}

108
server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java
View file

@ -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<Skill> 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<PackageEntry> 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<PackageEntry> 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<PackageEntry> 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

1
web/src/features/namespace/namespace-header.test.ts
View file

@ -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',
}

4
web/src/features/review/review-paths.test.ts
View file

@ -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: '',
},

1
web/src/pages/dashboard/my-namespaces.test.ts
View file

@ -95,6 +95,7 @@ function buildNamespace(overrides: Partial<ManagedNamespace> = {}): ManagedNames
description: 'namespace',
type: 'TEAM',
status: 'ACTIVE',
allowMemberOverwrite: false,
createdAt: '2026-05-07T00:00:00Z',
immutable: false,
canFreeze: false,