Conversation
… ineligible items
…able after failures
Nakagawa-master
left a comment
There was a problem hiding this comment.
There is a server-side authorization mismatch here that looks more fundamental than the stale-selection UI cases already discussed.
The current frontend permission matrix treats delete as an authority boundary:
- ADMIN / CREATOR: delete experiments, flags, segments
- USER_MANAGER: no experiment/flag delete, segment delete allowed
- READER: no delete for any of the three
But the current backend path does not enforce that matrix. The class-level @Authorized() only authenticates: authorizationChecker ignores the roles argument and returns true after attaching any validated user. BatchDeleteService.delete/deleteOne then calls the delete services without a role/permission check.
The integration suite makes the behavior explicit:
test.each(Object.values(UserRole))(
'matches single-delete behavior for the %s role',
async (role) => {
...
await request(app).delete(...).expect(200)
await request(app).post(batchRoute).expect(200)
...
expect(rows).toBeDeleted()
}
)and later specifically verifies a UserRole.READER can use the single-delete route successfully.
That contradicts the thread statement that “the batch API still checks the user's current database role on each request and rejects requests without delete permission with 403.” A stale UI is therefore not just able to show Delete after a downgrade; a Reader/User Manager can call the destructive endpoint directly and the server accepts it.
Because this PR introduces a bulk destructive surface, I would make the server—not the UI—the authority for this rule. Ideally use one shared backend policy for both single and batch deletes so the two routes cannot drift:
experiments: ADMIN | CREATOR
flags: ADMIN | CREATOR
segments: ADMIN | CREATOR | USER_MANAGER
READER: none
(or whatever role matrix is actually intended).
The discriminating integration coverage would be:
- authenticate as each role using the persisted/current DB role;
- attempt both single and batch deletion for each entity;
- allowed combinations delete and audit normally;
- disallowed combinations return 403 and leave the target, owned lists/members, caches/external-sync side effects, and deletion audit unchanged;
- downgrade the same account after the UI has loaded and prove the next request is rejected without relying on a client refresh.
If the product intentionally allows every authenticated role to delete through the API, then the frontend UserPermission.*.delete values and the “server rejects with 403” assumption should be renamed/documented as presentation-only, because today they read as authorization that the server does not enforce.
|
@Nakagawa-master No deletion route has ever enforced a role, so this isn't something the PR introduces. An earlier revision of this branch did guard the batch endpoints, but I took it out so batch and single delete behave the same way instead of having two different rules for the same targets. The statement you quoted about the batch API returning 403 was accurate while that guard existed, and it's out of date now. I've opened #3326 to enforce the role matrix and the state rules on both routes. |
Waiting for #3304 to merge. Once it lands, this branch will be updated with the latest
dev, merge conflicts resolved, and obsolete Mooclet dependencies removed from batch deletion and its tests. The combined changes will be verified before this PR is marked ready for review.Resolves #3313 (For reference, I updated the selection checkboxes to remain visible without hover as @amurphy-cl sugggested)
Changes
Where the changes live
core/batch-actions, the root-page directive, ~720 lines): selection state, reducer, effects, and facade. Each entity store attaches it through one wrapper so the three tables cannot drift apart.BatchDeleteServicewith per-item transactions, and an optional transaction parameter threaded through the existing delete services so single and batch deletion share one path.FeatureFlagService.findOneForDetailsmoved toFeatureFlagRepositoryunchanged so the flag delete can read inside its transaction.