diff --git a/desktop/crates/radicle-types/src/traits/release.rs b/desktop/crates/radicle-types/src/traits/release.rs index f895beeee..5f5995e0c 100644 --- a/desktop/crates/radicle-types/src/traits/release.rs +++ b/desktop/crates/radicle-types/src/traits/release.rs @@ -30,14 +30,13 @@ impl ReleaseFilter { pub trait Releases: Profile { /// List a repository's releases, newest first. /// - /// Scoped to releases created by a delegate and artifacts authored by a - /// delegate, or with an untrusted `filter` to those by non-delegates, so - /// the two scopes never overlap. Artifacts redacted by a trusted party are - /// hidden unless `filter` shows them. A release with no artifact in scope - /// shows every author's. A release whose artifacts were all redacted is - /// hidden with them; a release with no artifacts is shown. This is the view - /// `rad-artifact list` gives. Without `take` the full list is returned and - /// `skip` is ignored. + /// Scoped to releases created by a delegate, or with an untrusted `filter` + /// to those by non-delegates, so the two scopes never overlap. Each release + /// lists only the artifacts authored by a delegate, in either scope. + /// Artifacts redacted by a trusted party are hidden unless `filter` shows + /// them. A release whose artifacts were all redacted is hidden with them; a + /// release with no artifacts is shown. Without `take` the full list is + /// returned and `skip` is ignored. fn list_releases( &self, rid: identity::RepoId, @@ -60,19 +59,14 @@ pub trait Releases: Profile { .filter_map(Result::ok) .filter(|(_, release)| filters.shows_release(release)); - // With no artifact by an author in scope, fall back to every author, as - // the release page does, so a teaser never counts fewer artifacts than - // its page lists. A redacted artifact in scope keeps the scope. - let fallback = Filters { - scope: Scope::All, + // A release page opens on the delegates' artifacts, so a teaser counts + // those in either scope. + let artifact_filters = Filters { + scope: Scope::Trusted, ..filters }; let summary = |(id, release): (radicle::cob::ObjectId, radicle_artifact::Release)| { - let has_trusted = release - .artifacts() - .values() - .any(|artifact| filters.trust.admits(filters.scope, artifact.author())); - let artifacts = if has_trusted { filters } else { fallback } + let artifacts = artifact_filters .artifacts(&release) .map(|(cid, artifact)| { cobs::release::Artifact::new(cid, artifact, store.delegates(), &aliases) @@ -195,8 +189,8 @@ mod test { } #[test] - fn list_falls_back_to_every_author_without_delegate_artifact() { - assert_eq!(listed_artifacts(|_, _| {}), 1); + fn list_never_widens_to_non_delegate_artifact() { + assert_eq!(listed_artifacts(|_, _| {}), 0); } #[test] @@ -293,6 +287,40 @@ mod test { assert_eq!(listed(ReleaseScope::Untrusted), [theirs]); } + #[test] + fn list_others_releases_with_delegate_artifacts() { + let tmp = tempfile::tempdir().unwrap(); + let profile = test::profile(&tmp.path().join("home"), [0xff; 32]); + let signer = SigningKey::from_seed(Seed::new([0xff; 32])); + let other = SigningKey::from_seed(Seed::new([0xee; 32])); + let (rid, _, _, head) = + fixtures::project(tmp.path().join("project"), &profile.storage, &signer).unwrap(); + { + let repo = profile.storage.repository(rid).unwrap(); + let mut releases = ArtifactStore::open(&repo).unwrap(); + let mut theirs = releases + .create(radicle::git::Oid::from(head), None, &other) + .unwrap(); + theirs + .register_artifact(Cid::from_str(CID).unwrap(), "bin".into(), &signer) + .unwrap(); + theirs + .register_artifact(Cid::from_str(OTHER_CID).unwrap(), "eve".into(), &other) + .unwrap(); + } + + let filter = ReleaseFilter { + scope: ReleaseScope::Untrusted, + show_redacted: false, + }; + let list = AppState { profile } + .list_releases(rid, Some(filter), None, None) + .unwrap(); + let artifacts = &list.content[0].artifacts; + assert_eq!(artifacts.len(), 1); + assert_eq!(artifacts[0].name, "bin"); + } + #[test] fn release_counts_split_delegates_from_others() { let tmp = tempfile::tempdir().unwrap(); diff --git a/desktop/docs/adr/0004-untrusted-releases-and-artifacts.md b/desktop/docs/adr/0004-untrusted-releases-and-artifacts.md index 532a045a2..ca960a4c8 100644 --- a/desktop/docs/adr/0004-untrusted-releases-and-artifacts.md +++ b/desktop/docs/adr/0004-untrusted-releases-and-artifacts.md @@ -53,11 +53,18 @@ without alarm. It shows: counts delegates only, as anyone can make keys that attest their own artifact. The full list still shows every attestation. -**Keep untrusted content visible.** Others' releases and artifacts stay listed -by default, behind their own filter. +**Open on Delegates, and never switch scope by itself.** The releases list and +the artifacts of a release always open on Delegates, also when Delegates is +empty. The Others filter shows whenever Others has content, so untrusted content +is one deliberate click away. An empty Delegates scope tells the user that the +delegates published nothing. Opening on Others would hide that, and would show +untrusted content the user did not ask for. ## Alternatives considered +- **Open on Others when Delegates is empty.** This avoids an empty page, but + shows untrusted content without the user asking for it, and hides that the + delegates published nothing. - **Hide Others by default**, behind a global setting. This protects users best, and avoids showing links to malware at all, which is a reputation risk a warning doesn't remove. It also hides legitimate contributions, such as builds @@ -77,4 +84,6 @@ by default, behind their own filter. its title, tag and commit are not trusted. - Delegation is the only trust signal. Release managers who are not delegates show as Others until releases can use canonical ref rules. -- radicle-explorer should show the same warnings so that both clients agree. +- An empty Delegates scope shows an empty page, also when Others has content. +- radicle-explorer shows the same warnings and opens on the same scope, so that + both clients agree. diff --git a/desktop/src/components/ReleaseTeaser.svelte b/desktop/src/components/ReleaseTeaser.svelte index 407ee6db5..447faa541 100644 --- a/desktop/src/components/ReleaseTeaser.svelte +++ b/desktop/src/components/ReleaseTeaser.svelte @@ -1,6 +1,5 @@ @@ -79,7 +77,6 @@ resource: "repo.release", rid, release: release.id, - scope, }); }}>
diff --git a/desktop/src/lib/releases.ts b/desktop/src/lib/releases.ts index 7936cc515..c0fdaf21e 100644 --- a/desktop/src/lib/releases.ts +++ b/desktop/src/lib/releases.ts @@ -1,35 +1,21 @@ import type { Author } from "@bindings/cob/Author"; import type { Artifact } from "@bindings/cob/release/Artifact"; import type { Location } from "@bindings/cob/release/Location"; -import type { ReleaseCounts } from "@bindings/cob/release/ReleaseCounts"; import type { ReleaseScope } from "@bindings/cob/release/ReleaseScope"; import type { Commit } from "@bindings/repo/Commit"; -// With no scope asked for and no delegate releases, open on the untrusted -// scope rather than on an empty list. -export function releaseListScope( - requested: ReleaseScope | undefined, - counts: ReleaseCounts, -): ReleaseScope { - if (requested !== undefined) { - return requested; - } - return counts.delegate === 0 && counts.other > 0 ? "untrusted" : "trusted"; -} - export interface ArtifactView { - scope: ReleaseScope; shown: Artifact[]; redactedCount: number; counts: Record; showFilters: boolean; } -// Falls back to the other scope when the one asked for is empty. +// Never falls back to the other scope; see docs/adr/0004. export function artifactView( artifacts: Artifact[], delegates: Set, - wanted: ReleaseScope, + scope: ReleaseScope, showRedacted: boolean, ): ArtifactView { const byScope: Record = { @@ -38,18 +24,15 @@ export function artifactView( }; const visible = (list: Artifact[]) => showRedacted ? list : list.filter(a => !a.redacted); - const other = wanted === "trusted" ? "untrusted" : "trusted"; - const scope = byScope[wanted].length > 0 ? wanted : other; return { - scope, shown: visible(byScope[scope]), redactedCount: byScope[scope].filter(a => a.redacted).length, counts: { trusted: visible(byScope.trusted).length, untrusted: visible(byScope.untrusted).length, }, - showFilters: byScope.trusted.length > 0 && byScope.untrusted.length > 0, + showFilters: scope === "untrusted" || byScope.untrusted.length > 0, }; } diff --git a/desktop/src/views/repo/Release.svelte b/desktop/src/views/repo/Release.svelte index 5b3b40307..31ecee96d 100644 --- a/desktop/src/views/repo/Release.svelte +++ b/desktop/src/views/repo/Release.svelte @@ -36,31 +36,29 @@ config: Config; release: Release; scope?: ReleaseScope; - artifactScope?: ReleaseScope; } /* eslint-disable prefer-const */ - let { repo, config, release, scope, artifactScope }: Props = $props(); + let { repo, config, release, scope }: Props = $props(); /* eslint-enable prefer-const */ const ownDid = $derived(didFromPublicKey(config.publicKey)); const title = $derived(release.title || release.tagName || release.id); const delegateIds = $derived(new Set(repo.delegates.map(d => d.did))); + const listScope: ReleaseScope = $derived( + delegateIds.has(release.creator.did) ? "trusted" : "untrusted", + ); let showRedacted = $state(false); + const artifactScope: ReleaseScope = $derived(scope ?? "trusted"); const view = $derived( - artifactView( - release.artifacts, - delegateIds, - artifactScope ?? "trusted", - showRedacted, - ), + artifactView(release.artifacts, delegateIds, artifactScope, showRedacted), ); const warning = $derived( - releaseWarning(release.creator.did, delegateIds, view.scope), + releaseWarning(release.creator.did, delegateIds, artifactScope), ); const untrustedWarning = $derived( warning === "release" @@ -355,19 +353,17 @@ Releases - {#if scope} - - - {/if} + +
@@ -440,13 +436,12 @@ styleHeight="1.75rem" bordered flatRight - active={view.scope === "trusted"} + active={artifactScope === "trusted"} onclick={() => router.push({ resource: "repo.release", rid: repo.rid, release: release.id, - scope, })}> Delegates {view.counts.trusted} @@ -455,15 +450,14 @@ styleHeight="1.75rem" bordered flatLeft - active={view.scope === "untrusted"} + active={artifactScope === "untrusted"} title="Non-delegates" onclick={() => router.push({ resource: "repo.release", rid: repo.rid, release: release.id, - scope, - artifactScope: "untrusted", + scope: "untrusted", })}> Others @@ -498,7 +492,11 @@ {#if view.shown.length === 0}
- No artifacts + {!view.showFilters + ? "No artifacts" + : artifactScope === "trusted" + ? "No artifacts by delegates" + : "No artifacts by others"}
{/if} diff --git a/desktop/src/views/repo/Releases.svelte b/desktop/src/views/repo/Releases.svelte index 1ee02bfc9..c07175da2 100644 --- a/desktop/src/views/repo/Releases.svelte +++ b/desktop/src/views/repo/Releases.svelte @@ -183,7 +183,7 @@ onState={list.persistScroll}> {#snippet row(release)}
- +
{/snippet} diff --git a/desktop/src/views/repo/router.ts b/desktop/src/views/repo/router.ts index 01aab3f0c..501ff652c 100644 --- a/desktop/src/views/repo/router.ts +++ b/desktop/src/views/repo/router.ts @@ -24,7 +24,6 @@ import { cachedGetDiffText, invoke, } from "@app/lib/invoke"; -import { releaseListScope } from "@app/lib/releases"; import type { LoadedRoute, SidebarData } from "@app/lib/router/definitions"; import { loadSidebarData } from "@app/lib/router/definitions"; import { unreachable } from "@app/lib/utils"; @@ -215,9 +214,7 @@ export interface RepoReleaseRoute { resource: "repo.release"; rid: string; release: string; - // The release list the user came from, shown in the breadcrumb. scope?: ReleaseScope; - artifactScope?: ReleaseScope; } export interface LoadedRepoReleaseRoute { @@ -227,7 +224,6 @@ export interface LoadedRepoReleaseRoute { config: Config; release: Release; scope?: ReleaseScope; - artifactScope?: ReleaseScope; sidebarData: SidebarData; }; } @@ -570,8 +566,8 @@ export function listReleases( export async function loadReleases( route: RepoReleasesRoute, ): Promise { - let scope = route.scope ?? "trusted"; - const [sidebarData, repo, firstPage, releaseCounts] = await Promise.all([ + const scope = route.scope ?? "trusted"; + const [sidebarData, repo, releases, releaseCounts] = await Promise.all([ loadSidebarData(), invoke("repo_by_id", { rid: route.rid, @@ -581,15 +577,9 @@ export async function loadReleases( rid: route.rid, }), ]); - let releases = firstPage; - - const fallback = releaseListScope(route.scope, releaseCounts); - if (fallback !== scope) { - scope = fallback; - releases = await listReleases(route.rid, scope, 0, RELEASES_PER_PAGE); - } - - const showFilters = releaseCounts.delegate > 0 && releaseCounts.other > 0; + // Others is one click away whenever it has releases, and Delegates is one + // click back from it; see docs/adr/0004. + const showFilters = scope === "untrusted" || releaseCounts.other > 0; return { resource: "repo.releases", @@ -623,7 +613,6 @@ export async function loadRelease( return loadReleases({ resource: "repo.releases", rid: route.rid, - scope: route.scope, }); } @@ -635,7 +624,6 @@ export async function loadRelease( config: sidebarData.config, release, scope: route.scope, - artifactScope: route.artifactScope, }, }; } @@ -706,11 +694,6 @@ export function repoRouteToPath(route: RepoRoute): string { let url = [...pathSegments, "releases", route.release].join("/"); if (route.scope) { searchParams.set("scope", route.scope); - } - if (route.artifactScope === "untrusted") { - searchParams.set("artifacts", "untrusted"); - } - if (searchParams.size > 0) { url += `?${searchParams}`; } return url; @@ -818,17 +801,7 @@ export function repoUrlToRoute( const id = segments.shift(); const scope = parseReleaseScope(searchParams.get("scope")); if (id) { - const artifactScope = - searchParams.get("artifacts") === "untrusted" - ? "untrusted" - : undefined; - return { - resource: "repo.release", - rid, - release: id, - scope, - artifactScope, - }; + return { resource: "repo.release", rid, release: id, scope }; } else { return { resource: "repo.releases", rid, scope }; } diff --git a/desktop/tests/e2e/repo/createRelease.spec.ts b/desktop/tests/e2e/repo/createRelease.spec.ts index 22866c10a..9ff9dd2e2 100644 --- a/desktop/tests/e2e/repo/createRelease.spec.ts +++ b/desktop/tests/e2e/repo/createRelease.spec.ts @@ -310,6 +310,9 @@ test("an artifact by a non-delegate shows a warning", async ({ }); await reload(page); + // Releases always open on Delegates, see docs/adr/0004. + await expect(page.getByText("No artifacts by delegates")).toBeVisible(); + await page.getByRole("button", { name: /Others/ }).click(); await expect( page.getByRole("note").getByText("Not from delegates."), ).toBeVisible(); diff --git a/desktop/tests/unit/releases.test.ts b/desktop/tests/unit/releases.test.ts index 363c457b3..8eb9f38ad 100644 --- a/desktop/tests/unit/releases.test.ts +++ b/desktop/tests/unit/releases.test.ts @@ -1,5 +1,4 @@ import type { Artifact } from "@bindings/cob/release/Artifact"; -import type { ReleaseCounts } from "@bindings/cob/release/ReleaseCounts"; import type { Commit } from "@bindings/repo/Commit"; import { describe, expect, test } from "vitest"; @@ -15,7 +14,6 @@ import { matchCommits, parseMetadataValue, redactedByDelegate, - releaseListScope, releaseWarning, webLocations, } from "@app/lib/releases"; @@ -40,27 +38,6 @@ function artifact(cid: string, overrides: Partial = {}): Artifact { }; } -function counts(delegate: number, other: number): ReleaseCounts { - return { delegate, delegateRedacted: 0, other, otherRedacted: 0 }; -} - -describe("releaseListScope", () => { - test("keeps a requested scope", () => { - expect(releaseListScope("trusted", counts(0, 3))).toBe("trusted"); - }); - - test("opens on untrusted when only others released", () => { - expect(releaseListScope(undefined, counts(0, 3))).toBe("untrusted"); - }); - - test.each([ - [1, 3], - [0, 0], - ])("opens on trusted with %d delegate and %d other", (d, o) => { - expect(releaseListScope(undefined, counts(d, o))).toBe("trusted"); - }); -}); - describe("artifactView", () => { const ours = artifact("ours"); const ourRedacted = artifact("ours-redacted", { redacted: true }); @@ -68,15 +45,24 @@ describe("artifactView", () => { test("shows the wanted scope", () => { const view = artifactView([ours, theirs], delegates, "untrusted", false); - expect(view.scope).toBe("untrusted"); expect(view.shown).toEqual([theirs]); expect(view.showFilters).toBe(true); }); - test("falls back to the other scope when the wanted one is empty", () => { + test("keeps the wanted scope when it is empty", () => { const view = artifactView([theirs], delegates, "trusted", false); - expect(view.scope).toBe("untrusted"); - expect(view.shown).toEqual([theirs]); + expect(view.shown).toEqual([]); + expect(view.showFilters).toBe(true); + }); + + test("keeps the filter on others without artifacts by others", () => { + const view = artifactView([ours], delegates, "untrusted", false); + expect(view.shown).toEqual([]); + expect(view.showFilters).toBe(true); + }); + + test("hides the filter without artifacts by others", () => { + const view = artifactView([ours], delegates, "trusted", false); expect(view.showFilters).toBe(false); }); @@ -103,7 +89,6 @@ describe("artifactView", () => { "trusted", false, ); - expect(view.scope).toBe("trusted"); expect(view.shown).toEqual([]); expect(view.counts).toEqual({ trusted: 0, untrusted: 1 }); }); diff --git a/desktop/tests/unit/repoRouter.test.ts b/desktop/tests/unit/repoRouter.test.ts index cbe24385a..08872fb40 100644 --- a/desktop/tests/unit/repoRouter.test.ts +++ b/desktop/tests/unit/repoRouter.test.ts @@ -101,27 +101,15 @@ describe("repoRouteToPath and repoUrlToRoute round trip", () => { rid, release: "abc", scope: undefined, - artifactScope: undefined, }, ], [ - "/repos/RID/releases/abc?scope=trusted", - { - resource: "repo.release", - rid, - release: "abc", - scope: "trusted", - artifactScope: undefined, - }, - ], - [ - "/repos/RID/releases/abc?scope=untrusted&artifacts=untrusted", + "/repos/RID/releases/abc?scope=untrusted", { resource: "repo.release", rid, release: "abc", scope: "untrusted", - artifactScope: "untrusted", }, ], ])("%s", (template, route) => { @@ -173,27 +161,12 @@ describe("repoUrlToRoute", () => { }); }); - test("leaves the trusted artifact scope out of the URL", () => { - expect( - repoRouteToPath({ - resource: "repo.release", - rid, - release: "abc", - scope: undefined, - artifactScope: "trusted", - }), - ).toBe(`/repos/${rid}/releases/abc`); - }); - test("ignores an unknown release scope", () => { - expect( - parse(`/repos/${rid}/releases/abc?scope=bogus&artifacts=bogus`), - ).toEqual({ + expect(parse(`/repos/${rid}/releases/abc?scope=bogus`)).toEqual({ resource: "repo.release", rid, release: "abc", scope: undefined, - artifactScope: undefined, }); });