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. |
| <ng-container matColumnDef="select"> | ||
| <th mat-header-cell *matHeaderCellDef class="batch-select-column"> | ||
| <!-- Any selection makes the next click clear; indeterminate supplies the mixed appearance. --> | ||
| <mat-checkbox |
There was a problem hiding this comment.
these checkboxes are implemented exactly the same in the 3 places, and have a lot going on that would be better encapsulated as common-components, probably one for batch-action header, and one for batch-action row. additionally we'd have an easier time updating for other operations later, and/or dropping them into other tables for batch-actions there.
| const message = batchResultMessage('flags', counts, (key, params) => this.translate.instant(key, params)); | ||
| if (!counts.hasErrors) this.notificationService.showSuccess(message); | ||
| else if (counts.deleted || counts.absent) this.notificationService.showWarning(message); | ||
| else this.notificationService.showError(message); |
There was a problem hiding this comment.
this might be missing what is in the segments / experiments "finish-batch", does it not need this little bit also?
const pathname = (this.router.url || '').split('?')[0].split('#')[0];
return [
// Detail selectors share the rows array; replace it only while the root table is displayed.
...(pathname === '/segments'
? [SegmentsActions.actionFetchSegments({ fromStarting: true, batchRefresh: true })]
: []),
];.
These seem pretty much identical across all 3, maybe this is something that also deserves to be a common helper function kind of a thing.
| const distinct = new Map(rows.map((row) => [row.id, row])); | ||
| if (distinct.size !== rows.length) result = { ...result, [config.rowsKey]: [...distinct.values()] }; | ||
| } | ||
| // Tombstones also protect against late detail/stat responses, whose IDs are not tied to a root query. |
| } | ||
|
|
||
| /** Attach the shared selection lifecycle while preserving each entity's existing detail reducers. */ | ||
| export function withRootBatch<S extends { rootBatch: RootBatchState }>( |
There was a problem hiding this comment.
maybe this should be withRootBatchDelete, as it looks like it couldn't be reused for other batch operations
| } | ||
|
|
||
| /** Attach the shared selection lifecycle while preserving each entity's existing detail reducers. */ | ||
| export function withRootBatch<S extends { rootBatch: RootBatchState }>( |
There was a problem hiding this comment.
and this is a very chunky function, it's hard to read, can we please break this down into smaller functions? it's sort of acting like a reducer I guess? many of the ifs can have their inner logic pulled out as a smaller distinct function and make this easier to read both here and in the tests.
|
overall i have a few asks that are generally in the form of clean-up operations...
|
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.