Skip to content

Commit b46bfca

Browse files
committed
fix(middlewares): improve ownership check middleware
- Add logging to track middleware execution flow - Handle null user scenario for ownership check - Throw exception for misconfigured public routes requiring ownership check - Improve error handling and messaging for configuration errors - Optimize ownership check logic for better readability and performance
1 parent afbde1a commit b46bfca

File tree

1 file changed

+36
-2
lines changed

1 file changed

+36
-2
lines changed

lib/src/middlewares/ownership_check_middleware.dart

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@ import 'package:core/core.dart';
22
import 'package:dart_frog/dart_frog.dart';
33
import 'package:flutter_news_app_api_server_full_source_code/src/rbac/permission_service.dart';
44
import 'package:flutter_news_app_api_server_full_source_code/src/registry/model_registry.dart';
5+
import 'package:logging/logging.dart';
6+
7+
final _log = Logger('OwnershipCheckMiddleware');
58

69
/// A wrapper class to provide a fetched item into the request context.
710
///
@@ -33,8 +36,9 @@ class FetchedItem<T> {
3336
Middleware ownershipCheckMiddleware() {
3437
return (handler) {
3538
return (context) async {
39+
_log.finer('Entering ownershipCheckMiddleware.');
3640
final modelConfig = context.read<ModelConfig<dynamic>>();
37-
final user = context.read<User>();
41+
final user = context.read<User?>();
3842
final permissionService = context.read<PermissionService>();
3943
final method = context.request.method;
4044

@@ -49,20 +53,45 @@ Middleware ownershipCheckMiddleware() {
4953
permission = modelConfig.deletePermission;
5054
default:
5155
// For any other methods, no ownership check is performed.
56+
_log.finer('Method "$method" does not require ownership check. Skipping.');
5257
return handler(context);
5358
}
5459

5560
// If no ownership check is required for this action, or if the user is
5661
// an admin (who bypasses ownership checks), proceed immediately.
5762
if (!permission.requiresOwnershipCheck ||
58-
permissionService.isAdmin(user)) {
63+
(user != null && permissionService.isAdmin(user))) {
64+
_log.finer(
65+
'Ownership check not required or user is admin. Skipping ownership check.',
66+
);
5967
return handler(context);
6068
}
6169

6270
// At this point, an ownership check is required for a non-admin user.
71+
_log.finer(
72+
'Ownership check required for model "${context.read<String>()}", '
73+
'method "$method". User is not admin.',
74+
);
75+
76+
// If an ownership check IS required, we must have an authenticated user.
77+
// If user is null here, it means a public route is configured to require
78+
// an ownership check, which is a misconfiguration.
79+
if (user == null) {
80+
_log.warning(
81+
'Ownership check required but no authenticated user found. '
82+
'This indicates a configuration error.',
83+
);
84+
throw const OperationFailedException(
85+
'Internal Server Error: Ownership check required for unauthenticated access.',
86+
);
87+
}
6388

6489
// Ensure the model is configured to support ownership checks.
6590
if (modelConfig.getOwnerId == null) {
91+
_log.severe(
92+
'Configuration Error: Model "${context.read<String>()}" requires '
93+
'ownership check but getOwnerId is null.',
94+
);
6695
throw const OperationFailedException(
6796
'Internal Server Error: Model configuration error for ownership check.',
6897
);
@@ -76,10 +105,15 @@ Middleware ownershipCheckMiddleware() {
76105
// Compare the item's owner ID with the authenticated user's ID.
77106
final itemOwnerId = modelConfig.getOwnerId!(item);
78107
if (itemOwnerId != user.id) {
108+
_log.warning(
109+
'Ownership check failed for user ${user.id} on item with owner ID '
110+
'$itemOwnerId. Denying access.',
111+
);
79112
throw const ForbiddenException(
80113
'You do not have permission to access this item.',
81114
);
82115
}
116+
_log.finer('Ownership check passed for user ${user.id}.');
83117

84118
// If the ownership check passes, proceed to the final route handler.
85119
return handler(context);

0 commit comments

Comments
 (0)