feat(namespace): auto-provision a personal namespace on registration - #712
Open
Vast-Stars wants to merge 7 commits into
Open
feat(namespace): auto-provision a personal namespace on registration#712Vast-Stars wants to merge 7 commits into
Vast-Stars wants to merge 7 commits into
Conversation
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.
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.
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.
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.
Enabling personal namespace provisioning appeared to save — the request
succeeded and the row held enabled=true — but a refresh showed it disabled
again, and the template inputs stayed greyed out.
The form mounted before the fetched settings reached it. Radix's Select
keeps a hidden native <select> for form integration, and its <option>s only
exist while the dropdown content is mounted. Changing the controlled value
from "disabled" to "enabled" therefore assigned a value that native select
had no option for, which lands on "" and fires a real change event. Radix
forwarded it as onValueChange(""), which read as "disabled" and put the
form straight back where it started.
Hold the form state as null until the settings arrive, so the Select mounts
once with the value it will keep and the controlled value never changes
underneath it. Also ignore any value that is not one of the two real
choices, so a stray event cannot decide the setting.
The page's other Select-bearing sibling never hit this because it lives in
a dialog whose form state is set before the dialog mounts.
The old test rendered with renderToStaticMarkup, which never runs effects
and so could not see this at all. The page tests now run in jsdom via
@testing-library/react; the new one was confirmed to fail against the
previous code and pass against this one. Also verified in a browser against
a real backend: enable, save, reload, disable, save, reload.
|
|
1 similar comment
|
|
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.
Author
|
Pushed another commit: Same story as the backfill commit — found by running this on our own intranet A namespace you create to replace This makes that list a setting:
Publishing only requires membership of any role, so being enrolled is enough Verified against a real PostgreSQL end to end: create a namespace, reject an As before, happy to split this out if you would rather keep the PR narrower. |
The setting took a comma-separated string, which puts the operator in the position of remembering exact slugs and getting the punctuation right. The server rejects a bad slug, but only after a round trip, and only for the first mistake in the list. Offer the active namespaces as checkboxes instead. Nothing to spell. One case the list alone would get wrong: a slug that is configured but has since been deleted, archived or renamed is not among the choices, so rendering only the choices would quietly drop it on the next save. Those are appended as their own rows, ticked and flagged, so dropping one is a decision rather than a side effect. The choice list is capped, and a directory larger than the cap says so rather than presenting a partial list as complete.
Member
|
Reviewed against #711 — the design lines up with what the issue asked for: operator-controlled, editable slug/name templates, off by default so existing deployments are unchanged on upgrade, and the "personal namespace = TEAM namespace with a single OWNER member" framing matches the current data model rather than inventing a visibility concept. The Two blockers before review can proceed:
On the two-commit structure: keeping them in one PR is fine, the settings store reads better with its first consumer attached. 已对照 #711 核对：由运营方开关控制、slug/名称模板可配、默认关闭因此升级不影响存量部署，以及把"个人命名空间"落成"仅有一个 OWNER 成员的 TEAM 命名空间"而不新造可见性概念 —— 都符合 issue 的诉求， 评审前有两个阻塞项：
关于两个 commit 的拆分：放在同一个 PR 里没问题，设置存储配上首个消费方更好判断。 |
FenjuFu
reviewed
Aug 20, 2026
FenjuFu
left a comment
FenjuFu
left a comment
Member
There was a problem hiding this comment.
Reviewed the provisioning core against #711. The engineering here is solid — in particular the parts that are easy to get wrong are handled correctly:
- After-commit transaction: the listener fires on
@TransactionalEventListener(AFTER_COMMIT), andprovisionForis annotated@Transactional(propagation = REQUIRES_NEW). That is exactly right — a plain@Transactionalthere would join an already-committed context and silently drop the namespace write. Good catch. - Idempotency:
if (alreadyOwnsNamespace(owner.userId())) return emptyguards both the listener path and the backfill against duplicate namespaces. - Collision + fail-open:
MAX_SLUG_ATTEMPTSslug allocation returns empty (logged) rather than throwing, and the listener swallowsRuntimeException, so a naming clash or DB hiccup costs the user a namespace, not their registration/login. The backfill is batched withMAX_BACKFILL_ACCOUNTSand skips system/already-owning accounts. - Operator-gated behind
settings.enabled(), with admin settings + backfill endpoints.
Blocking: DCO is currently red (ACTION_REQUIRED) — the sign-off email on at least one commit doesn't match the commit author. Please git commit --amend -s (or rebase --signoff) so Signed-off-by: matches the author identity, and force-push.
This is a sizeable feature touching admin + namespace provisioning, so the go/no-go and rollout are the maintainers' call, but from a correctness standpoint it looks well-built. Once DCO is green it's in good shape for a maintainer pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #711
Motivation
On a self-hosted instance a brand-new account has nowhere of its own to publish —
only
global, or a namespace an administrator creates for it by hand. This addsan operator-controlled policy that gives each newly activated account a namespace
it owns.
Doing that needs a setting an operator can change at runtime, and SkillHub has no
mechanism for one. So this comes as two commits that can be reviewed
independently:
feat(admin): add system settings storage— a generic store, no consumer.feat(namespace): auto-provision a personal namespace on registration—the first consumer, plus the admin console surface.
Happy to split these into two PRs if you would rather review them separately; I
kept them together because a settings store with no caller is hard to judge.
Scope
Off by default.
skillhub.namespace.personal-provisioning.enableddefaults tofalse, so upgrading changes no behaviour."Personal" means what the model supports. Namespaces have no visibility flag —
only
GLOBALandTEAM— and skill visibility is a property of each skill. So thenamespace created here is an ordinary team namespace whose only member is that
account, holding
OWNER. Namespace-level visibility would be a separate feature.Design notes worth a reviewer's attention
Why an
AFTER_COMMITlistener rather than a call next toGlobalNamespaceMembershipService.ensureMemberThat would have been the obvious spot, since it is already called from both
account-creation paths. But
namespace.created_byandnamespace_member.user_idboth reference
user_account(id), which rules out each variant:back, so a namespace failure costs the user their account — or, on OAuth, their
login.
REQUIRES_NEWleaves the new transaction unable to seethe uncommitted
user_accountrow, so the foreign key check blocks on the outertransaction's row lock and the two wait on each other.
Provisioning after the account is committed avoids both. The listener is
deliberately not
@Async, so the namespace exists by the time the user's nextrequest arrives, and it swallows failures — a naming clash or a database hiccup
should cost a namespace, not a login.
Three trigger points
UserActivatedEventis published wherever an account first becomes usable:LocalAuthService.registerIdentityBindingService.bindOrCreate(ACTIVE only)AdminUserAppService.updateUserStatus(transition into ACTIVE)The third matters for deployments gated behind approval: those accounts are
created
PENDINGat the first OAuth attempt and only become usable when anadministrator approves them. Re-enabling a disabled account publishes the event
again, so provisioning is idempotent — owning any non-global namespace skips it.
Naming
Templates over
${username},${email_prefix}and${user_id}. Unknownplaceholders 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 rules, which is why the console renders a live
preview: underscores are not legal in a slug, so
${username}_spaceyieldsalice-space. A taken or reserved candidate gets a numeric suffix, so a usernamed
admingetsadmin-2.SlugValidatorgains two non-throwing helpers (normalize,isValid) forgenerated candidates;
slugifykeeps its exact previous behaviour.Why the templates are not in
application.ymlThey contain
${...}, which Spring would resolve as property references, andBoot 3.2 predates placeholder escaping. Only the enable flag lives in YAML;
template defaults come from
PersonalNamespaceProvisioningPropertiesand areedited in the console.
Settings storage
One row per setting group, value as JSONB, so a group gains fields without a
migration. Reads take the caller's defaults:
That gives two properties worth keeping:
deployment configured. Configuration-file-only deployments keep working exactly
as before.
defaults, with a warning. One malformed row must not take down login.
Groups ignore unknown fields so a rolling upgrade can read documents written by a
newer node.
Rollout impact
V44__system_setting.sql; additive, no changes to existing tables.SUPER_ADMINendpoints under/api/v1/admin/settings/personal-namespace;no existing contract changes, no CLI impact.
Verification
make test— backendBUILD SUCCESS; frontend 193 files / 679 testsV44__system_setting.sqlappliesscripts/check-openapi-generated.shused to regenerateweb/src/api/generated/schema.d.ts, which is committedpnpm typecheckandpnpm buildcleanFollow-up this unblocks
system_settingis generic. The most direct next consumer is #318 — anadministrator switch for local registration, which today can only be done by
blocking
/api/v1/auth/local/registerat the gateway. I am happy to send that asa follow-up if you want it.