Concurrent publishes for the same (namespace_id, slug, owner_id) or
(skill_id, version) coordinate both pass the check-then-create reads and
race on the database unique constraints. The losing request surfaced an
unhandled DataIntegrityViolationException as HTTP 500.
Translate the constraint violation at both insert points into a
deterministic DomainBadRequestException (error.skill.publish.concurrentConflict),
matching the existing idiom in LabelDefinitionService/ReviewService/
PromotionService. No same-transaction re-read is attempted, so the losing
publish rolls back cleanly and returns a retryable conflict instead of a 500.
Add the i18n key (en/zh) and two unit tests covering the skill-insert and
version-insert races.
Closes#617
Signed-off-by: FenjuFu <92919259+FenjuFu@users.noreply.github.com>
audit_log.detail_json is a JSONB column, so the value has to be valid
JSON or the insert fails. It was built by string concatenation at every
call site, with three inconsistent levels of escaping: none at all in
ClawHubCompatAppService, DeviceAuthWebController, LabelAdminAppService
and SkillLabelAppService; quotes only in ReviewPortalAppService,
SkillLifecycleAppService, NamespaceGovernanceService and
SkillGovernanceService; quotes and backslashes in
PromotionPortalAppService.escapeJson.
None of the three escapes control characters, which JSON forbids raw
inside a string. A reviewer pressing Enter in a review comment therefore
produced a payload PostgreSQL rejects, and because the audit write
happens after the domain mutation, the review was already approved when
the request returned 500.
Add AuditDetail, which renders the payload through Jackson, and route
all 29 construction sites through it. 17 of those interpolate a string
value and are the actual defect surface; the numeric and constant ones
are converted too so there is one way to build audit detail and no
hand-rolled example left to copy.
SkillHardDeleteService.toAuditPayload already did this correctly with a
LinkedHashMap and an ObjectMapper; AuditDetail is that shape extracted.
The service itself is left alone rather than changing its constructor
signature for no behavior gain.
Output is byte-identical for values that were already escaped correctly,
so the existing exact-string assertions in AdminSearchControllerTest and
PromotionPortalAppServiceTest are unchanged. null still means "no
detail": the builder returns null rather than {} when no field is set.
Addresses the JSON half of #615. The transaction half -- the domain
mutation and the audit write not sharing one transaction -- is a
separate design decision about whether an audit failure should roll back
a review, and is not bundled here.
Signed-off-by: FenjuFu <fufenjupku@gmail.com>
Use a bounded safe YAML constructor, remove the polynomial placeholder regex, keep public label access GET-only with CSRF protection, validate CLI callbacks as loopback URL objects, and use cryptographic UUIDs for E2E identities.
Signed-off-by: FenjuFu <92919259+FenjuFu@users.noreply.github.com>
* feat(api): return skill labels from the skill listing endpoints on request
Skill labels were reachable only one skill at a time, through
/api/{v1,web}/skills/{namespace}/{slug}/labels, so a client rendering a list had
to issue a follow-up request per row.
Add includeLabels=true to GET /api/v1/skills and GET /api/web/skills. The labels
array is populated only when the parameter is set and left out of the payload
otherwise, so existing responses are byte-identical.
Labels for the whole page are resolved by SkillLabelProjectionService in three
queries — assignments, definitions, translations — rather than three per skill.
Closes#710
Signed-off-by: FenjuFu <fufenjupku@gmail.com>
* fix(api): use include parameter for skill labels
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
* test(api): reject unsupported include before search
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
---------
Signed-off-by: FenjuFu <fufenjupku@gmail.com>
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
Co-authored-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
* fix(security): publish scan task after transaction commit
SecurityScanService.triggerScan is @Transactional but published the Redis
Stream scan task inline, before the transaction committed. The stream
consumer could receive the task before the skill_version / security_audit
rows were visible, fail with "SkillVersion not found" / "SecurityAudit not
found", exhaust its immediate retries while the publishing transaction was
still open, and leave the committed version stuck in SCANNING.
Defer the publish to an afterCommit transaction synchronization so the
consumer only ever sees the task once the rows are committed and visible; on
rollback the task is never published. Falls back to an inline publish when
called outside a transaction.
Closes#612
Signed-off-by: FenjuFu <fufenjupku@gmail.com>
* test(security): cover scan task after-commit publishing
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
* refactor(security): hide scan publish transaction callback
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
---------
Signed-off-by: FenjuFu <fufenjupku@gmail.com>
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
Co-authored-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
A deployment that stands up its own organisation-wide namespace — to use
instead of the built-in global one — finds it invisible to everybody. The
namespace listing only returns namespaces the caller belongs to, and the
only thing that ever added members automatically was hard-wired to the slug
"global".
Make that list a setting. namespace.default-membership holds the slugs every
newly activated account is enrolled in, defaulting to ["global"], which is
what every deployment did before. Publishing only requires membership of any
role, so being enrolled is enough to publish there; no extra grant needed.
GlobalNamespaceMembershipService becomes DefaultNamespaceMembershipService,
since it no longer means one specific namespace.
Where the strictness sits:
- Saving validates every slug resolves to an ACTIVE namespace, so a typo
fails at the moment an administrator makes it.
- Enrolling tolerates a slug that no longer resolves: it logs and skips.
A namespace that was deleted or renamed must not cost somebody their
login.
Adding a namespace to the list after people have signed up leaves them out,
the same trap the personal-namespace work hit, so this ships with the same
preview-then-apply backfill.
Verified against a real PostgreSQL, end to end: create a namespace, reject an
unknown slug, save with whitespace and duplicates and see them normalised,
preview, apply, re-preview showing nothing left, and a fresh registration
landing in the global namespace, the new shared one, and its own personal one
at once.
The backfill preview returned 500 on PostgreSQL:
SQLState 42883: function lower(bytea) does not exist
It reused UserAccountRepository.search(keyword, status, pageable) with a
null keyword. That query compares the keyword with lower(...), and a null
bind leaves PostgreSQL to infer the parameter type as bytea, so lower()
has no matching signature.
Nothing had exercised that branch before: the admin user list goes through
AdminUserSearchRepository, and the member-candidate lookup always passes a
real keyword. The backfill was the first caller to pass null.
Give callers that want every account in a status a query without a keyword
to bind, rather than papering over the null with a cast or an empty string.
Neither test layer would have caught this. The unit tests mock the
repository, and the integration tests run on H2 in PostgreSQL mode, which
accepts the null-typed bind that PostgreSQL rejects. Verified instead
against a real PostgreSQL: preview, apply, and a second preview showing
alreadyProvisioned with nothing left to do, with namespace_member rows
confirming each owner holds OWNER on a TEAM namespace.
Turning provisioning on only affects accounts activated afterwards, which
on a registry that has already been running means nobody. The first person
to hit this on our deployment was the operator who enabled it: they signed
in, got no namespace, and had no way to find out why.
Two fixes.
Backfill. POST /api/v1/admin/settings/personal-namespace/backfill walks the
active accounts and gives a namespace to those without one, skipping system
accounts and anyone who already owns a non-global namespace. Details worth
knowing:
- dryRun reports the plan — each account and the slug it would take —
without writing. The console requires a preview before it will enable the
apply button.
- The response lists only accounts that changed or could not be placed;
the rest are counted, so an operator reads the work rather than the whole
directory.
- A run stops at a per-run account cap and reports truncated rather than
looking like it covered everything.
- Slugs promised earlier in a run are reserved, so one batch cannot hand
the same slug to two accounts.
- Not @Transactional: each namespace is created in its own transaction, so
an account that cannot be placed does not discard the rest of the run.
Diagnosability. The skip paths — provisioning disabled, account already owns
a namespace, no slug available — were silent returns, which is why "nothing
happened and I cannot tell why" was the actual user experience. They now log
their reason; account activation is rare enough that the extra lines cost
nothing.
Self-hosted deployments want every new account to have somewhere of its
own to publish, without asking an administrator for a namespace first and
without pushing drafts into `global`.
Add an operator-controlled policy, off by default so upgrading changes no
behaviour. When enabled, an account that becomes usable gets a namespace
it owns. "Private" here means a team namespace whose only member is that
account: namespaces have no visibility flag, and skill visibility stays a
property of each skill.
Trigger points. UserActivatedEvent is published wherever an account first
becomes usable:
- LocalAuthService.register
- IdentityBindingService.bindOrCreate, for ACTIVE first logins
- AdminUserAppService.updateUserStatus, on a transition into ACTIVE
The third matters for deployments that gate access behind approval: those
accounts are created PENDING at the first OAuth attempt and only become
usable when an administrator approves them.
Why an AFTER_COMMIT listener rather than a call alongside
GlobalNamespaceMembershipService.ensureMember. Both namespace.created_by
and namespace_member.user_id reference user_account(id), which rules out
each obvious alternative:
- Joining the registration transaction lets a slug clash roll the
registration back, so a namespace failure costs the user their account
— or, on OAuth, their login.
- Suspending it with REQUIRES_NEW leaves the new transaction unable to see
the uncommitted user_account row, so the foreign key check blocks on the
outer transaction's row lock and the two wait on each other.
Provisioning after commit avoids both. The listener is deliberately not
@Async, so the namespace exists by the time the user's next request
arrives, and it swallows failures.
Naming. Two templates over ${username}, ${email_prefix} and ${user_id};
unknown placeholders are left in place so a typo is visible rather than
silently dropped. ${username} falls back to the email local part and then
to the user id. Slugs go through the existing slug rules, which is why the
console renders a live preview: underscores are not legal in a slug, so
`${username}_space` yields `alice-space`. A taken or reserved candidate
gets a numeric suffix, so `admin` becomes `admin-2`. Owning any non-global
namespace already skips provisioning, which keeps re-enabling an account
from handing out a second one.
The templates are not exposed in application.yml: they contain ${...},
which Spring would resolve as property references, and Boot 3.2 predates
placeholder escaping. Only the enable flag lives there; templates are set
in the console and default from PersonalNamespaceProvisioningProperties.
Updating the policy writes an audit entry with the before and after.
SkillHub has no mechanism for settings an operator can change without a
redeploy: the only per-deployment knobs live in application.yml, and the
only stored preferences are per-user notification preferences.
Add a generic store. One row holds one setting group serialized as JSON,
so a group can gain fields without a schema migration.
Reads take the caller's defaults:
<T> T get(String settingKey, Class<T> type, T defaults)
which gives two properties worth keeping:
- A group nobody has overridden has no row, and resolves to whatever the
deployment configured. Configuration-file-only deployments keep working
exactly as before, and an upgrade changes no behaviour.
- A stored document that can no longer be parsed also falls back to the
defaults, with a warning. One malformed row must not take down the flows
that read settings, such as login.
Groups are deserialized with unknown fields ignored so a rolling upgrade
can read documents written by a newer node.
No consumer yet; the following commit adds the first one.
Remove terminal review tasks before deleting an allowed skill version. Lock all versions of the aggregate in stable order so concurrent deletes preserve the last-version invariant and return business errors instead of 500 responses.
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
* fix(publish): delete review tasks of any status when replacing a version
Re-uploading a rejected version under the same version number returned
HTTP 500. deleteReplaceableVersionArtifacts only removed a PENDING review
task, but a rejected version owns a REJECTED one; that row kept a foreign
key on the skill_version, so the subsequent delete hit a constraint
violation that surfaced as a 500.
Delete every review task attached to the version instead.
Signed-off-by: FenjuFu <92919259+FenjuFu@users.noreply.github.com>
* test(publish): drop the spring-test dependency from the new test
skillhub-domain has no spring-test on its test classpath, so
ReflectionTestUtils does not resolve there. Use plain JDK reflection for
setting the generated id and invoking the private method.
Signed-off-by: FenjuFu <92919259+FenjuFu@users.noreply.github.com>
* fix(publish): constrain rejected version replacement
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
* test(publish): verify replaced review is deleted
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
* test(e2e): use generated API response types
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
---------
Signed-off-by: FenjuFu <92919259+FenjuFu@users.noreply.github.com>
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
Co-authored-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
