Skip to content

Commit a921be9

Browse files
committed
refactor(data): simplify ownership check in delete handler
- Remove redundant ownership check logic from _handleDelete function - Add comment about middleware handling authorization and ownership checks - Simplify userIdForRepoCall assignment using ternary operator - Remove unnecessary fetched item storage before deletion
1 parent 13c1219 commit a921be9

File tree

1 file changed

+11
-88
lines changed

1 file changed

+11
-88
lines changed

routes/api/v1/data/[id]/index.dart

Lines changed: 11 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -426,94 +426,17 @@ Future<Response> _handleDelete(
426426
User authenticatedUser,
427427
PermissionService permissionService,
428428
) async {
429-
// Authorization check is handled by authorizationMiddleware before this.
430-
// This handler only needs to perform the ownership check if required.
431-
432-
// Determine userId for repository call based on ModelConfig (for data scoping/ownership enforcement)
433-
String? userIdForRepoCall;
434-
// If the model is user-owned, pass the authenticated user's ID to the repository
435-
// for ownership enforcement. Otherwise, pass null.
436-
if (modelConfig.getOwnerId != null &&
437-
!permissionService.isAdmin(authenticatedUser)) {
438-
userIdForRepoCall = authenticatedUser.id;
439-
} else {
440-
userIdForRepoCall = null;
441-
}
442-
443-
// --- Handler-Level Ownership Check (for DELETE) ---
444-
// For DELETE, we need to fetch the item *before* attempting deletion
445-
// to perform the ownership check if required.
446-
dynamic itemToDelete;
447-
if (modelConfig.deletePermission.requiresOwnershipCheck &&
448-
!permissionService.isAdmin(authenticatedUser)) {
449-
// Ensure getOwnerId is provided for models requiring ownership check
450-
if (modelConfig.getOwnerId == null) {
451-
_logger.severe(
452-
'Configuration Error: Model "$modelName" requires '
453-
'ownership check for DELETE but getOwnerId is not provided.',
454-
);
455-
// Throw an exception to be caught by the errorHandler
456-
throw const OperationFailedException(
457-
'Internal Server Error: Model configuration error.',
458-
);
459-
}
460-
// Fetch the item to check ownership. Use userIdForRepoCall for scoping.
461-
// Repository exceptions (like NotFoundException) will propagate up to the errorHandler.
462-
switch (modelName) {
463-
case 'headline':
464-
final repo = context.read<DataRepository<Headline>>();
465-
itemToDelete = await repo.read(id: id, userId: userIdForRepoCall);
466-
case 'topic':
467-
final repo = context.read<DataRepository<Topic>>();
468-
itemToDelete = await repo.read(id: id, userId: userIdForRepoCall);
469-
case 'source':
470-
final repo = context.read<DataRepository<Source>>();
471-
itemToDelete = await repo.read(id: id, userId: userIdForRepoCall);
472-
case 'country':
473-
final repo = context.read<DataRepository<Country>>();
474-
itemToDelete = await repo.read(id: id, userId: userIdForRepoCall);
475-
case 'language':
476-
final repo = context.read<DataRepository<Language>>();
477-
itemToDelete = await repo.read(id: id, userId: userIdForRepoCall);
478-
case 'user':
479-
final repo = context.read<DataRepository<User>>();
480-
itemToDelete = await repo.read(id: id, userId: userIdForRepoCall);
481-
case 'user_app_settings': // New case for UserAppSettings
482-
final repo = context.read<DataRepository<UserAppSettings>>();
483-
itemToDelete = await repo.read(id: id, userId: userIdForRepoCall);
484-
case 'user_content_preferences': // New case for UserContentPreferences
485-
final repo = context.read<DataRepository<UserContentPreferences>>();
486-
itemToDelete = await repo.read(id: id, userId: userIdForRepoCall);
487-
case 'remote_config': // New case for RemoteConfig (delete by admin)
488-
final repo = context.read<DataRepository<RemoteConfig>>();
489-
itemToDelete = await repo.read(
490-
id: id,
491-
userId: userIdForRepoCall,
492-
); // userId should be null for AppConfig
493-
default:
494-
_logger.severe(
495-
'Unsupported model type "$modelName" reached _handleDelete ownership check.',
496-
);
497-
// Throw an exception to be caught by the errorHandler
498-
throw OperationFailedException(
499-
'Unsupported model type "$modelName" reached handler.',
500-
);
501-
}
502-
503-
// Perform the ownership check if the item was found
504-
if (itemToDelete != null) {
505-
final itemOwnerId = modelConfig.getOwnerId!(itemToDelete);
506-
if (itemOwnerId != authenticatedUser.id) {
507-
// If the authenticated user is not the owner, deny access.
508-
// Throw ForbiddenException to be caught by the errorHandler
509-
throw const ForbiddenException(
510-
'You do not have permission to delete this specific item.',
511-
);
512-
}
513-
}
514-
// If itemToDelete is null here, it means the item wasn't found during the read.
515-
// The subsequent delete call will likely throw NotFoundException, which is correct.
516-
}
429+
// Authorization and ownership checks are handled by the middleware.
430+
// The `ownershipCheckMiddleware` has already verified that the user is
431+
// the owner if required. This handler's only job is to perform the deletion.
432+
433+
// Determine the userId for the repository call. For non-admins, this
434+
// provides an additional layer of security at the database level.
435+
final userIdForRepoCall =
436+
(modelConfig.getOwnerId != null &&
437+
!permissionService.isAdmin(authenticatedUser))
438+
? authenticatedUser.id
439+
: null;
517440

518441
// Allow repository exceptions (e.g., NotFoundException) to propagate
519442
// upwards to be handled by the standard error handling mechanism.

0 commit comments

Comments
 (0)