Merge pull request #57 from flutter-news-app-full-source-code/Implement-Configurable-Authentication-for-/api/v1/data-Generic-Route

Implement configurable authentication for `/api/v1/data` generic route

Author: fulleni

lib/src/middlewares/authorization_middleware.dart

@@ -9,14 +9,16 @@ final _log = Logger('AuthorizationMiddleware');
 /// {@template authorization_middleware}
 /// Middleware to enforce role-based permissions and model-specific access rules.
 ///
-/// This middleware reads the authenticated [User], the requested `modelName`,
-/// the `HttpMethod`, and the `ModelConfig` from the request context. It then
-/// determines the required permission based on the `ModelConfig` metadata for
-/// the specific HTTP method and checks if the authenticated user has that
-/// permission using the [PermissionService].
+/// This middleware reads the authenticated [User] (which may be null for
+/// public routes), the requested `modelName`, the `HttpMethod`, and the
+/// `ModelConfig` from the request context. It then determines the required
+/// permission based on the `ModelConfig` metadata for the specific HTTP method
+/// and checks if the authenticated user has that permission using the
+/// [PermissionService].
 ///
-/// If the user does not have the required permission, it throws a
-/// [ForbiddenException], which should be caught by the 'errorHandler' middleware.
+/// If the user does not have the required permission, or if authentication is
+/// required but no user is present, it throws a [ForbiddenException] or
+/// [UnauthorizedException], which should be caught by the 'errorHandler' middleware.
 ///
 /// This middleware runs *after* authentication and model validation.
 /// It does NOT perform instance-level ownership checks; those are handled
@@ -27,8 +29,9 @@ Middleware authorizationMiddleware() {
   return (handler) {
     return (context) async {
       // Read dependencies from the context.
-      // User is guaranteed non-null by requireAuthentication() middleware.
-      final user = context.read<User>();
+      // User is now read as nullable, as _conditionalAuthenticationMiddleware
+      // might provide null for public routes.
+      final user = context.read<User?>();
       final permissionService = context.read<PermissionService>();
       final modelName = context.read<String>(); // Provided by data/_middleware
       final modelConfig = context
@@ -64,23 +67,33 @@ Middleware authorizationMiddleware() {
           );
       }
 
-      // Perform the permission check based on the configuration type
+      // Handle authentication requirement based on ModelConfig ---
+      if (requiredPermissionConfig.requiresAuthentication && user == null) {
+        _log.warning(
+          'Authorization required but no valid user found. Denying access.',
+        );
+        throw const UnauthorizedException('Authentication required.');
+      }
+
+      // If authentication is not required, or if user is present, proceed with permission checks.
+      // All subsequent checks must now handle a potentially null 'user' if 'requiresAuthentication' is false.
       switch (requiredPermissionConfig.type) {
         case RequiredPermissionType.none:
-          // No specific permission required (beyond authentication if applicable)
-          // This case is primarily for documentation/completeness if a route
-          // group didn't require authentication, but the /data route does.
-          // For the /data route, 'none' effectively means 'authenticated users allowed'.
+          // No specific permission required.
+          // If user is null, it's a public route. If user is not null, they are authenticated.
           break;
         case RequiredPermissionType.adminOnly:
-          // Requires the user to be an admin
-          if (!permissionService.isAdmin(user)) {
+          // Requires the user to be an admin.
+          // If user is null here, it means requiresAuthentication was false,
+          // but adminOnly implies authentication. This is a misconfiguration
+          // or an attempt to access an admin-only resource publicly.
+          if (user == null || !permissionService.isAdmin(user)) {
             throw const ForbiddenException(
               'Only administrators can perform this action.',
             );
           }
         case RequiredPermissionType.specificPermission:
-          // Requires a specific permission string
+          // Requires a specific permission string.
           final permission = requiredPermissionConfig.permission;
           if (permission == null) {
             // This indicates a configuration error in ModelRegistry
@@ -92,19 +105,21 @@ Middleware authorizationMiddleware() {
               'Internal Server Error: Authorization configuration error.',
             );
           }
-          if (!permissionService.hasPermission(user, permission)) {
+          // If user is null here, it means requiresAuthentication was false,
+          // but specificPermission implies authentication. This is a misconfiguration
+          // or an attempt to access a protected resource publicly.
+          if (user == null ||
+              !permissionService.hasPermission(user, permission)) {
             throw const ForbiddenException(
               'You do not have permission to perform this action.',
             );
           }
         case RequiredPermissionType.unsupported:
           // This action is explicitly marked as not supported via this generic route.
-          // Return Method Not Allowed.
           _log.warning(
             'Action for model "$modelName", method "$method" is marked as '
             'unsupported via generic route.',
           );
-          // Throw ForbiddenException to be caught by the errorHandler
           throw ForbiddenException(
             'Method "$method" is not supported for model "$modelName" '
             'via this generic data endpoint.',

lib/src/middlewares/ownership_check_middleware.dart

@@ -2,6 +2,9 @@ import 'package:core/core.dart';
 import 'package:dart_frog/dart_frog.dart';
 import 'package:flutter_news_app_api_server_full_source_code/src/rbac/permission_service.dart';
 import 'package:flutter_news_app_api_server_full_source_code/src/registry/model_registry.dart';
+import 'package:logging/logging.dart';
+
+final _log = Logger('OwnershipCheckMiddleware');
 
 /// A wrapper class to provide a fetched item into the request context.
 ///
@@ -33,8 +36,9 @@ class FetchedItem<T> {
 Middleware ownershipCheckMiddleware() {
   return (handler) {
     return (context) async {
+      _log.finer('Entering ownershipCheckMiddleware.');
       final modelConfig = context.read<ModelConfig<dynamic>>();
-      final user = context.read<User>();
+      final user = context.read<User?>();
       final permissionService = context.read<PermissionService>();
       final method = context.request.method;
 
@@ -49,20 +53,47 @@ Middleware ownershipCheckMiddleware() {
           permission = modelConfig.deletePermission;
         default:
           // For any other methods, no ownership check is performed.
+          _log.finer(
+            'Method "$method" does not require ownership check. Skipping.',
+          );
           return handler(context);
       }
 
       // If no ownership check is required for this action, or if the user is
       // an admin (who bypasses ownership checks), proceed immediately.
       if (!permission.requiresOwnershipCheck ||
-          permissionService.isAdmin(user)) {
+          (user != null && permissionService.isAdmin(user))) {
+        _log.finer(
+          'Ownership check not required or user is admin. Skipping ownership check.',
+        );
         return handler(context);
       }
 
       // At this point, an ownership check is required for a non-admin user.
+      _log.finer(
+        'Ownership check required for model "${context.read<String>()}", '
+        'method "$method". User is not admin.',
+      );
+
+      // If an ownership check IS required, we must have an authenticated user.
+      // If user is null here, it means a public route is configured to require
+      // an ownership check, which is a misconfiguration.
+      if (user == null) {
+        _log.warning(
+          'Ownership check required but no authenticated user found. '
+          'This indicates a configuration error.',
+        );
+        throw const OperationFailedException(
+          'Internal Server Error: Ownership check required for unauthenticated access.',
+        );
+      }
 
       // Ensure the model is configured to support ownership checks.
       if (modelConfig.getOwnerId == null) {
+        _log.severe(
+          'Configuration Error: Model "${context.read<String>()}" requires '
+          'ownership check but getOwnerId is null.',
+        );
         throw const OperationFailedException(
           'Internal Server Error: Model configuration error for ownership check.',
         );
@@ -76,10 +107,15 @@ Middleware ownershipCheckMiddleware() {
       // Compare the item's owner ID with the authenticated user's ID.
       final itemOwnerId = modelConfig.getOwnerId!(item);
       if (itemOwnerId != user.id) {
+        _log.warning(
+          'Ownership check failed for user ${user.id} on item with owner ID '
+          '$itemOwnerId. Denying access.',
+        );
         throw const ForbiddenException(
           'You do not have permission to access this item.',
         );
       }
+      _log.finer('Ownership check passed for user ${user.id}.');
 
       // If the ownership check passes, proceed to the final route handler.
       return handler(context);