diff --git a/.github/scripts/knowledge-deploy-gates.cjs b/.github/scripts/knowledge-deploy-gates.cjs index 6b3f262c45f..448d4e1549f 100644 --- a/.github/scripts/knowledge-deploy-gates.cjs +++ b/.github/scripts/knowledge-deploy-gates.cjs @@ -1,39 +1,62 @@ // A workflow_run event is privileged: accept only trusted pushes and the exact tested revision. -module.exports = async function knowledgeDeployGates({ github, context, core, now = Date.now, sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)) }) { - const source = context.payload.workflow_run; - const repository = `${context.repo.owner}/${context.repo.repo}`; - const branch = 'deploy/konwledge'; - if (source?.event !== 'push' || source.conclusion !== 'success' || source.head_branch !== branch || source.head_repository?.full_name !== repository || !/^[0-9a-f]{40}$/.test(source.head_sha)) { - throw new Error('Deployment requires a successful trusted branch push.'); +module.exports = async function knowledgeDeployGates({ + github, + context, + core, + now = Date.now, + sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)), +}) { + const source = context.payload.workflow_run + const repository = `${context.repo.owner}/${context.repo.repo}` + const branch = 'deploy/konwledge' + if ( + source?.event !== 'push' || + source.conclusion !== 'success' || + source.head_branch !== branch || + source.head_repository?.full_name !== repository || + !/^[0-9a-f]{40}$/.test(source.head_sha) + ) { + throw new Error('Deployment requires a successful trusted branch push.') } const isCurrent = async () => { - const { data } = await github.rest.repos.getBranch({ ...context.repo, branch }); - return data.commit.sha === source.head_sha; - }; + const { data } = await github.rest.repos.getBranch({ ...context.repo, branch }) + return data.commit.sha === source.head_sha + } const skip = () => { - core.info('Skipping a superseded deployment revision.'); - core.setOutput('current', 'false'); - }; - if (!(await isCurrent())) return skip(); - const deadline = now() + 30 * 60 * 1000; + core.info('Skipping a superseded deployment revision.') + core.setOutput('current', 'false') + } + if (!(await isCurrent())) return skip() + const deadline = now() + 30 * 60 * 1000 while (now() < deadline) { const { data } = await github.rest.actions.listWorkflowRuns({ - ...context.repo, workflow_id: 'knowledge-fs-ci.yml', branch, event: 'push', - head_sha: source.head_sha, per_page: 100, - }); + ...context.repo, + workflow_id: 'knowledge-fs-ci.yml', + branch, + event: 'push', + head_sha: source.head_sha, + per_page: 100, + }) const run = data.workflow_runs - .filter((candidate) => candidate.head_sha === source.head_sha && candidate.head_branch === branch && candidate.event === 'push' && candidate.head_repository?.full_name === repository) - .sort((a, b) => b.run_number - a.run_number || b.run_attempt - a.run_attempt)[0]; + .filter( + (candidate) => + candidate.head_sha === source.head_sha && + candidate.head_branch === branch && + candidate.event === 'push' && + candidate.head_repository?.full_name === repository, + ) + .sort((a, b) => b.run_number - a.run_number || b.run_attempt - a.run_attempt)[0] if (run?.status === 'completed') { - if (run.conclusion !== 'success') throw new Error(`KnowledgeFS CI did not succeed for ${source.head_sha}: ${run.conclusion}.`); - if (!(await isCurrent())) return skip(); - core.info(`KnowledgeFS CI and image build succeeded for ${source.head_sha}.`); - core.setOutput('current', 'true'); - return; + if (run.conclusion !== 'success') + throw new Error(`KnowledgeFS CI did not succeed for ${source.head_sha}: ${run.conclusion}.`) + if (!(await isCurrent())) return skip() + core.info(`KnowledgeFS CI and image build succeeded for ${source.head_sha}.`) + core.setOutput('current', 'true') + return } - if (!(await isCurrent())) return skip(); - core.info(`Waiting for KnowledgeFS CI at ${source.head_sha}.`); - await sleep(15000); + if (!(await isCurrent())) return skip() + core.info(`Waiting for KnowledgeFS CI at ${source.head_sha}.`) + await sleep(15000) } - throw new Error(`Timed out waiting for KnowledgeFS CI for ${source.head_sha}.`); -}; + throw new Error(`Timed out waiting for KnowledgeFS CI for ${source.head_sha}.`) +} diff --git a/.github/scripts/knowledge-deploy-gates.test.cjs b/.github/scripts/knowledge-deploy-gates.test.cjs index 8e3bf8ad022..890c9e2dd7a 100644 --- a/.github/scripts/knowledge-deploy-gates.test.cjs +++ b/.github/scripts/knowledge-deploy-gates.test.cjs @@ -1,75 +1,128 @@ -const assert = require('node:assert/strict'); -const test = require('node:test'); -const gate = require('./knowledge-deploy-gates.cjs'); -const sha = 'a'.repeat(40); -const otherSha = 'b'.repeat(40); +const assert = require('node:assert/strict') +const test = require('node:test') +const gate = require('./knowledge-deploy-gates.cjs') +const sha = 'a'.repeat(40) +const otherSha = 'b'.repeat(40) function fixture() { - const source = { event: 'push', conclusion: 'success', head_branch: 'deploy/konwledge', head_repository: { full_name: 'langgenius/dify' }, head_sha: sha }; - let time = 0; - let branchSha = sha; - let runs = [{ ...source, status: 'completed', run_number: 2, run_attempt: 1 }]; - const outputs = {}; - const requests = []; + const source = { + event: 'push', + conclusion: 'success', + head_branch: 'deploy/konwledge', + head_repository: { full_name: 'langgenius/dify' }, + head_sha: sha, + } + let time = 0 + let branchSha = sha + let runs = [{ ...source, status: 'completed', run_number: 2, run_attempt: 1 }] + const outputs = {} + const requests = [] return { - source, outputs, requests, - setRuns: (value) => { runs = value; }, - setBranch: (value) => { branchSha = value; }, + source, + outputs, + requests, + setRuns: (value) => { + runs = value + }, + setBranch: (value) => { + branchSha = value + }, args: { context: { repo: { owner: 'langgenius', repo: 'dify' }, payload: { workflow_run: source } }, - github: { rest: { - repos: { getBranch: async () => ({ data: { commit: { sha: branchSha } } }) }, - actions: { listWorkflowRuns: async (request) => { requests.push(request); return { data: { workflow_runs: runs } }; } }, - } }, - core: { info: () => {}, setOutput: (key, value) => { outputs[key] = value; } }, + github: { + rest: { + repos: { getBranch: async () => ({ data: { commit: { sha: branchSha } } }) }, + actions: { + listWorkflowRuns: async (request) => { + requests.push(request) + return { data: { workflow_runs: runs } } + }, + }, + }, + }, + core: { + info: () => {}, + setOutput: (key, value) => { + outputs[key] = value + }, + }, now: () => time, - sleep: async (ms) => { time += ms; }, + sleep: async (ms) => { + time += ms + }, }, - }; + } } test('only a successful CI run from the same branch, repo, event, and SHA admits deployment', async () => { - const f = fixture(); - await gate(f.args); - assert.equal(f.outputs.current, 'true'); - assert.equal(f.requests[0].head_sha, sha); - assert.equal(f.requests[0].workflow_id, 'knowledge-fs-ci.yml'); -}); + const f = fixture() + await gate(f.args) + assert.equal(f.outputs.current, 'true') + assert.equal(f.requests[0].head_sha, sha) + assert.equal(f.requests[0].workflow_id, 'knowledge-fs-ci.yml') +}) for (const [name, mutation] of [ - ['pull request', { event: 'pull_request' }], ['failed build', { conclusion: 'failure' }], - ['foreign branch', { head_branch: 'main' }], ['foreign repo', { head_repository: { full_name: 'attacker/fork' } }], + ['pull request', { event: 'pull_request' }], + ['failed build', { conclusion: 'failure' }], + ['foreign branch', { head_branch: 'main' }], + ['foreign repo', { head_repository: { full_name: 'attacker/fork' } }], ['invalid revision', { head_sha: 'main' }], -]) test(`rejects ${name} workflow events before any API query`, async () => { - const f = fixture(); Object.assign(f.source, mutation); - await assert.rejects(gate(f.args), /trusted branch push/); - assert.deepEqual(f.requests, []); -}); -for (const conclusion of ['failure', 'cancelled', 'skipped', 'timed_out', 'neutral']) test(`CI ${conclusion} fails closed`, async () => { - const f = fixture(); f.setRuns([{ ...f.source, status: 'completed', conclusion }]); - await assert.rejects(gate(f.args), /did not succeed/); - assert.notEqual(f.outputs.current, 'true'); -}); -for (const mutation of [{ head_sha: otherSha }, { head_branch: 'main' }, { event: 'pull_request' }, { head_repository: { full_name: 'attacker/fork' } }]) test(`ignores unrelated successful CI run ${JSON.stringify(mutation)}`, async () => { - const f = fixture(); f.setRuns([{ ...f.source, ...mutation, status: 'completed' }]); - await assert.rejects(gate(f.args), /Timed out/); - assert.notEqual(f.outputs.current, 'true'); -}); +]) + test(`rejects ${name} workflow events before any API query`, async () => { + const f = fixture() + Object.assign(f.source, mutation) + await assert.rejects(gate(f.args), /trusted branch push/) + assert.deepEqual(f.requests, []) + }) +for (const conclusion of ['failure', 'cancelled', 'skipped', 'timed_out', 'neutral']) + test(`CI ${conclusion} fails closed`, async () => { + const f = fixture() + f.setRuns([{ ...f.source, status: 'completed', conclusion }]) + await assert.rejects(gate(f.args), /did not succeed/) + assert.notEqual(f.outputs.current, 'true') + }) +for (const mutation of [ + { head_sha: otherSha }, + { head_branch: 'main' }, + { event: 'pull_request' }, + { head_repository: { full_name: 'attacker/fork' } }, +]) + test(`ignores unrelated successful CI run ${JSON.stringify(mutation)}`, async () => { + const f = fixture() + f.setRuns([{ ...f.source, ...mutation, status: 'completed' }]) + await assert.rejects(gate(f.args), /Timed out/) + assert.notEqual(f.outputs.current, 'true') + }) test('latest failed attempt cannot be hidden by an earlier successful run', async () => { - const f = fixture(); f.setRuns([ + const f = fixture() + f.setRuns([ { ...f.source, status: 'completed', run_number: 1, run_attempt: 1 }, { ...f.source, status: 'completed', run_number: 2, run_attempt: 1, conclusion: 'failure' }, - ]); - await assert.rejects(gate(f.args), /did not succeed/); -}); + ]) + await assert.rejects(gate(f.args), /did not succeed/) +}) test('stale revision skips without waiting or deploying', async () => { - const f = fixture(); f.setBranch(otherSha); await gate(f.args); - assert.equal(f.outputs.current, 'false'); assert.deepEqual(f.requests, []); -}); + const f = fixture() + f.setBranch(otherSha) + await gate(f.args) + assert.equal(f.outputs.current, 'false') + assert.deepEqual(f.requests, []) +}) test('branch advancing during CI wait prevents deployment', async () => { - const f = fixture(); f.setRuns([{ ...f.source, status: 'in_progress' }]); - f.args.sleep = async () => { f.setBranch(otherSha); f.setRuns([{ ...f.source, status: 'completed' }]); }; - await gate(f.args); assert.equal(f.outputs.current, 'false'); -}); + const f = fixture() + f.setRuns([{ ...f.source, status: 'in_progress' }]) + f.args.sleep = async () => { + f.setBranch(otherSha) + f.setRuns([{ ...f.source, status: 'completed' }]) + } + await gate(f.args) + assert.equal(f.outputs.current, 'false') +}) test('running CI is awaited before reporting success', async () => { - const f = fixture(); f.setRuns([]); - f.args.sleep = async () => { f.setRuns([{ ...f.source, status: 'completed' }]); }; - await gate(f.args); assert.equal(f.outputs.current, 'true'); assert.equal(f.requests.length, 2); -}); + const f = fixture() + f.setRuns([]) + f.args.sleep = async () => { + f.setRuns([{ ...f.source, status: 'completed' }]) + } + await gate(f.args) + assert.equal(f.outputs.current, 'true') + assert.equal(f.requests.length, 2) +}) diff --git a/api/controllers/console/knowledge_fs/resources.py b/api/controllers/console/knowledge_fs/resources.py index 7717fca9d19..54ad50b80cc 100644 --- a/api/controllers/console/knowledge_fs/resources.py +++ b/api/controllers/console/knowledge_fs/resources.py @@ -66,6 +66,7 @@ from services.knowledge_fs.buffered_upload_admission import DEFAULT_KNOWLEDGE_FS from services.knowledge_fs.control_plane_service import ( KnowledgeFSControlPlaneInvariantError, ) +from services.knowledge_fs.control_space_commands import KnowledgeFSControlSpaceIntentConflictError from services.knowledge_fs.download_service import ( KnowledgeFSDownloadObjectNotFoundError, KnowledgeFSDownloadService, @@ -460,6 +461,8 @@ def _knowledge_fs_errors[**P, R](view: Callable[P, R]) -> Callable[P, R]: raise KnowledgeFSSpaceNotFoundHTTPError() from exc except KnowledgeFSOperationUnavailableError as exc: raise KnowledgeFSOperationUnavailableHTTPError() from exc + except KnowledgeFSControlSpaceIntentConflictError as exc: + raise KnowledgeFSConflictHTTPError() from exc except KnowledgeFSInitialSourcePreviewJobAlreadyRunningError as exc: raise Conflict() from exc except KnowledgeFSProductResourceNotFoundError as exc: diff --git a/api/models/knowledge_fs.py b/api/models/knowledge_fs.py index 15a61e09aaa..fd162b2e6b4 100644 --- a/api/models/knowledge_fs.py +++ b/api/models/knowledge_fs.py @@ -60,6 +60,9 @@ class KnowledgeFSProvisionCommandPayload(KnowledgeFSCommandEnvelope): slug: str model_intent: NotRequired[KnowledgeFSModelSelectionIntentPayload] profile_intent: NotRequired[KnowledgeFSRetrievalProfileIntentPayload] + # Local authorization snapshot; never sent to the KnowledgeFS data plane. + initial_visibility: NotRequired[str] + initial_member_account_ids: NotRequired[list[str]] class KnowledgeFSMetadataUpdateCommandPayload(KnowledgeFSCommandEnvelope): diff --git a/api/openapi/markdown/console-openapi.md b/api/openapi/markdown/console-openapi.md index 170b5564620..8e0ae6d8107 100644 --- a/api/openapi/markdown/console-openapi.md +++ b/api/openapi/markdown/console-openapi.md @@ -23601,6 +23601,13 @@ the file name and a short-lived signed preview URL for files the caller still ow | workspace_id | string | | No | | workspace_name | string | | No | +#### KnowledgeFSSpaceCreateMemberPayload + +| Name | Type | Description | Required | +| ---- | ---- | ----------- | -------- | +| account_id | string | | Yes | +| role | string | | Yes | + #### KnowledgeFSSpaceCreatePayload | Name | Type | Description | Required | @@ -23610,6 +23617,7 @@ the file name and a short-lived signed preview URL for files the caller still ow | icon | string | | No | | idempotency_key | string | | No | | initial_source | [KnowledgeFSInitialWebsiteSourcePayload](#knowledgefsinitialwebsitesourcepayload)
[KnowledgeFSInitialOnlineDocumentSourcePayload](#knowledgefsinitialonlinedocumentsourcepayload)
[KnowledgeFSInitialOnlineDriveSourcePayload](#knowledgefsinitialonlinedrivesourcepayload) | | No | +| members | [ [KnowledgeFSSpaceCreateMemberPayload](#knowledgefsspacecreatememberpayload) ] | | No | | name | string | | Yes | | retrieval | [KnowledgeFSRetrievalProfileIntent](#knowledgefsretrievalprofileintent) | | No | | slug | string | | Yes | diff --git a/api/services/knowledge_fs/control_plane_service.py b/api/services/knowledge_fs/control_plane_service.py index 28980fbe001..9687b57b319 100644 --- a/api/services/knowledge_fs/control_plane_service.py +++ b/api/services/knowledge_fs/control_plane_service.py @@ -3,13 +3,11 @@ from __future__ import annotations from collections.abc import Sequence -from typing import Protocol import sqlalchemy as sa from sqlalchemy.orm import Session, sessionmaker from libs.datetime_utils import naive_utc_now -from models import TenantAccountJoin from models.knowledge_fs import ( KnowledgeFSAuthorizationRevision, KnowledgeFSControlSpace, @@ -33,30 +31,20 @@ from services.knowledge_fs.revocation_commands import ( KnowledgeFSRevocationCommandPort, KnowledgeFSRevocationCommandProducer, ) +from services.knowledge_fs.workspace_members import ( + KnowledgeFSControlPlaneInvariantError, + KnowledgeFSWorkspaceMemberPort, + SQLKnowledgeFSWorkspaceMemberPort, + validate_member_account_ids, +) - -class KnowledgeFSControlPlaneInvariantError(RuntimeError): - """Required authorization revision state is absent or inconsistent.""" - - -class KnowledgeFSWorkspaceMemberPort(Protocol): - def are_active_members(self, *, session: Session, tenant_id: str, account_ids: Sequence[str]) -> bool: ... - - -class SQLKnowledgeFSWorkspaceMemberPort: - def are_active_members(self, *, session: Session, tenant_id: str, account_ids: Sequence[str]) -> bool: - unique_ids = frozenset(account_ids) - if not unique_ids: - return True - found = frozenset( - session.scalars( - sa.select(TenantAccountJoin.account_id).where( - TenantAccountJoin.tenant_id == tenant_id, - TenantAccountJoin.account_id.in_(unique_ids), - ) - ) - ) - return found == unique_ids +# Preserve the established import paths for callers of the control-plane service. +__all__ = [ + "KnowledgeFSControlPlaneInvariantError", + "KnowledgeFSControlPlaneService", + "KnowledgeFSWorkspaceMemberPort", + "SQLKnowledgeFSWorkspaceMemberPort", +] class KnowledgeFSControlPlaneService: @@ -126,8 +114,10 @@ class KnowledgeFSControlPlaneService: permission=KnowledgeFSProductPermission.ACCESS_CONFIG, ) desired = {member.account_id: member.role for member in members} - if len(desired) != len(members) or authorized.control_space.owner_account_id in desired: - raise KnowledgeFSControlPlaneInvariantError("Member bindings must be unique and exclude the owner") + validate_member_account_ids( + owner_account_id=authorized.control_space.owner_account_id, + account_ids=tuple(member.account_id for member in members), + ) with self._session_maker.begin() as session: control_space = session.scalar( sa.select(KnowledgeFSControlSpace) diff --git a/api/services/knowledge_fs/control_space_commands.py b/api/services/knowledge_fs/control_space_commands.py index d90bb82cde2..3126d6e9df5 100644 --- a/api/services/knowledge_fs/control_space_commands.py +++ b/api/services/knowledge_fs/control_space_commands.py @@ -18,6 +18,7 @@ from models.knowledge_fs import ( KnowledgeFSControlSpacePermission, KnowledgeFSControlSpacePermissionRole, KnowledgeFSControlSpaceState, + KnowledgeFSControlSpaceVisibility, KnowledgeFSDeleteCommandPayload, KnowledgeFSExternalAccessPolicy, KnowledgeFSLifecycleOperation, @@ -41,6 +42,12 @@ from services.knowledge_fs.control_space_lifecycle import ( KnowledgeFSControlSpaceVersionConflictError, KnowledgeFSWorkspaceDeletionBlockedError, ) +from services.knowledge_fs.workspace_members import ( + KnowledgeFSControlPlaneInvariantError, + KnowledgeFSWorkspaceMemberPort, + SQLKnowledgeFSWorkspaceMemberPort, + validate_member_account_ids, +) class KnowledgeFSProvisionIntent(NamedTuple): @@ -55,6 +62,8 @@ class KnowledgeFSProvisionIntent(NamedTuple): description: str | None model_intent: KnowledgeFSModelSelectionIntentPayload | None profile_intent: KnowledgeFSRetrievalProfileIntentPayload | None + visibility: KnowledgeFSControlSpaceVisibility = KnowledgeFSControlSpaceVisibility.ONLY_ME + member_account_ids: tuple[str, ...] = () class KnowledgeFSProvisionIntentResult(NamedTuple): @@ -83,10 +92,20 @@ def _provision_payload_requires_model_setup(payload: KnowledgeFSProvisionCommand class KnowledgeFSControlSpaceCommandService: """Write control state and its outbox command in one database transaction.""" - def __init__(self, session_maker: sessionmaker[Session]): + def __init__( + self, + session_maker: sessionmaker[Session], + *, + members: KnowledgeFSWorkspaceMemberPort | None = None, + ): self._session_maker = session_maker + self._members = members or SQLKnowledgeFSWorkspaceMemberPort() def create_provision_intent(self, intent: KnowledgeFSProvisionIntent) -> KnowledgeFSProvisionIntentResult: + validate_member_account_ids(owner_account_id=intent.owner_account_id, account_ids=intent.member_account_ids) + if bool(intent.member_account_ids) != (intent.visibility is KnowledgeFSControlSpaceVisibility.PARTIAL_MEMBERS): + raise KnowledgeFSControlPlaneInvariantError("Initial members must match partial member visibility") + member_account_ids = sorted(intent.member_account_ids) with self._session_maker.begin() as session: control_repository = SQLAlchemyKnowledgeFSControlSpaceRepository(session) outbox_repository = SQLAlchemyKnowledgeFSLifecycleOutboxRepository(session) @@ -106,6 +125,17 @@ class KnowledgeFSControlSpaceCommandService: raise KnowledgeFSControlSpaceIntentConflictError( "KnowledgeFS provisioning key was reused for a different intent" ) + saved_visibility = command.command_payload.get("initial_visibility") + saved_members = command.command_payload.get("initial_member_account_ids", []) + # Old commands had no initial access snapshot. Preserve their empty-member + # replay contract, without changing current visibility or regranting access. + if (saved_visibility is None and member_account_ids) or ( + saved_visibility is not None + and (saved_visibility != intent.visibility.value or saved_members != member_account_ids) + ): + raise KnowledgeFSControlSpaceIntentConflictError( + "KnowledgeFS provisioning key was reused for different initial access" + ) return KnowledgeFSProvisionIntentResult( existing, command, @@ -114,12 +144,17 @@ class KnowledgeFSControlSpaceCommandService: ), ) + if not self._members.are_active_members( + session=session, tenant_id=intent.tenant_id, account_ids=member_account_ids + ): + raise KnowledgeFSControlPlaneInvariantError("Every KnowledgeFS member must belong to the workspace") control_space = control_repository.add( KnowledgeFSControlSpace( tenant_id=intent.tenant_id, owner_account_id=intent.owner_account_id, provisioning_key=intent.provisioning_key, lifecycle_operation_id=intent.operation_id, + visibility=intent.visibility, ) ) session.add( @@ -148,6 +183,16 @@ class KnowledgeFSControlSpaceCommandService: updated_by_account_id=intent.owner_account_id, ) ) + for account_id in member_account_ids: + session.add( + KnowledgeFSControlSpacePermission( + tenant_id=intent.tenant_id, + control_space_id=control_space.id, + account_id=account_id, + role=KnowledgeFSControlSpacePermissionRole.VIEWER, + granted_by_account_id=intent.owner_account_id, + ) + ) payload = KnowledgeFSProvisionCommandPayload( schema_version=1, idempotency_key=intent.idempotency_key, @@ -157,6 +202,8 @@ class KnowledgeFSControlSpaceCommandService: icon=intent.icon, description=intent.description, slug=intent.slug, + initial_visibility=intent.visibility.value, + initial_member_account_ids=member_account_ids, ) if intent.model_intent is not None: payload["model_intent"] = KnowledgeFSModelSelectionIntentPayload(**intent.model_intent) diff --git a/api/services/knowledge_fs/product_application_service.py b/api/services/knowledge_fs/product_application_service.py index 2211ce71b81..4dcf3613737 100644 --- a/api/services/knowledge_fs/product_application_service.py +++ b/api/services/knowledge_fs/product_application_service.py @@ -86,6 +86,14 @@ class KnowledgeFSProductApplicationService: permission=KnowledgeFSProductPermission.CREATE, ): raise PermissionError("KnowledgeFS space creation is not allowed") + if payload.visibility is not KnowledgeFSControlSpaceVisibility.ONLY_ME and not ( + self._rbac.workspace_permission_allowed( + tenant_id=tenant_id, + account_id=account_id, + permission=KnowledgeFSProductPermission.ACCESS_CONFIG, + ) + ): + raise PermissionError("KnowledgeFS initial access configuration is not allowed") idempotency_key = payload.idempotency_key or str(uuid.uuid4()) operation_id = str( uuid.uuid5(uuid.NAMESPACE_URL, f"dify-kfs-provision:{tenant_id}:{account_id}:{idempotency_key}") @@ -105,15 +113,10 @@ class KnowledgeFSProductApplicationService: profile_intent=( _retrieval_profile_intent(payload.retrieval) if payload.retrieval is not None else None ), + visibility=payload.visibility, + member_account_ids=tuple(member.account_id for member in payload.members), ) ) - if payload.visibility is not KnowledgeFSControlSpaceVisibility.ONLY_ME: - self._control_plane.update_visibility( - tenant_id=tenant_id, - actor_account_id=account_id, - control_space_id=result.control_space.id, - visibility=payload.visibility, - ) if payload.initial_source is not None: from tasks.knowledge_fs_initial_source_tasks import import_initial_source diff --git a/api/services/knowledge_fs/product_dto.py b/api/services/knowledge_fs/product_dto.py index 83186d91cc3..8fbdfee160b 100644 --- a/api/services/knowledge_fs/product_dto.py +++ b/api/services/knowledge_fs/product_dto.py @@ -410,12 +410,20 @@ KnowledgeFSInitialSourcePayload = Annotated[ ] +class KnowledgeFSSpaceCreateMemberPayload(BaseModel): + account_id: str = Field(min_length=1, max_length=255) + role: Literal["viewer"] + + model_config = ConfigDict(extra="forbid") + + class KnowledgeFSSpaceCreatePayload(BaseModel): name: str = Field(min_length=1, max_length=40) slug: str = Field(min_length=1, max_length=160, pattern=r"^[a-z0-9]+(?:-[a-z0-9]+)*$") icon: KnowledgeFSIconIdentity | None = None description: str | None = Field(default=None, max_length=2_000) visibility: KnowledgeFSControlSpaceVisibility = KnowledgeFSControlSpaceVisibility.ONLY_ME + members: list[KnowledgeFSSpaceCreateMemberPayload] = Field(default_factory=list, max_length=1_000) embedding: KnowledgeFSModelIntent | None = None retrieval: KnowledgeFSRetrievalProfileIntent | None = None idempotency_key: str | None = Field(default=None, min_length=1, max_length=255) @@ -427,6 +435,13 @@ class KnowledgeFSSpaceCreatePayload(BaseModel): def validate_initial_model_configuration(self) -> KnowledgeFSSpaceCreatePayload: if self.retrieval is not None and self.embedding is None: raise ValueError("Agent Knowledge Base retrieval requires an embedding model") + if self.visibility is KnowledgeFSControlSpaceVisibility.PARTIAL_MEMBERS: + if not self.members: + raise ValueError("Partial member visibility requires at least one member") + elif self.members: + raise ValueError("Initial members require partial member visibility") + if len({member.account_id for member in self.members}) != len(self.members): + raise ValueError("Initial members must be unique") return self diff --git a/api/services/knowledge_fs/workspace_members.py b/api/services/knowledge_fs/workspace_members.py new file mode 100644 index 00000000000..392669b46c1 --- /dev/null +++ b/api/services/knowledge_fs/workspace_members.py @@ -0,0 +1,38 @@ +"""Shared workspace membership validation for KnowledgeFS access mutations.""" + +from collections.abc import Sequence +from typing import Protocol + +import sqlalchemy as sa +from sqlalchemy.orm import Session + +from models import TenantAccountJoin + + +class KnowledgeFSControlPlaneInvariantError(RuntimeError): + """Required authorization revision state is absent or inconsistent.""" + + +class KnowledgeFSWorkspaceMemberPort(Protocol): + def are_active_members(self, *, session: Session, tenant_id: str, account_ids: Sequence[str]) -> bool: ... + + +class SQLKnowledgeFSWorkspaceMemberPort: + def are_active_members(self, *, session: Session, tenant_id: str, account_ids: Sequence[str]) -> bool: + unique_ids = frozenset(account_ids) + if not unique_ids: + return True + found = frozenset( + session.scalars( + sa.select(TenantAccountJoin.account_id).where( + TenantAccountJoin.tenant_id == tenant_id, + TenantAccountJoin.account_id.in_(unique_ids), + ) + ) + ) + return found == unique_ids + + +def validate_member_account_ids(*, owner_account_id: str, account_ids: Sequence[str]) -> None: + if len(set(account_ids)) != len(account_ids) or owner_account_id in account_ids: + raise KnowledgeFSControlPlaneInvariantError("Member bindings must be unique and exclude the owner") diff --git a/api/tests/unit_tests/controllers/test_knowledge_fs_product_controllers.py b/api/tests/unit_tests/controllers/test_knowledge_fs_product_controllers.py index fb9527c065b..62ba0318fc4 100644 --- a/api/tests/unit_tests/controllers/test_knowledge_fs_product_controllers.py +++ b/api/tests/unit_tests/controllers/test_knowledge_fs_product_controllers.py @@ -20,12 +20,14 @@ from controllers.console import console_ns from controllers.console import wraps as console_wraps from controllers.console.knowledge_fs import resources as console_resources from controllers.console.knowledge_fs.error import ( + KnowledgeFSConflictHTTPError, KnowledgeFSRequestTooLargeHTTPError, KnowledgeFSResourceNotFoundHTTPError, ) from controllers.console.wraps import RBACPermission from controllers.service_api import service_api_ns from controllers.service_api.knowledge_fs import resources as service_resources +from services.knowledge_fs.control_space_commands import KnowledgeFSControlSpaceIntentConflictError from services.knowledge_fs.download_service import KnowledgeFSDownloadUnavailableError from services.knowledge_fs.object_storage import KnowledgeFSObjectMetadata from services.knowledge_fs.product_dto import ( @@ -1715,3 +1717,24 @@ def test_service_api_errors_distinguish_unknown_key_from_out_of_scope_key( with pytest.raises(expected): view() + + +def test_space_create_member_schema_requires_viewer_role() -> None: + payload_schema = console_ns.models["KnowledgeFSSpaceCreatePayload"].__schema__ + member_schema = console_ns.models["KnowledgeFSSpaceCreateMemberPayload"].__schema__ + + assert payload_schema["properties"]["members"]["maxItems"] == 1_000 + assert member_schema["properties"]["role"]["const"] == "viewer" + assert set(member_schema["required"]) == {"account_id", "role"} + assert member_schema["additionalProperties"] is False + + +def test_space_create_initial_access_idempotency_conflict_returns_409() -> None: + @console_resources._knowledge_fs_errors + def conflicting_creation() -> None: + raise KnowledgeFSControlSpaceIntentConflictError("different initial access") + + with pytest.raises(KnowledgeFSConflictHTTPError) as exc: + conflicting_creation() + + assert exc.value.code == 409 diff --git a/api/tests/unit_tests/services/test_knowledge_fs_initial_members.py b/api/tests/unit_tests/services/test_knowledge_fs_initial_members.py new file mode 100644 index 00000000000..0bf0bba8617 --- /dev/null +++ b/api/tests/unit_tests/services/test_knowledge_fs_initial_members.py @@ -0,0 +1,245 @@ +from __future__ import annotations + +import pytest +from pydantic import ValidationError +from sqlalchemy import delete, func, select +from sqlalchemy.orm import Session, sessionmaker + +from models import TenantAccountJoin +from models.knowledge_fs import ( + KnowledgeFSAuthorizationRevision, + KnowledgeFSControlSpace, + KnowledgeFSControlSpacePermission, + KnowledgeFSControlSpacePermissionRole, + KnowledgeFSControlSpacePermissionStatus, + KnowledgeFSControlSpaceVisibility, + KnowledgeFSExternalAccessPolicy, + KnowledgeFSLifecycleOutbox, +) +from repositories.sqlalchemy_knowledge_fs_lifecycle_outbox_repository import ( + SQLAlchemyKnowledgeFSLifecycleOutboxRepository, +) +from services.knowledge_fs.control_space_commands import ( + KnowledgeFSControlSpaceCommandService, + KnowledgeFSControlSpaceIntentConflictError, + KnowledgeFSProvisionIntent, +) +from services.knowledge_fs.product_dto import KnowledgeFSSpaceCreatePayload +from services.knowledge_fs.workspace_members import KnowledgeFSControlPlaneInvariantError + + +def _intent() -> KnowledgeFSProvisionIntent: + return KnowledgeFSProvisionIntent( + tenant_id="tenant-1", + owner_account_id="owner-1", + provisioning_key="provision-1", + operation_id="operation-1", + idempotency_key="create-1", + name="Selected members", + slug="selected-members", + icon=None, + description=None, + model_intent=None, + profile_intent=None, + visibility=KnowledgeFSControlSpaceVisibility.PARTIAL_MEMBERS, + member_account_ids=("member-2", "member-1"), + ) + + +def _seed_members(session: Session) -> None: + session.add_all( + [ + TenantAccountJoin(tenant_id="tenant-1", account_id="member-1"), + TenantAccountJoin(tenant_id="tenant-1", account_id="member-2"), + TenantAccountJoin(tenant_id="other-tenant", account_id="outsider"), + ] + ) + session.commit() + + +def _assert_no_creation(session: Session) -> None: + for model in ( + KnowledgeFSControlSpace, + KnowledgeFSControlSpacePermission, + KnowledgeFSAuthorizationRevision, + KnowledgeFSExternalAccessPolicy, + KnowledgeFSLifecycleOutbox, + ): + assert session.scalar(select(func.count()).select_from(model)) == 0 + + +def test_partial_members_create_dto_accepts_viewers_and_preserves_default_creation() -> None: + payload = KnowledgeFSSpaceCreatePayload.model_validate( + { + "name": "Selected members", + "slug": "selected-members", + "visibility": "partial_members", + "members": [{"account_id": "member-1", "role": "viewer"}], + } + ) + assert payload.members[0].account_id == "member-1" + assert payload.members[0].role == "viewer" + assert KnowledgeFSSpaceCreatePayload(name="Private", slug="private").members == [] + assert KnowledgeFSSpaceCreatePayload(name="Team", slug="team", visibility="all_team_members").members == [] + + +@pytest.mark.parametrize( + "updates", + [ + {"visibility": "partial_members"}, + {"visibility": "partial_members", "members": []}, + {"visibility": "partial_members", "members": None}, + {"members": [{"account_id": "member-1", "role": "viewer"}]}, + {"visibility": "all_team_members", "members": [{"account_id": "member-1", "role": "viewer"}]}, + {"visibility": "partial_members", "members": [{"account_id": "member-1", "role": "owner"}]}, + {"visibility": "partial_members", "members": [{"account_id": "member-1", "role": "editor"}]}, + {"visibility": "partial_members", "members": [{"account_id": "member-1"}]}, + {"visibility": "partial_members", "members": [{"account_id": "", "role": "viewer"}]}, + { + "visibility": "partial_members", + "members": [{"account_id": "member-1", "role": "viewer"}] * 2, + }, + { + "visibility": "partial_members", + "members": [{"account_id": f"member-{i}", "role": "viewer"} for i in range(1_001)], + }, + ], +) +def test_partial_members_create_dto_rejects_invalid_or_privileged_members(updates: dict[str, object]) -> None: + with pytest.raises(ValidationError): + KnowledgeFSSpaceCreatePayload.model_validate({"name": "Space", "slug": "space", **updates}) + + +def test_initial_members_and_visibility_are_committed_with_the_space_and_outbox( + sqlite_session: Session, sqlite_session_factory: sessionmaker[Session] +) -> None: + _seed_members(sqlite_session) + service = KnowledgeFSControlSpaceCommandService(sqlite_session_factory) + + result = service.create_provision_intent(_intent()) + replay = service.create_provision_intent(_intent()._replace(member_account_ids=("member-1", "member-2"))) + + assert result.control_space.visibility is KnowledgeFSControlSpaceVisibility.PARTIAL_MEMBERS + assert replay.control_space.id == result.control_space.id + assert replay.outbox.id == result.outbox.id + assert result.outbox.expected_control_space_version == result.control_space.resource_version == 0 + assert result.outbox.command_payload["initial_visibility"] == "partial_members" + assert result.outbox.command_payload["initial_member_account_ids"] == ["member-1", "member-2"] + permissions = list(sqlite_session.scalars(select(KnowledgeFSControlSpacePermission))) + assert {permission.account_id: permission.role for permission in permissions} == { + "owner-1": KnowledgeFSControlSpacePermissionRole.OWNER, + "member-1": KnowledgeFSControlSpacePermissionRole.VIEWER, + "member-2": KnowledgeFSControlSpacePermissionRole.VIEWER, + } + assert all(permission.granted_by_account_id == "owner-1" for permission in permissions) + assert sqlite_session.scalar(select(func.count()).select_from(KnowledgeFSLifecycleOutbox)) == 1 + assert sqlite_session.scalar(select(func.count()).select_from(KnowledgeFSAuthorizationRevision)) == 1 + assert sqlite_session.scalar(select(func.count()).select_from(KnowledgeFSExternalAccessPolicy)) == 1 + + +@pytest.mark.parametrize("member_ids", [(), ("owner-1",), ("member-1", "member-1"), ("outsider",), ("missing",)]) +def test_initial_member_validation_precedes_any_creation( + sqlite_session: Session, sqlite_session_factory: sessionmaker[Session], member_ids: tuple[str, ...] +) -> None: + _seed_members(sqlite_session) + service = KnowledgeFSControlSpaceCommandService(sqlite_session_factory) + + with pytest.raises(KnowledgeFSControlPlaneInvariantError): + service.create_provision_intent(_intent()._replace(member_account_ids=member_ids)) + + _assert_no_creation(sqlite_session) + + +def test_initial_members_roll_back_if_the_outbox_write_fails( + sqlite_session: Session, + sqlite_session_factory: sessionmaker[Session], + monkeypatch: pytest.MonkeyPatch, +) -> None: + _seed_members(sqlite_session) + original_add = SQLAlchemyKnowledgeFSLifecycleOutboxRepository.add + + def fail_after_flush( + repository: SQLAlchemyKnowledgeFSLifecycleOutboxRepository, command: KnowledgeFSLifecycleOutbox + ) -> KnowledgeFSLifecycleOutbox: + original_add(repository, command) + raise RuntimeError("outbox write interrupted") + + monkeypatch.setattr(SQLAlchemyKnowledgeFSLifecycleOutboxRepository, "add", fail_after_flush) + + with pytest.raises(RuntimeError, match="outbox write interrupted"): + KnowledgeFSControlSpaceCommandService(sqlite_session_factory).create_provision_intent(_intent()) + + _assert_no_creation(sqlite_session) + + +def test_creation_replay_does_not_restore_revoked_members_or_initial_visibility( + sqlite_session: Session, sqlite_session_factory: sessionmaker[Session] +) -> None: + _seed_members(sqlite_session) + service = KnowledgeFSControlSpaceCommandService(sqlite_session_factory) + result = service.create_provision_intent(_intent()) + space = sqlite_session.get(KnowledgeFSControlSpace, result.control_space.id) + assert space is not None + space.visibility = KnowledgeFSControlSpaceVisibility.ONLY_ME + member = sqlite_session.scalar( + select(KnowledgeFSControlSpacePermission).where(KnowledgeFSControlSpacePermission.account_id == "member-1") + ) + assert member is not None + member.status = KnowledgeFSControlSpacePermissionStatus.REVOKED + sqlite_session.execute(delete(TenantAccountJoin).where(TenantAccountJoin.account_id == "member-1")) + sqlite_session.commit() + + replay = service.create_provision_intent(_intent()) + + assert replay.control_space.visibility is KnowledgeFSControlSpaceVisibility.ONLY_ME + assert replay.outbox.id == result.outbox.id + sqlite_session.refresh(member) + assert member.status is KnowledgeFSControlSpacePermissionStatus.REVOKED + + +@pytest.mark.parametrize( + "changes", + [ + {"member_account_ids": ("member-1",)}, + {"visibility": KnowledgeFSControlSpaceVisibility.ALL_TEAM_MEMBERS, "member_account_ids": ()}, + {"visibility": KnowledgeFSControlSpaceVisibility.ONLY_ME, "member_account_ids": ()}, + ], +) +def test_creation_replay_cannot_change_initial_access( + sqlite_session: Session, sqlite_session_factory: sessionmaker[Session], changes: dict[str, object] +) -> None: + _seed_members(sqlite_session) + service = KnowledgeFSControlSpaceCommandService(sqlite_session_factory) + service.create_provision_intent(_intent()) + + with pytest.raises(KnowledgeFSControlSpaceIntentConflictError, match="different initial access"): + service.create_provision_intent(_intent()._replace(**changes)) + + assert sqlite_session.scalar(select(func.count()).select_from(KnowledgeFSControlSpacePermission)) == 3 + assert sqlite_session.scalar(select(func.count()).select_from(KnowledgeFSLifecycleOutbox)) == 1 + + +def test_legacy_creation_replay_keeps_current_access_and_rejects_added_members( + sqlite_session: Session, sqlite_session_factory: sessionmaker[Session] +) -> None: + _seed_members(sqlite_session) + service = KnowledgeFSControlSpaceCommandService(sqlite_session_factory) + private_intent = _intent()._replace(visibility=KnowledgeFSControlSpaceVisibility.ONLY_ME, member_account_ids=()) + result = service.create_provision_intent(private_intent) + command = sqlite_session.get(KnowledgeFSLifecycleOutbox, result.outbox.id) + assert command is not None + command.command_payload = { + key: value + for key, value in command.command_payload.items() + if key not in {"initial_visibility", "initial_member_account_ids"} + } + sqlite_session.commit() + + replay = service.create_provision_intent( + private_intent._replace(visibility=KnowledgeFSControlSpaceVisibility.ALL_TEAM_MEMBERS) + ) + + assert replay.control_space.visibility is KnowledgeFSControlSpaceVisibility.ONLY_ME + assert replay.outbox.id == result.outbox.id + with pytest.raises(KnowledgeFSControlSpaceIntentConflictError): + service.create_provision_intent(_intent()) diff --git a/api/tests/unit_tests/services/test_knowledge_fs_product_application_service.py b/api/tests/unit_tests/services/test_knowledge_fs_product_application_service.py index 51f924b6b2d..f1222ece684 100644 --- a/api/tests/unit_tests/services/test_knowledge_fs_product_application_service.py +++ b/api/tests/unit_tests/services/test_knowledge_fs_product_application_service.py @@ -11,13 +11,14 @@ from services.knowledge_fs.control_plane_service import KnowledgeFSControlPlaneS from services.knowledge_fs.control_space_commands import KnowledgeFSControlSpaceCommandService from services.knowledge_fs.data_facade import KnowledgeFSDataFacade from services.knowledge_fs.product_application_service import KnowledgeFSProductApplicationService -from services.knowledge_fs.product_authorization import KnowledgeFSProductRBACPort +from services.knowledge_fs.product_authorization import DifyKnowledgeFSProductRBACPort, KnowledgeFSProductRBACPort from services.knowledge_fs.product_dto import KnowledgeFSSpaceCreatePayload, KnowledgeFSSpaceUpdatePayload from services.knowledge_fs.product_remote import ( KnowledgeFSOperationUnavailableError, KnowledgeFSProductRequestRejectedError, ) from services.knowledge_fs.product_service import KnowledgeFSProductService +from tests.unit_tests.config_override import apply_config_overrides class RejectingProduct: @@ -177,7 +178,7 @@ def test_product_application_create_enforces_workspace_rbac_before_command() -> commands.create_provision_intent.assert_not_called() -def test_product_application_create_preserves_model_profile_and_updates_non_private_visibility() -> None: +def test_product_application_create_preserves_model_profile_and_visibility_in_one_intent() -> None: application, product, control_plane, commands, _facade, _rbac = _application() payload = _create_payload( idempotency_key="create-once", @@ -206,13 +207,66 @@ def test_product_application_create_preserves_model_profile_and_updates_non_priv "stage": "mode-final", "value": 0.5, } - control_plane.update_visibility.assert_called_once_with( - tenant_id="tenant-1", - actor_account_id="account-1", - control_space_id="control-1", - visibility=KnowledgeFSControlSpaceVisibility.ALL_TEAM_MEMBERS, + assert intent.visibility is KnowledgeFSControlSpaceVisibility.ALL_TEAM_MEMBERS + assert intent.member_account_ids == () + control_plane.update_visibility.assert_not_called() + control_plane.replace_members.assert_not_called() + + +def test_product_application_create_passes_initial_viewers_in_the_atomic_intent() -> None: + application, _product, control_plane, commands, _facade, rbac = _application() + payload = _create_payload( + visibility="partial_members", + members=[{"account_id": "member-1", "role": "viewer"}], ) + application.create_space(tenant_id="tenant-1", account_id="account-1", payload=payload) + + intent = commands.create_provision_intent.call_args.args[0] + assert intent.visibility is KnowledgeFSControlSpaceVisibility.PARTIAL_MEMBERS + assert intent.member_account_ids == ("member-1",) + assert [item.kwargs["permission"].value for item in rbac.workspace_permission_allowed.call_args_list] == [ + "knowledge_space_create", + "knowledge_space_access_config", + ] + control_plane.update_visibility.assert_not_called() + control_plane.replace_members.assert_not_called() + + +@pytest.mark.parametrize("visibility", ["all_team_members", "partial_members"]) +def test_product_application_create_cannot_share_without_access_configuration_permission(visibility: str) -> None: + application, _product, _control_plane, commands, _facade, rbac = _application() + rbac.workspace_permission_allowed.side_effect = [True, False] + payload = _create_payload( + visibility=visibility, + members=[{"account_id": "member-1", "role": "viewer"}] if visibility == "partial_members" else [], + ) + + with pytest.raises(PermissionError, match="initial access configuration"): + application.create_space(tenant_id="tenant-1", account_id="account-1", payload=payload) + + commands.create_provision_intent.assert_not_called() + + +@pytest.mark.parametrize("visibility", ["all_team_members", "partial_members"]) +def test_product_application_create_shared_space_without_enterprise_rbac( + visibility: str, monkeypatch: pytest.MonkeyPatch +) -> None: + apply_config_overrides(monkeypatch, RBAC_ENABLED=False) + application, _product, _control_plane, commands, _facade, rbac = _application() + rbac.workspace_permission_allowed.side_effect = DifyKnowledgeFSProductRBACPort().workspace_permission_allowed + payload = _create_payload( + visibility=visibility, + members=[{"account_id": "member-1", "role": "viewer"}] if visibility == "partial_members" else [], + ) + + with patch("services.knowledge_fs.product_authorization.RBACService.CheckAccess.check") as check_access: + response = application.create_space(tenant_id="tenant-1", account_id="account-1", payload=payload) + + assert response.control_space_id == "control-1" + commands.create_provision_intent.assert_called_once() + check_access.assert_not_called() + def test_product_application_create_allows_model_setup_after_creation() -> None: application, _product, _control_plane, commands, _facade, _rbac = _application() diff --git a/packages/contracts/generated/api/console/knowledge-fs/types.gen.ts b/packages/contracts/generated/api/console/knowledge-fs/types.gen.ts index 9848135d231..85ba4a7adfa 100644 --- a/packages/contracts/generated/api/console/knowledge-fs/types.gen.ts +++ b/packages/contracts/generated/api/console/knowledge-fs/types.gen.ts @@ -87,6 +87,7 @@ export type KnowledgeFsSpaceCreatePayload = { kind: 'online_drive' } & KnowledgeFsInitialOnlineDriveSourcePayload) | null + members?: Array name: string retrieval?: KnowledgeFsRetrievalProfileIntent | null slug: string @@ -1418,6 +1419,11 @@ export type KnowledgeFsInitialOnlineDriveSourcePayload = { sync_policy?: 'custom' | 'daily' | 'manual' } +export type KnowledgeFsSpaceCreateMemberPayload = { + account_id: string + role: 'viewer' +} + export type KnowledgeFsRetrievalProfileIntent = { default_mode: 'deep' | 'fast' | 'research' reasoning_model: KnowledgeFsModelIntent diff --git a/packages/contracts/generated/api/console/knowledge-fs/zod.gen.ts b/packages/contracts/generated/api/console/knowledge-fs/zod.gen.ts index 3f14e201261..69f4ffc6e70 100644 --- a/packages/contracts/generated/api/console/knowledge-fs/zod.gen.ts +++ b/packages/contracts/generated/api/console/knowledge-fs/zod.gen.ts @@ -694,6 +694,14 @@ export const zKnowledgeFsModelIntent = z.object({ provider: z.string().min(1).max(255), }) +/** + * KnowledgeFSSpaceCreateMemberPayload + */ +export const zKnowledgeFsSpaceCreateMemberPayload = z.object({ + account_id: z.string().min(1).max(255), + role: z.literal('viewer'), +}) + /** * KnowledgeFSControlSpaceVisibility */ @@ -2576,6 +2584,7 @@ export const zKnowledgeFsSpaceCreatePayload = z.object({ zKnowledgeFsInitialOnlineDriveSourcePayload.extend({ kind: z.literal('online_drive') }), ]) .nullish(), + members: z.array(zKnowledgeFsSpaceCreateMemberPayload).max(1000).optional(), name: z.string().min(1).max(40), retrieval: zKnowledgeFsRetrievalProfileIntent.nullish(), slug: z diff --git a/web/app/components/datasets/settings/permission-selector/__tests__/index.spec.tsx b/web/app/components/datasets/settings/permission-selector/__tests__/index.spec.tsx index b30f1c5cff4..5db961877af 100644 --- a/web/app/components/datasets/settings/permission-selector/__tests__/index.spec.tsx +++ b/web/app/components/datasets/settings/permission-selector/__tests__/index.spec.tsx @@ -137,6 +137,64 @@ describe('PermissionSelector', () => { expect(screen.getByRole('dialog', { name: /form.permissions/ })).toBeInTheDocument() }) + it('freezes an already open member picker when disabled and permits changes after unlocking', async () => { + const user = userEvent.setup() + const onChange = vi.fn() + const onMemberSelect = vi.fn() + const props = { + ...defaultProps, + permission: DatasetPermission.partialMembers, + value: ['user-1', 'user-2'], + onChange, + onMemberSelect, + } + const { rerender } = renderSelector(props) + await user.click(screen.getByRole('button', { name: /Current User/ })) + const dialog = screen.getByRole('dialog', { name: /form.permissions/ }) + const member = within(dialog).getByRole('button', { name: /John Doe/ }) + const onlyMe = within(dialog).getByRole('radio', { name: /permissionsOnlyMe/ }) + const search = within(dialog).getByRole('searchbox', { name: /operation.search/ }) + + rerender() + + expect(member).toBeDisabled() + expect(onlyMe).toBeDisabled() + expect(search).toBeDisabled() + await user.click(member) + await user.click(onlyMe) + await user.type(search, 'Jane') + expect(search).toHaveValue('') + expect(onMemberSelect).not.toHaveBeenCalled() + expect(onChange).not.toHaveBeenCalled() + + rerender() + await user.click(member) + expect(onMemberSelect).toHaveBeenCalledWith(['user-1']) + await user.click(onlyMe) + expect(onChange).toHaveBeenCalledWith(DatasetPermission.onlyMe) + }) + + it('allows member selection when only visibility changes are disabled', async () => { + const user = userEvent.setup() + const onChange = vi.fn() + const onMemberSelect = vi.fn() + renderSelector({ + permission: DatasetPermission.partialMembers, + permissionChangeDisabled: true, + value: ['user-1'], + onChange, + onMemberSelect, + }) + await user.click(screen.getByRole('button', { name: /Current User/ })) + const dialog = screen.getByRole('dialog', { name: /form.permissions/ }) + const onlyMe = within(dialog).getByRole('radio', { name: /permissionsOnlyMe/ }) + expect(onlyMe).toBeDisabled() + await user.click(onlyMe) + await user.click(within(dialog).getByRole('button', { name: /John Doe/ })) + expect(onChange).not.toHaveBeenCalled() + expect(onMemberSelect).toHaveBeenCalledWith(['user-1', 'user-2']) + }) + it('exposes validation state and its accessible description on the trigger', () => { renderSelector({ ariaDescribedBy: 'permission-error', invalid: true }) @@ -145,6 +203,41 @@ describe('PermissionSelector', () => { expect(trigger).toHaveAttribute('aria-describedby', 'permission-error') }) + it('announces the external field label together with the current permission or member names', () => { + render( + <> + Knowledge permission + Choose who can read this knowledge. + , + ) + const props = { + ...defaultProps, + ariaLabelledBy: 'permission-label', + ariaDescribedBy: 'permission-help', + } + const { rerender } = renderSelector(props) + const trigger = screen.getByRole('button', { name: /^Knowledge permission / }) + expect(trigger).toHaveAccessibleName( + 'Knowledge permission datasetSettings.form.permissionsOnlyMe', + ) + expect(trigger).toHaveAccessibleDescription('Choose who can read this knowledge.') + + rerender() + expect(trigger).toHaveAccessibleName( + 'Knowledge permission datasetSettings.form.permissionsAllMember', + ) + + rerender( + , + ) + expect(trigger).toHaveAccessibleName('Knowledge permission Current User, John Doe') + expect(trigger).toHaveAccessibleDescription('Choose who can read this knowledge.') + }) + it.each([ [DatasetPermission.onlyMe, /permissionsOnlyMe/], [DatasetPermission.allTeamMembers, /permissionsAllMember/], diff --git a/web/app/components/datasets/settings/permission-selector/index.tsx b/web/app/components/datasets/settings/permission-selector/index.tsx index 4b3d1bd708a..3627eaac1a2 100644 --- a/web/app/components/datasets/settings/permission-selector/index.tsx +++ b/web/app/components/datasets/settings/permission-selector/index.tsx @@ -5,7 +5,7 @@ import { Popover, PopoverContent, PopoverTitle, PopoverTrigger } from '@langgeni import { RadioGroup } from '@langgenius/dify-ui/radio-group' import { useSuspenseQuery } from '@tanstack/react-query' import { useDebouncedValue } from 'foxact/use-debounced-value' -import { useMemo, useState } from 'react' +import { useId, useMemo, useState } from 'react' import { useTranslation } from 'react-i18next' import { SearchInput } from '@/app/components/base/search-input' import { userProfileQueryOptions } from '@/features/account-profile/client' @@ -16,19 +16,21 @@ import PermissionItem from './permission-item' type PermissionSelectorProps = { ariaDescribedBy?: string + ariaLabelledBy?: string disabled?: boolean disableWhenRbacEnabled?: boolean invalid?: boolean permissionChangeDisabled?: boolean permission?: DatasetPermission value: string[] - memberList: Member[] + memberList: Pick[] onChange: (permission?: DatasetPermission) => void onMemberSelect: (value: string[]) => void } const PermissionSelector = ({ ariaDescribedBy, + ariaLabelledBy, disabled, disableWhenRbacEnabled = true, invalid, @@ -40,6 +42,7 @@ const PermissionSelector = ({ onMemberSelect, }: PermissionSelectorProps) => { const { t } = useTranslation() + const selectedValueId = useId() const { data: userProfile } = useSuspenseQuery({ ...userProfileQueryOptions(), select: (data) => data.profile, @@ -51,7 +54,10 @@ const PermissionSelector = ({ const [keywords, setKeywords] = useState('') const debouncedKeywords = useDebouncedValue(keywords, 500) const searchKeywords = keywords ? debouncedKeywords : '' - const selectMember = (member: Member) => { + const isDisabledByRbac = disableWhenRbacEnabled && isRbacEnabled + const isDisabled = disabled || isDisabledByRbac + const selectMember = (member: PermissionSelectorProps['memberList'][number]) => { + if (isDisabled) return if (value.includes(member.id)) onMemberSelect(value.filter((id) => id !== member.id)) else onMemberSelect([...value, member.id]) } @@ -79,14 +85,13 @@ const PermissionSelector = ({ const showMe = userProfile.name.includes(searchKeywords) || userProfile.email.includes(searchKeywords) const selectedMemberNames = selectedMembers.map((member) => member.name).join(', ') - const isDisabledByRbac = disableWhenRbacEnabled && isRbacEnabled - const isDisabled = disabled || isDisabledByRbac const permissionLabel = t(($) => $['form.permissions'], { ns: 'datasetSettings' }) return (