Fix campaign visibility bug for unassigned users after agency reassignment
Unassigned (no agency) non-admin users previously saw ALL campaigns due to a truthiness check that treated None agency_id as "no filter". This was a security bug — they should see NO campaigns and be blocked from creating them. Backend: Add _NO_AGENCY sentinel to distinguish "no filter" from "no agency", add early-returns at all 5 list/analytics endpoints, fix _check_campaign_access to explicitly reject unassigned users, and block campaign creation with 403. Frontend: Add isUnassigned boolean to UserContext, show informational empty state on Campaigns view, and reinforce readOnly for defense-in-depth. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
a08d54ec6d
commit
ebfcd60c71
3 changed files with 46 additions and 7 deletions
|
|
@ -49,27 +49,36 @@ from app.services.pdf_service import pdf_service
|
|||
|
||||
router = APIRouter()
|
||||
|
||||
# Sentinel: distinguishes "no filter (admins see all)" from "user has no agency"
|
||||
_NO_AGENCY = object()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Helpers
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def _resolve_agency_filter(current_user: User, agency_id_param: Optional[uuid.UUID] = None) -> Optional[uuid.UUID]:
|
||||
def _resolve_agency_filter(current_user: User, agency_id_param: Optional[uuid.UUID] = None):
|
||||
"""Determine which agency_id to filter by based on user role and query param.
|
||||
|
||||
- super_admin / oversight_admin: use agency_id_param if provided, else None (all)
|
||||
- agency_admin / basic_user: forced to current_user.agency_id
|
||||
Returns:
|
||||
uuid.UUID – filter to a specific agency
|
||||
None – no filter (admins see all)
|
||||
_NO_AGENCY – user is unassigned; callers should return empty results
|
||||
"""
|
||||
if current_user.role in ("super_admin", "oversight_admin"):
|
||||
return agency_id_param # None means "all"
|
||||
return current_user.agency_id # may be None (user not assigned yet)
|
||||
if current_user.agency_id is None:
|
||||
return _NO_AGENCY
|
||||
return current_user.agency_id
|
||||
|
||||
|
||||
def _check_campaign_access(current_user: User, campaign) -> None:
|
||||
"""Raise 404 if the user's role restricts them and the campaign doesn't belong to their agency."""
|
||||
if current_user.role in ("super_admin", "oversight_admin"):
|
||||
return
|
||||
if current_user.agency_id and campaign.agency_id == current_user.agency_id:
|
||||
if current_user.agency_id is None:
|
||||
raise HTTPException(status_code=404, detail="Campaign not found")
|
||||
if campaign.agency_id == current_user.agency_id:
|
||||
return
|
||||
raise HTTPException(status_code=404, detail="Campaign not found")
|
||||
|
||||
|
|
@ -107,6 +116,8 @@ async def list_campaigns(
|
|||
):
|
||||
"""List campaigns, filtered by the user's role and optional agency filter."""
|
||||
effective_agency_id = _resolve_agency_filter(current_user, agency_id)
|
||||
if effective_agency_id is _NO_AGENCY:
|
||||
return []
|
||||
repo = CampaignRepository(db)
|
||||
campaigns_with_counts = await repo.get_with_proof_counts(agency_id=effective_agency_id)
|
||||
|
||||
|
|
@ -135,6 +146,8 @@ async def create_campaign(
|
|||
current_user: User = Depends(require_write_access),
|
||||
):
|
||||
"""Create a new campaign. Blocked for oversight_admin."""
|
||||
if current_user.agency_id is None and current_user.role not in ("super_admin", "oversight_admin"):
|
||||
raise HTTPException(status_code=403, detail="You must be assigned to an agency before creating campaigns.")
|
||||
repo = CampaignRepository(db)
|
||||
|
||||
# Check if campaign name already exists
|
||||
|
|
@ -464,6 +477,8 @@ async def list_flagged_items(
|
|||
):
|
||||
"""List flagged items, filtered by role."""
|
||||
effective_agency_id = _resolve_agency_filter(current_user, agency_id)
|
||||
if effective_agency_id is _NO_AGENCY:
|
||||
return []
|
||||
audit_repo = AuditRepository(db)
|
||||
flagged_items = await audit_repo.get_flagged_items(agency_id=effective_agency_id, limit=limit, offset=offset)
|
||||
|
||||
|
|
@ -494,6 +509,8 @@ async def list_resolved_items(
|
|||
):
|
||||
"""List resolved items, filtered by role."""
|
||||
effective_agency_id = _resolve_agency_filter(current_user, agency_id)
|
||||
if effective_agency_id is _NO_AGENCY:
|
||||
return []
|
||||
audit_repo = AuditRepository(db)
|
||||
resolved_items = await audit_repo.get_resolved_items(agency_id=effective_agency_id, limit=limit, offset=offset)
|
||||
|
||||
|
|
@ -525,6 +542,8 @@ async def list_error_items(
|
|||
):
|
||||
"""List error items, filtered by role."""
|
||||
effective_agency_id = _resolve_agency_filter(current_user, agency_id)
|
||||
if effective_agency_id is _NO_AGENCY:
|
||||
return []
|
||||
audit_repo = AuditRepository(db)
|
||||
error_items = await audit_repo.get_error_items(agency_id=effective_agency_id, limit=limit, offset=offset)
|
||||
|
||||
|
|
@ -556,6 +575,8 @@ async def get_analytics(
|
|||
):
|
||||
"""Get analytics data, filtered by role."""
|
||||
effective_agency_id = _resolve_agency_filter(current_user, agency_id)
|
||||
if effective_agency_id is _NO_AGENCY:
|
||||
return AnalyticsResponse(total_reviews=0, passed=0, failed=0, errors=0, legal_review=0)
|
||||
repo = CampaignRepository(db)
|
||||
analytics = await repo.get_analytics(agency_id=effective_agency_id)
|
||||
return AnalyticsResponse(**analytics)
|
||||
|
|
|
|||
|
|
@ -74,7 +74,7 @@ const App: React.FC = () => {
|
|||
|
||||
const AppContent: React.FC<{ msalInstance: any }> = ({ msalInstance }) => {
|
||||
const isAuthenticated = true; // We're inside the authenticated boundary
|
||||
const { user, isLoading: isUserLoading, canWrite, canSeeAnalytics, canSeeAuditing, canSeeKnowledgeBase, canSeeSettings, canSeeUserManagement, canEditSettings, isSuperAdmin, isOversightAdmin } = useUser();
|
||||
const { user, isLoading: isUserLoading, canWrite, canSeeAnalytics, canSeeAuditing, canSeeKnowledgeBase, canSeeSettings, canSeeUserManagement, canEditSettings, isSuperAdmin, isOversightAdmin, isUnassigned } = useUser();
|
||||
|
||||
// Get initial state from URL
|
||||
const initialUrlState = parseUrlState();
|
||||
|
|
@ -840,6 +840,19 @@ const AppContent: React.FC<{ msalInstance: any }> = ({ msalInstance }) => {
|
|||
case 'CopyGenAI':
|
||||
return <CopyGenAI />;
|
||||
case 'Campaigns':
|
||||
if (isUnassigned) {
|
||||
return (
|
||||
<div className="flex-1 flex items-center justify-center p-8">
|
||||
<div className="text-center max-w-md">
|
||||
<div className="text-5xl mb-4">🏢</div>
|
||||
<h2 className="text-xl font-semibold text-gray-700 mb-2">No Agency Assigned</h2>
|
||||
<p className="text-gray-500">
|
||||
You are not currently assigned to an agency. Please contact your administrator to be assigned before you can view or create campaigns.
|
||||
</p>
|
||||
</div>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
return <Campaigns
|
||||
selectedCampaign={selectedCampaign}
|
||||
selectedProof={selectedProof}
|
||||
|
|
@ -860,7 +873,7 @@ const AppContent: React.FC<{ msalInstance: any }> = ({ msalInstance }) => {
|
|||
onResolveSubmit={handleResolveSubmit}
|
||||
flaggedItems={flaggedItems}
|
||||
resolvedItems={resolvedItems}
|
||||
readOnly={readOnly}
|
||||
readOnly={readOnly || isUnassigned}
|
||||
/>;
|
||||
case 'WIP Reviewer':
|
||||
return <WIPReviewer dropdownOptions={dropdownOptions} msalInstance={msalInstance} />;
|
||||
|
|
|
|||
|
|
@ -15,6 +15,8 @@ interface UserContextValue {
|
|||
canSeeSettings: boolean;
|
||||
canSeeUserManagement: boolean;
|
||||
canEditSettings: boolean;
|
||||
/** True when the user exists but has no agency and is not an admin */
|
||||
isUnassigned: boolean;
|
||||
/** Re-fetch user from backend (e.g. after role change) */
|
||||
refresh: () => Promise<void>;
|
||||
}
|
||||
|
|
@ -31,6 +33,7 @@ const UserContext = createContext<UserContextValue>({
|
|||
canSeeSettings: false,
|
||||
canSeeUserManagement: false,
|
||||
canEditSettings: false,
|
||||
isUnassigned: false,
|
||||
refresh: async () => {},
|
||||
});
|
||||
|
||||
|
|
@ -77,6 +80,8 @@ export const UserProvider: React.FC<{ children: React.ReactNode }> = ({ children
|
|||
canSeeSettings: role === 'super_admin' || role === 'oversight_admin' || role === 'agency_admin',
|
||||
canSeeUserManagement: role === 'super_admin',
|
||||
canEditSettings: role === 'super_admin',
|
||||
isUnassigned: user != null && user.agencyId == null
|
||||
&& role !== 'super_admin' && role !== 'oversight_admin',
|
||||
refresh: fetchUser,
|
||||
};
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue