fix: post-review followups on slug-vs-UUID refactor
Addresses every finding from the four-agent review of commit aa493b9.
Dashboard — honor redirect_to on subtab loadApp() (closes the silent
historical-slug redirect UX gap):
- queues/+page.svelte
- queues/[name]/+page.svelte
- files/+page.svelte
- dead-letters/+page.svelte
After a rename, the URL bar now reflects the canonical slug instead
of silently rendering the renamed app's data under the stale URL.
Mirrors the established pattern at apps/[slug]/+page.svelte:619-623.
manager-core:
- queues_api.rs IntoResponse now uses the JSON envelope shape
`{"error": "..."}` consistent with every sibling admin api file.
- triggers_api::delete_trigger reordered: cap check fires BEFORE the
trigger load, closing the 404-vs-403 existence side channel an
unauthorized caller could otherwise probe.
- InMemoryAppRepo mocks in topics_api + triggers_api now implement
get_by_slug + get_by_slug_or_history (previously
`unimplemented!()`), unblocking handler-level slug-input tests.
- Added 4 slug-acceptance tests to topics_api and 2 to triggers_api
(slug-resolves, unknown-slug-404, historical-slug-resolves,
create-via-slug). Also added delete-without-cap-is-forbidden test
pinning the new cap-first order.
e2e:
- navigation/tabs.spec.ts split per-tab so a regression on one tab
no longer masks regressions on the others.
- Negative assertion widened: captures every /api/v1/admin/apps/*
response and fails on any 4xx/5xx — not just the literal "Cannot
parse" string. Catches a broader regression shape.
- networkidle replaced with `expect(<main>).toBeVisible()` —
networkidle is officially discouraged for SPAs and was at risk of
timing out behind the queues auto-refresh.
- Cleanup registration moved BEFORE the create-app API call so a
flaky create still gets swept up.
- Queue drilldown route /queues/[name] now covered.
- Stable `data-testid="queues-empty-state"` replaces fragile
UI-copy substring match for the positive assertion.
- Header comment now spells out what this spec does and doesn't
catch.
docs:
- serverless_cloud_blueprint.md: slug-history described as
"200 OK + redirect_to" JSON envelope rather than "301 redirect"
— matches what apps_api actually implements (SPA can't honor a
mid-tree HTTP redirect).
Unit-test gap (acknowledged): queues_api, files_api, secrets_api,
dead_letters_api have zero in-process tests. Adding them properly
needs a shared mock-repo helper crate — the standalone trait surface
(QueueRepo + TriggerRepo + ScriptRepository + AuthzRepo + repo-
specific) is ~30 methods per file. Documented inline in queues_api.rs
near the resolver. Integration coverage via crates/picloud/tests/ and
the new e2e spec cover the same paths end-to-end.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -593,9 +593,16 @@ async fn delete_trigger(
|
||||
Path((id_or_slug, trigger_id)): Path<(String, TriggerId)>,
|
||||
) -> Result<StatusCode, TriggersApiError> {
|
||||
let app_id = resolve_app(&*s.apps, &id_or_slug).await?;
|
||||
// Load the trigger so we can confirm it belongs to the right
|
||||
// app; this prevents a caller from deleting a trigger by id alone
|
||||
// when their capability is bound to a different app.
|
||||
// Cap check first so an unauthorized caller can't probe trigger
|
||||
// existence via 404-vs-403 discrimination. Only after we know the
|
||||
// caller may manage triggers in this app do we load the row to
|
||||
// verify the cross-app guard.
|
||||
require(
|
||||
s.authz.as_ref(),
|
||||
&principal,
|
||||
Capability::AppManageTriggers(app_id),
|
||||
)
|
||||
.await?;
|
||||
let trigger = s
|
||||
.triggers
|
||||
.get(trigger_id)
|
||||
@@ -604,12 +611,6 @@ async fn delete_trigger(
|
||||
if trigger.app_id != app_id {
|
||||
return Err(TriggersApiError::NotFound(trigger_id));
|
||||
}
|
||||
require(
|
||||
s.authz.as_ref(),
|
||||
&principal,
|
||||
Capability::AppManageTriggers(app_id),
|
||||
)
|
||||
.await?;
|
||||
if !s.triggers.delete(trigger_id).await? {
|
||||
return Err(TriggersApiError::NotFound(trigger_id));
|
||||
}
|
||||
@@ -1106,15 +1107,24 @@ mod tests {
|
||||
}
|
||||
async fn get_by_slug(
|
||||
&self,
|
||||
_slug: &str,
|
||||
slug: &str,
|
||||
) -> Result<Option<App>, crate::repo::ScriptRepositoryError> {
|
||||
unimplemented!()
|
||||
let map = self.existing.lock().await;
|
||||
Ok(map.values().find(|a| a.slug == slug).cloned())
|
||||
}
|
||||
async fn get_by_slug_or_history(
|
||||
&self,
|
||||
_slug: &str,
|
||||
slug: &str,
|
||||
) -> Result<Option<AppLookup>, crate::repo::ScriptRepositoryError> {
|
||||
unimplemented!()
|
||||
let map = self.existing.lock().await;
|
||||
Ok(map
|
||||
.values()
|
||||
.find(|a| a.slug == slug)
|
||||
.cloned()
|
||||
.map(|app| AppLookup {
|
||||
app,
|
||||
redirected: false,
|
||||
}))
|
||||
}
|
||||
async fn update(
|
||||
&self,
|
||||
@@ -1593,6 +1603,27 @@ mod tests {
|
||||
assert!(matches!(err, TriggersApiError::NotFound(_)));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn delete_without_manage_cap_is_forbidden_not_notfound() {
|
||||
// Confirms the cap check fires before the trigger load — an
|
||||
// unauthorized caller can't probe trigger existence by trying
|
||||
// to delete one and reading 404 vs 403.
|
||||
let app_id = AppId::new();
|
||||
let state = state_with(Arc::new(AlwaysDenyAuthzRepo), app_id);
|
||||
let unknown_trigger = TriggerId::new();
|
||||
let res = delete_trigger(
|
||||
State(state),
|
||||
Extension(member_principal()),
|
||||
Path((app_id.to_string(), unknown_trigger)),
|
||||
)
|
||||
.await;
|
||||
let err = res.expect_err("denied cap should surface as Forbidden");
|
||||
assert!(
|
||||
matches!(err, TriggersApiError::Forbidden),
|
||||
"got {err:?}; expected Forbidden so unauthorized callers can't probe existence"
|
||||
);
|
||||
}
|
||||
|
||||
// ----------------------------------------------------------------
|
||||
// v1.1.3: kind + cross-app target validation on trigger create.
|
||||
// ----------------------------------------------------------------
|
||||
@@ -2220,4 +2251,36 @@ mod tests {
|
||||
TriggersApiError::Forbidden
|
||||
));
|
||||
}
|
||||
|
||||
// ----------------------------------------------------------------
|
||||
// Slug-or-UUID path acceptance (post-aa493b9 refactor).
|
||||
// ----------------------------------------------------------------
|
||||
|
||||
#[tokio::test]
|
||||
async fn list_accepts_slug_path() {
|
||||
let app_id = AppId::new();
|
||||
let state = state_with(Arc::new(AlwaysAllowAuthzRepo), app_id);
|
||||
let Json(resp) = list_triggers(
|
||||
State(state),
|
||||
Extension(member_principal()),
|
||||
Path("test".into()),
|
||||
)
|
||||
.await
|
||||
.expect("slug path should resolve");
|
||||
assert!(resp.triggers.is_empty());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn list_unknown_slug_returns_404() {
|
||||
let app_id = AppId::new();
|
||||
let state = state_with(Arc::new(AlwaysAllowAuthzRepo), app_id);
|
||||
let err = list_triggers(
|
||||
State(state),
|
||||
Extension(member_principal()),
|
||||
Path("does-not-exist".into()),
|
||||
)
|
||||
.await
|
||||
.expect_err("unknown slug should 404");
|
||||
assert!(matches!(err, TriggersApiError::AppNotFound(_)));
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user