fix(website): stop club rank matrix from faking a permission save (WEB-W1)
The Ranks & Roles "Operational Permission Matrix" previously waited ~400ms and showed a "Saved" state without persisting anything. No Firestore document, API route, or security rule stores or enforces a per-rank capability matrix; club authorization is role-based (Owner/Admin via getPortalPermissions). Persisting the matrix would require frozen security-rule changes and would create an unenforced permission model that misleads owners. Make the control truthful instead of faking success: - Render the matrix as a read-only reference of the standard governance model by default (persistenceAvailable=false): no Save/Reset controls, disabled toggles, a "View-only reference" indicator, and a notice stating that access is enforced by Owner/Admin roles and per-rank customization is planned. - Remove the fake setTimeout save handler and the unconditional success state. - Retain a forward-compatible editable path behind persistenceAvailable for when a backend contract ships. - Document the precise backend contract required to enable persistence. Tests: read-only matrix renders no Save control, reports no save, and does not mutate permissions even for Owner/Admin; editable path still fires toggle/save /reset when persistenceAvailable=true. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Developer
parent
493192078f
commit
c014c2f04f
|
|
@ -34,7 +34,6 @@ export default function ClubFounderRanksRolesPage({ params }: PageProps) {
|
|||
const [members, setMembers] = useState<ClubMember[]>([]);
|
||||
const [permissions, setPermissions] = useState(DEFAULT_RANK_PERMISSIONS);
|
||||
const [selectedRankId, setSelectedRankId] = useState<string | null>('president');
|
||||
const [isSavingPermissions, setIsSavingPermissions] = useState(false);
|
||||
const [loading, setLoading] = useState(true);
|
||||
const [error, setError] = useState<string | null>(null);
|
||||
const [unauthenticated, setUnauthenticated] = useState(false);
|
||||
|
|
@ -104,16 +103,6 @@ export default function ClubFounderRanksRolesPage({ params }: PageProps) {
|
|||
}));
|
||||
}
|
||||
|
||||
function handleResetDefaults() {
|
||||
setPermissions(DEFAULT_RANK_PERMISSIONS);
|
||||
}
|
||||
|
||||
async function handleSavePermissions() {
|
||||
setIsSavingPermissions(true);
|
||||
await new Promise((r) => setTimeout(r, 400));
|
||||
setIsSavingPermissions(false);
|
||||
}
|
||||
|
||||
if (loading) {
|
||||
return (
|
||||
<div className="min-h-screen bg-brand-bg telemetry-grid-bg p-6">
|
||||
|
|
@ -220,16 +209,23 @@ export default function ClubFounderRanksRolesPage({ params }: PageProps) {
|
|||
/>
|
||||
|
||||
{/* Interactive Permission Toggle Switches Matrix */}
|
||||
{/*
|
||||
Per-rank capability permissions have no server-side persistence contract yet.
|
||||
The enforced authorization model is role-based (Owner / Admin) via
|
||||
getPortalPermissions in lib/club-data.ts, not this rank matrix. Rendering the
|
||||
matrix read-only (persistenceAvailable={false}) keeps it truthful: it shows the
|
||||
standard governance conventions without claiming a save that does not occur.
|
||||
See website/docs/backend-contracts/club-rank-permissions.md for the proposed
|
||||
contract that would flip persistenceAvailable to true.
|
||||
*/}
|
||||
<ClubPermissionMatrix
|
||||
ranks={DEFAULT_CLUB_RANKS}
|
||||
capabilities={DEFAULT_CLUB_CAPABILITIES}
|
||||
permissions={permissions}
|
||||
selectedRankId={selectedRankId}
|
||||
onTogglePermission={handleTogglePermission}
|
||||
onResetDefaults={handleResetDefaults}
|
||||
onSavePermissions={handleSavePermissions}
|
||||
isSaving={isSavingPermissions}
|
||||
canEdit={context.permissions.canManageSettings}
|
||||
persistenceAvailable={false}
|
||||
/>
|
||||
</div>
|
||||
</FounderPortalShell>
|
||||
|
|
|
|||
|
|
@ -9,10 +9,22 @@ interface ClubPermissionMatrixProps {
|
|||
permissions: Record<string, Record<ClubCapability, boolean>>;
|
||||
selectedRankId: string | null;
|
||||
onTogglePermission: (rankId: string, capability: ClubCapability) => void;
|
||||
onResetDefaults: () => void;
|
||||
onSavePermissions: () => Promise<void>;
|
||||
isSaving: boolean;
|
||||
onResetDefaults?: () => void;
|
||||
onSavePermissions?: () => Promise<void>;
|
||||
isSaving?: boolean;
|
||||
canEdit: boolean;
|
||||
/**
|
||||
* Whether a server-side persistence contract for per-rank capability
|
||||
* permissions actually exists. Defaults to false.
|
||||
*
|
||||
* The enforced club authorization model is role-based (Owner / Admin), not
|
||||
* rank-based (see getPortalPermissions in lib/club-data.ts). No Firestore
|
||||
* document or API persists this capability matrix today, so by default the
|
||||
* matrix renders as a read-only reference of the standard governance model.
|
||||
* The editable + save path is retained for when a backend contract ships
|
||||
* (see website/docs/backend-contracts/club-rank-permissions.md).
|
||||
*/
|
||||
persistenceAvailable?: boolean;
|
||||
}
|
||||
|
||||
export function ClubPermissionMatrix({
|
||||
|
|
@ -23,12 +35,19 @@ export function ClubPermissionMatrix({
|
|||
onTogglePermission,
|
||||
onResetDefaults,
|
||||
onSavePermissions,
|
||||
isSaving,
|
||||
isSaving = false,
|
||||
canEdit,
|
||||
persistenceAvailable = false,
|
||||
}: ClubPermissionMatrixProps) {
|
||||
const [saveSuccess, setSaveSuccess] = useState(false);
|
||||
|
||||
// Edit + save controls are only live when the caller both grants edit rights
|
||||
// AND a real persistence backend is wired. Without persistence the matrix is
|
||||
// a truthful read-only reference: it never mutates and never reports a save.
|
||||
const editable = canEdit && persistenceAvailable && typeof onSavePermissions === 'function';
|
||||
|
||||
async function handleSave() {
|
||||
if (!editable || !onSavePermissions) return;
|
||||
await onSavePermissions();
|
||||
setSaveSuccess(true);
|
||||
setTimeout(() => setSaveSuccess(false), 2500);
|
||||
|
|
@ -48,12 +67,13 @@ export function ClubPermissionMatrix({
|
|||
</h2>
|
||||
</div>
|
||||
<p className="font-body text-xs sm:text-sm text-slate-400 mt-1">
|
||||
Configure delegated operational privileges per squad rank. Administrative roles enforce backend limits.
|
||||
Standard delegated operational privileges per squad rank. Administrative roles enforce backend limits.
|
||||
</p>
|
||||
</div>
|
||||
|
||||
{canEdit && (
|
||||
{editable ? (
|
||||
<div className="flex items-center gap-2.5 self-start sm:self-center">
|
||||
{onResetDefaults && (
|
||||
<button
|
||||
type="button"
|
||||
onClick={onResetDefaults}
|
||||
|
|
@ -61,6 +81,7 @@ export function ClubPermissionMatrix({
|
|||
>
|
||||
Reset Defaults
|
||||
</button>
|
||||
)}
|
||||
<button
|
||||
type="button"
|
||||
disabled={isSaving}
|
||||
|
|
@ -85,9 +106,33 @@ export function ClubPermissionMatrix({
|
|||
)}
|
||||
</button>
|
||||
</div>
|
||||
) : (
|
||||
<span
|
||||
className="inline-flex items-center gap-1.5 self-start sm:self-center px-3 py-1.5 rounded-xl text-[11px] font-mono text-slate-400 bg-surface-container border border-brand-border/60"
|
||||
title="Per-rank permission customization is not yet available. Software access is enforced by Owner and Admin roles."
|
||||
>
|
||||
<span className="material-symbols-outlined text-[15px]">visibility</span>
|
||||
View-only reference
|
||||
</span>
|
||||
)}
|
||||
</div>
|
||||
|
||||
{/* Truthful availability notice: shown whenever persistence is not wired. */}
|
||||
{!editable && (
|
||||
<div className="flex items-start gap-2.5 p-3 rounded-xl bg-amber-500/10 border border-amber-500/30 text-xs font-body text-amber-200/90">
|
||||
<span className="material-symbols-outlined text-[16px] text-amber-400 shrink-0 mt-0.5">
|
||||
info
|
||||
</span>
|
||||
<span>
|
||||
This matrix shows MotorSignal’s standard rank conventions for reference. Custom per-rank
|
||||
permissions are not saved yet. Who can actually manage this club is enforced by the{' '}
|
||||
<strong className="font-semibold text-amber-100">Owner</strong> and{' '}
|
||||
<strong className="font-semibold text-amber-100">Admin</strong> roles in the member roster — see
|
||||
Ranks & Roles above. Per-rank customization is planned.
|
||||
</span>
|
||||
</div>
|
||||
)}
|
||||
|
||||
{/* Desktop / Tablet Matrix Table */}
|
||||
<div className="overflow-x-auto rounded-xl border border-brand-border/60 bg-surface-container/60">
|
||||
<table className="w-full text-left border-collapse min-w-[720px]">
|
||||
|
|
@ -142,17 +187,22 @@ export function ClubPermissionMatrix({
|
|||
{capabilities.map((cap) => {
|
||||
const isGranted = rankPerms[cap.id] ?? false;
|
||||
const isImmutableOwner = rank.id === 'president';
|
||||
const toggleDisabled = !editable || isImmutableOwner;
|
||||
|
||||
return (
|
||||
<td key={cap.id} className="py-3.5 px-3 text-center">
|
||||
<button
|
||||
type="button"
|
||||
disabled={!canEdit || isImmutableOwner}
|
||||
onClick={() => onTogglePermission(rank.id, cap.id)}
|
||||
aria-label={`Toggle ${cap.label} for ${rank.title}`}
|
||||
className={`relative inline-flex h-5 w-9 shrink-0 cursor-pointer rounded-full border-2 border-transparent transition-colors duration-200 ease-in-out focus:outline-none disabled:opacity-75 disabled:cursor-not-allowed ${
|
||||
isGranted ? 'bg-brand-cyan' : 'bg-slate-700'
|
||||
}`}
|
||||
disabled={toggleDisabled}
|
||||
aria-readonly={!editable}
|
||||
onClick={() => {
|
||||
if (toggleDisabled) return;
|
||||
onTogglePermission(rank.id, cap.id);
|
||||
}}
|
||||
aria-label={`${editable ? 'Toggle' : 'Standard setting:'} ${cap.label} for ${rank.title}`}
|
||||
className={`relative inline-flex h-5 w-9 shrink-0 rounded-full border-2 border-transparent transition-colors duration-200 ease-in-out focus:outline-none disabled:opacity-75 disabled:cursor-not-allowed ${
|
||||
editable ? 'cursor-pointer' : 'cursor-default'
|
||||
} ${isGranted ? 'bg-brand-cyan' : 'bg-slate-700'}`}
|
||||
>
|
||||
<span
|
||||
className={`pointer-events-none inline-block h-4 w-4 transform rounded-full bg-black shadow ring-0 transition duration-200 ease-in-out ${
|
||||
|
|
|
|||
|
|
@ -0,0 +1,98 @@
|
|||
# Backend Contract Proposal — Club Rank Permission Matrix (WEB-W1)
|
||||
|
||||
Status: **Proposed / not implemented.** This documents the backend work required
|
||||
before the "Operational Permission Matrix" on
|
||||
`app/clubs/[clubId]/manage/ranks-roles/page.tsx` can be made editable. Until this
|
||||
ships, the matrix is rendered read-only (`persistenceAvailable={false}`).
|
||||
|
||||
## Problem
|
||||
|
||||
`handleSavePermissions` previously waited ~400 ms and showed a "Saved" state
|
||||
without writing anything (confirmed in source prior to this change). There is no
|
||||
Firestore document, API route, or security rule that stores or enforces a
|
||||
per-rank capability matrix. The enforced club authorization model is **role
|
||||
based**, derived solely from the administrative role on the member document:
|
||||
|
||||
- `lib/club-data.ts` → `getPortalPermissions(role)` maps `owner`/`admin` →
|
||||
`canManageSettings`, `canInviteMembers`, `canApproveApplications`,
|
||||
`canPlanRides`; `member` → all false.
|
||||
- Display rank (`President`, `Road Captain`, …) is cosmetic and explicitly does
|
||||
**not** grant software authority.
|
||||
|
||||
Persisting the matrix without enforcement would mislead club owners into
|
||||
believing a toggle restricts members when it does nothing. That is prohibited by
|
||||
the project rules (no unenforced permission models, no false success).
|
||||
|
||||
## Why this was not implemented in the website task
|
||||
|
||||
- Enforcement requires **Firestore security rule** changes (out of scope / frozen).
|
||||
- A new persistence model must not conflict with the existing Flutter club role
|
||||
model (`owner`/`admin`/`member`). Rank → capability is a different axis and
|
||||
needs a product decision on precedence.
|
||||
|
||||
## Proposed contract (for a future, authorized backend change)
|
||||
|
||||
### 1. Storage
|
||||
|
||||
Document: `clubs/{clubId}/settings/rankPermissions`
|
||||
|
||||
```jsonc
|
||||
{
|
||||
"version": 1,
|
||||
"updatedAt": "<serverTimestamp>",
|
||||
"updatedByUid": "<uid>",
|
||||
"ranks": {
|
||||
"road_captain": {
|
||||
"planRides": true,
|
||||
"manageApplications": false,
|
||||
"postAnnouncements": true,
|
||||
"moderateWall": true,
|
||||
"manageRoster": false,
|
||||
"editProfile": false
|
||||
}
|
||||
// ... one entry per rank id in DEFAULT_CLUB_RANKS except `president`
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Invariants:
|
||||
- `president` is immutable (always full authority); never written or editable.
|
||||
- Unknown rank ids and unknown capability keys are rejected.
|
||||
- Capability keys are constrained to `DEFAULT_CLUB_CAPABILITIES` ids.
|
||||
|
||||
### 2. Security rules (requires authorized change to the frozen rules)
|
||||
|
||||
- Read: any active club member.
|
||||
- Write: only `clubs/{clubId}.ownerUid == request.auth.uid`, or an active member
|
||||
whose `role == 'admin'`. Validate the written shape (version, allowed rank ids,
|
||||
boolean capability values only). Reject writes that would alter `president`.
|
||||
|
||||
### 3. Enforcement precedence (product decision required)
|
||||
|
||||
Rank capabilities may only **narrow**, never widen, what the administrative role
|
||||
already denies. Administrative `role` remains the hard security boundary enforced
|
||||
in rules; rank capabilities are a soft, owner-configurable delegation layer that
|
||||
the Flutter and web clients honor for UI affordances. The Flutter club model must
|
||||
adopt the same precedence so web and mobile agree.
|
||||
|
||||
### 4. Website wiring once the above exists
|
||||
|
||||
1. Add `fetchRankPermissions(clubId)` and `saveRankPermissions(clubId, callerUid, matrix)`
|
||||
to `lib/club-data.ts`, mirroring the authorization pattern of
|
||||
`updateClubAccessSettings` (owner/admin check + `updateDoc`).
|
||||
2. In `ranks-roles/page.tsx`: load persisted permissions in `loadData`, and
|
||||
implement `handleSavePermissions` to call `saveRankPermissions`, then re-read
|
||||
to confirm the write survived before showing success.
|
||||
3. Flip `<ClubPermissionMatrix persistenceAvailable={true} />` and pass
|
||||
`onSavePermissions` / `onResetDefaults` / `isSaving`.
|
||||
4. Add tests: persistence round-trip (save → reload → values survive), owner/admin
|
||||
authorization success, member/unauthenticated rejection, `president`
|
||||
immutability, and rejection of unknown rank/capability keys.
|
||||
|
||||
## Current truthful behavior (shipped in this branch)
|
||||
|
||||
- Matrix renders as a read-only reference of the standard governance model.
|
||||
- No Save/Reset controls; a "View-only reference" indicator and an availability
|
||||
notice explain that access is enforced by Owner/Admin roles and that per-rank
|
||||
customization is planned.
|
||||
- No save is attempted and no success is reported.
|
||||
|
|
@ -119,10 +119,8 @@ describe('WEB-13: ClubHierarchyBuilder & ClubPermissionMatrix', () => {
|
|||
expect(handleUpdateMemberRank).toHaveBeenCalledWith('rider-03', expect.any(String));
|
||||
});
|
||||
|
||||
it('renders permission matrix table with capabilities and allows toggling', () => {
|
||||
it('renders permission matrix capabilities for reference', () => {
|
||||
const handleToggle = vi.fn();
|
||||
const handleReset = vi.fn();
|
||||
const handleSave = vi.fn().mockResolvedValue(undefined);
|
||||
|
||||
render(
|
||||
<ClubPermissionMatrix
|
||||
|
|
@ -131,9 +129,6 @@ describe('WEB-13: ClubHierarchyBuilder & ClubPermissionMatrix', () => {
|
|||
permissions={DEFAULT_RANK_PERMISSIONS}
|
||||
selectedRankId="road_captain"
|
||||
onTogglePermission={handleToggle}
|
||||
onResetDefaults={handleReset}
|
||||
onSavePermissions={handleSave}
|
||||
isSaving={false}
|
||||
canEdit={true}
|
||||
/>
|
||||
);
|
||||
|
|
@ -143,15 +138,50 @@ describe('WEB-13: ClubHierarchyBuilder & ClubPermissionMatrix', () => {
|
|||
expect(screen.getByText('Review Applications')).toBeDefined();
|
||||
expect(screen.getByText('Post Announcements')).toBeDefined();
|
||||
expect(screen.getByText('Moderate Club Wall')).toBeDefined();
|
||||
|
||||
// Find toggle button for road captain and click it
|
||||
const toggleButton = screen.getByRole('button', { name: 'Toggle Plan Club Rides for Road Captain' });
|
||||
fireEvent.click(toggleButton);
|
||||
|
||||
expect(handleToggle).toHaveBeenCalledWith('road_captain', 'planRides');
|
||||
});
|
||||
|
||||
it('triggers save matrix and reset defaults', async () => {
|
||||
// WEB-W1 regression: without a persistence backend the matrix must be an honest
|
||||
// read-only reference. It must not offer a Save control, must not report a save,
|
||||
// and must not mutate permissions — even for an Owner/Admin (canEdit=true).
|
||||
it('is read-only and never reports a false save when persistence is unavailable', () => {
|
||||
const handleToggle = vi.fn();
|
||||
const handleSave = vi.fn().mockResolvedValue(undefined);
|
||||
|
||||
render(
|
||||
<ClubPermissionMatrix
|
||||
ranks={DEFAULT_CLUB_RANKS}
|
||||
capabilities={DEFAULT_CLUB_CAPABILITIES}
|
||||
permissions={DEFAULT_RANK_PERMISSIONS}
|
||||
selectedRankId="road_captain"
|
||||
onTogglePermission={handleToggle}
|
||||
onSavePermissions={handleSave}
|
||||
canEdit={true}
|
||||
// persistenceAvailable defaults to false
|
||||
/>
|
||||
);
|
||||
|
||||
// No save/reset affordances are rendered.
|
||||
expect(screen.queryByRole('button', { name: /Save Matrix/i })).toBeNull();
|
||||
expect(screen.queryByRole('button', { name: /Reset Defaults/i })).toBeNull();
|
||||
expect(screen.queryByText(/Saved/)).toBeNull();
|
||||
|
||||
// A truthful view-only indicator and availability notice are present.
|
||||
expect(screen.getByText(/View-only reference/i)).toBeDefined();
|
||||
expect(screen.getByText(/not saved yet/i)).toBeDefined();
|
||||
|
||||
// Toggles are disabled and do not mutate permissions.
|
||||
const toggleButton = screen.getByRole('button', {
|
||||
name: /Standard setting:\s*Plan Club Rides for Road Captain/i,
|
||||
});
|
||||
expect((toggleButton as HTMLButtonElement).disabled).toBe(true);
|
||||
fireEvent.click(toggleButton);
|
||||
expect(handleToggle).not.toHaveBeenCalled();
|
||||
expect(handleSave).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// Forward-compatible editable path: when a persistence backend is wired AND the
|
||||
// caller may edit, the matrix exposes real Save/Reset controls and toggles.
|
||||
it('allows editing and saving only when persistence is available and canEdit', async () => {
|
||||
const handleToggle = vi.fn();
|
||||
const handleReset = vi.fn();
|
||||
const handleSave = vi.fn().mockResolvedValue(undefined);
|
||||
|
|
@ -167,9 +197,14 @@ describe('WEB-13: ClubHierarchyBuilder & ClubPermissionMatrix', () => {
|
|||
onSavePermissions={handleSave}
|
||||
isSaving={false}
|
||||
canEdit={true}
|
||||
persistenceAvailable={true}
|
||||
/>
|
||||
);
|
||||
|
||||
const toggleButton = screen.getByRole('button', { name: 'Toggle Plan Club Rides for Road Captain' });
|
||||
fireEvent.click(toggleButton);
|
||||
expect(handleToggle).toHaveBeenCalledWith('road_captain', 'planRides');
|
||||
|
||||
const resetBtn = screen.getByRole('button', { name: /Reset Defaults/i });
|
||||
fireEvent.click(resetBtn);
|
||||
expect(handleReset).toHaveBeenCalled();
|
||||
|
|
|
|||
Loading…
Reference in New Issue