Skip to content

feat: Implement custom authorization filter and enhance JWT authentication responses#52

Merged
taiphanvan2k3 merged 2 commits intomainfrom
refactor/change-all-code-from-throwing-exception-to-return-error
Nov 22, 2025
Merged

feat: Implement custom authorization filter and enhance JWT authentication responses#52
taiphanvan2k3 merged 2 commits intomainfrom
refactor/change-all-code-from-throwing-exception-to-return-error

Conversation

@taiphanvan2k3
Copy link
Copy Markdown
Member

@taiphanvan2k3 taiphanvan2k3 commented Nov 22, 2025

Summary by CodeRabbit

Release Notes

Refactor

  • Removed authorization pipeline checks; authorization enforcement now handled separately from request processing
  • Migrated error handling from exceptions to structured error results throughout command handlers (Login, RefreshAccessToken, etc.)
  • Simplified request pipeline by removing intermediate processing layers
  • Standardized API error responses with consistent error codes and HTTP status mapping
  • Removed GetProfile endpoint; user information retrieval requires alternative approach

Chores

  • Removed unused JWT configuration model
  • Eliminated obsolete exception types from error handling

✏️ Tip: You can customize this high-level summary in your review settings.

…ndling

- Deleted AuthorizationBehavior and associated interfaces to simplify authorization logic.
- Removed RequestProcessingException and related exception classes to enhance clarity in error handling.
- Updated various behaviors to utilize the new Result extensions for failure checks.
- Refactored controllers to handle results more succinctly, improving response management.
- Cleaned up documentation and unnecessary files to maintain project organization.
…ation responses

- Added CustomAuthorizeFilter to provide consistent API responses for authorization failures.
- Updated GlobalExceptionMiddleware to utilize shared JSON serializer options for error responses.
- Enhanced JWT Bearer authentication to return custom JSON responses for authentication and authorization failures.
- Registered CustomAuthorizeFilter globally in the controller options for uniform behavior across the API.
@taiphanvan2k3 taiphanvan2k3 self-assigned this Nov 22, 2025
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Nov 22, 2025

Walkthrough

This PR removes the authorization pipeline behavior and transitions the application from exception-based to result-based error handling. Changes include deleting AuthorizationBehavior, ICurrentUser interface, and CurrentUserService; removing custom exception types (BusinessRuleException, ConflictException, RequestProcessingException, SecurityException); adding new error types (Unauthorized, Forbidden, Security) to the domain model; updating authentication command handlers to return Result.Failure instead of throwing exceptions; and centralizing result handling in the base controller via HandleResult methods.

Changes

Cohort / File(s) Change Summary
Documentation
docs/ARCHITECTURE_OVERVIEW.md, docs/BEHAVIORS_GUIDE.md, docs/EXCEPTION_GUIDELINES.md, file_tree.md
Removed AuthorizationBehavior from MediatR pipeline documentation and sequence diagrams. Deleted RequestProcessingException references from exception hierarchy docs. Removed generated file tree.
Authorization Pipeline Removal
src/Application/Common/Behaviors/AuthorizationBehavior.cs, src/Web.Api/Services/CurrentUserService.cs
Deleted entire authorization pipeline integration: removed AuthorizationBehavior class, IAuthorizedRequest and ICurrentUser interfaces, and CurrentUserService implementation (including role-to-permission mapping).
Exception Type Removal
src/Application/Common/Exceptions/BusinessRuleException.cs, src/Application/Common/Exceptions/ConflictException.cs, src/Application/Common/Exceptions/RequestProcessingException.cs, src/Application/Common/Exceptions/SecurityException.cs
Removed four custom exception types used for domain-specific error signaling.
Behavior Pipeline Updates
src/Application/Common/Behaviors/CachingBehavior.cs, src/Application/Common/Behaviors/DomainEventBehavior.cs, src/Application/Common/Behaviors/LoggingBehavior.cs, src/Application/Common/Behaviors/PerformanceBehavior.cs, src/Application/Common/Behaviors/TransactionBehavior.cs
Updated behaviors to work with Result types: added failure checks to guard caching/domain events, refactored logging to handle failure results as warnings, added performance check guards for successful responses only.
Error Domain Extensions
src/Domain/Common/Error.cs, src/Domain/Common/ErrorType.cs, src/Application/Common/ResultExtensions.cs
Added three new ErrorType enum members (Unauthorized, Forbidden, Security) and corresponding static Error factory methods. Added IsFailure extension method to check Result.IsSuccess.
Result Type Updates
src/Domain/Common/Result.cs
Added implicit operator Result(TValue? value) for null-safe result conversion. Removed ValidationFailure factory method.
Feature Removal
src/Application/Features/Auth/GetProfile/GetProfileQuery.cs, src/Application/Features/Auth/GetProfile/GetProfileQueryHandler.cs
Deleted GetProfile query feature and handler implementation.
Query Modifications
src/Application/Features/Conversation/GetMessages/GetMessagesQuery.cs, src/Application/Features/User/GetUsers/GetUsersQuery.cs
Removed ICacheableQuery from GetMessagesQuery, removing caching. Removed IAuthorizedRequest from GetUsersQuery, removing per-query authorization.
Auth Command Handler Updates
src/Application/Features/Auth/Login/LoginCommandHandler.cs, src/Application/Features/Auth/RefreshAccessToken/RefreshAccessTokenCommandHandler.cs
Replaced exception-throwing with Result.Failure returns for error conditions: user not found, deactivated user, invalid password, invalid refresh token, revoked token, expired token.
Configuration Updates
src/Web.Api/Configurations/Options/JwtOptions.cs, src/Web.Api/Extensions/ApplicationServiceExtensions.cs, src/Web.Api/Extensions/AuthenticationExtensions.cs
Deleted JwtOptions class. Removed CurrentUserService and AuthorizationBehavior DI registration. Added JWT Bearer event handling for authentication failures and forbidden responses via customized OnChallenge and OnForbidden callbacks.
Base Controller Refactoring
src/Web.Api/Controllers/V1/BaseController.cs
Replaced Ok and Error methods with HandleResult and HandleResult overloads. Added centralized error-type-to-HTTP-status mapping. Introduced HandleFailure for structured error responses. Added protected ISender property backed by MediatR dependency.
Controller Updates
src/Web.Api/Controllers/V1/AuthController.cs, src/Web.Api/Controllers/V1/ConversationController.cs, src/Web.Api/Controllers/V1/UsersController.cs
Centralized result handling via HandleResult calls across actions. Added [Authorize(Roles = "Admin,Manager")] attribute to UsersController for declarative authorization.
Authorization Filtering
src/Web.Api/Filters/AuthorizationLoggingFilter.cs, src/Web.Api/Filters/CustomAuthorizeFilter.cs
Added empty AuthorizationLoggingFilter placeholder. Added CustomAuthorizeFilter implementation: distinguishes ChallengeResult (unauthenticated, 401) from ForbidResult (unauthorized, 403), logs context, and returns standardized ApiResponse errors.
Middleware & Exception Handling
src/Web.Api/Middleware/GlobalExceptionMiddleware.cs
Removed exception unwrapping and custom exception mappings for SecurityException, ConflictException, BusinessRuleException. Simplified to direct exception handling and JsonOptions.Default serialization.
Shared Utilities
src/Web.Api/Shared/JsonOptions.cs
Added shared JsonSerializerOptions configuration with CamelCase property naming policy for consistent API response serialization.
Program Configuration
src/Web.Api/Program.cs
Updated AddControllers to register CustomAuthorizeFilter as a global filter for centralized authorization response handling.

Sequence Diagram(s)

sequenceDiagram
    actor Client
    participant Controller as AuthController
    participant Handler as LoginCommandHandler
    participant Repository as UserRepository
    participant Domain as Domain Logic
    participant Response as HTTP Response

    Client->>Controller: POST /login
    Controller->>Handler: Handle(LoginCommand)
    
    Handler->>Repository: FindUserByEmail(email)
    alt User Not Found
        Repository-->>Handler: null
        Handler-->>Controller: Result.Failure(Unauthorized)
    else User Deactivated
        Repository-->>Handler: User { IsDeleted = true }
        Handler-->>Controller: Result.Failure(Forbidden)
    else Valid User
        Repository-->>Handler: User { IsDeleted = false }
        Handler->>Domain: ValidatePassword(password)
        alt Password Invalid
            Domain-->>Handler: false
            Handler-->>Controller: Result.Failure(Unauthorized)
        else Password Valid
            Domain-->>Handler: true
            Handler->>Domain: GenerateTokens()
            Domain-->>Handler: Tokens
            Handler-->>Controller: Result.Success(LoginResponse)
        end
    end
    
    Controller->>Controller: HandleResult(result)
    alt Success
        Controller-->>Response: 200 OK + ApiResponse<LoginResponse>
    else Failure
        Controller-->>Response: 401/403 + ApiResponse Error
    end
    Response-->>Client: HTTP Response
Loading
sequenceDiagram
    participant Client as API Client
    participant Filter as CustomAuthorizeFilter
    participant AuthHandler as Authorization Middleware
    participant Controller as Endpoint Controller
    
    Client->>Filter: HTTP Request
    Filter->>AuthHandler: Check [Authorize]
    alt Authentication Required (Missing/Invalid Token)
        AuthHandler-->>Filter: ChallengeResult
        Filter->>Filter: Log unauthenticated access
        Filter-->>Client: 401 Unauthorized + ApiResponse { ErrorInfo: AuthenticationRequired }
    else Insufficient Permissions
        AuthHandler-->>Filter: ForbidResult
        Filter->>Filter: Log insufficient permissions
        Filter-->>Client: 403 Forbidden + ApiResponse { ErrorInfo: InsufficientPermissions }
    else Authorized
        AuthHandler-->>Filter: Pass
        Filter->>Controller: Proceed to endpoint
        Controller-->>Client: Endpoint response
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

  • BaseController.cs: Substantial restructuring of response handling logic with new error-to-HTTP-status mapping switch; centralized result handling affects all controller responses.
  • GlobalExceptionMiddleware.cs: Exception mapping changes and removal of custom exception handling paths require verification that all error scenarios are properly covered.
  • AuthenticationExtensions.cs: JWT Bearer event customization introduces new response serialization paths and error structures that need validation.
  • LoginCommandHandler.cs & RefreshAccessTokenCommandHandler.cs: Exception-to-Result conversion across multiple error paths; each path transition requires careful verification to ensure semantics are preserved.
  • Behavior classes (CachingBehavior, DomainEventBehavior, LoggingBehavior, PerformanceBehavior): Multiple Result-based guards and control-flow changes across the pipeline; cumulative impact on request processing.
  • CustomAuthorizeFilter.cs: New global authorization filter introduces new control flow and response handling; requires validation against various authorization scenarios.

Possibly related PRs

  • PR #48: Adds conversation handlers/validators relying on ICurrentUserService and authorization checks, which directly conflicts with this PR's removal of ICurrentUser interface and CurrentUserService implementation.
  • PR #51: Modifies RefreshAccessTokenCommandHandler to use SecurityException for revoked/compromised tokens, which directly conflicts with this PR's removal of SecurityException and conversion to Result.Failure-based error handling in the same handler.
  • PR #47: Adds CurrentUserService/ICurrentUserService and authorization filters/behaviors with modifications to GlobalExceptionMiddleware, which directly conflicts with this PR's removal of CurrentUserService, ICurrentUser, and AuthorizationBehavior.

Poem

🐰 Hops through the code with glee,
Auth pipeline? No, we're free!
Results flow where exceptions flew,
Behaviors dance in code anew.
One filter guards them all so tight,
The API responses, pure delight! 🎯

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 64.71% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main architectural change: transitioning from exception-throwing to error-returning patterns, removing authorization behavior, and implementing a custom filter for consistent JWT responses.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor/change-all-code-from-throwing-exception-to-return-error

Comment @coderabbitai help to get the list of available commands and usage tips.

@taiphanvan2k3 taiphanvan2k3 merged commit b20dd7f into main Nov 22, 2025
2 of 3 checks passed
@taiphanvan2k3 taiphanvan2k3 deleted the refactor/change-all-code-from-throwing-exception-to-return-error branch November 22, 2025 14:45
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/Web.Api/Controllers/V1/UsersController.cs (1)

29-34: Inconsistent result handling pattern.

This controller still uses manual result checking (if (result.IsSuccess)) while AuthController and ConversationController have been refactored to use the centralized HandleResult(result) pattern. For consistency and maintainability, this should follow the same approach.

Apply this diff to align with the centralized pattern:

     var result = await mediator.Send(query);
-
-    if (result.IsSuccess)
-    {
-        return Ok(ApiResponse<PaginatedResult<UserSummary>>.CreateSuccess(result.Value!, "Users retrieved successfully"));
-    }
-
-    return BadRequest(ApiResponse<object>.CreateFailure(result.Error!.Description));
+    return HandleResult(result);
src/Application/Features/Conversation/GetMessages/GetMessagesQuery.cs (1)

10-21: Query summary still mentions caching though ICacheableQuery was removed

GetMessagesQuery no longer implements ICacheableQuery and has no cache-related members, but the XML summary still says “+ Strategy Pattern (caching)”. This is misleading now; consider updating the summary (and any related docs) to reflect that this query is no longer cached.

🧹 Nitpick comments (7)
src/Web.Api/Filters/AuthorizationLoggingFilter.cs (1)

1-1: Remove or implement the empty filter file.

The file is empty and should either contain logging implementation or be removed. Given that the PR introduces CustomAuthorizeFilter for standardizing authorization outcomes, clarify whether AuthorizationLoggingFilter is needed. If it's not part of the authorization refactoring scope, remove the file to avoid confusing future maintainers.

Can you confirm:

  1. Is this filter intended to be implemented as part of this PR, or should it be deleted?
  2. If it should be implemented, what logging functionality should it provide?

I can help you implement it once the requirements are clarified, or generate a removal commit if it's not needed.

src/Application/Features/Auth/Login/LoginCommandHandler.cs (1)

24-41: Result-based login failures are consistent; consider deactivation message semantics

The switch from exceptions to Result.Failure<LoginResponse> with Error.Unauthorized/Error.Forbidden is coherent and keeps the “Invalid email or password” message consistent for not-found vs bad-password cases. If you want to minimize user enumeration, you may also want to align the deactivated-account message with the generic invalid-credentials wording on Line 33 to avoid revealing account state.

src/Application/Common/Behaviors/CachingBehavior.cs (1)

57-77: Caching only successful results is good; verify non-Result/null behavior

Guarding the cache write with if (!response.IsFailure()) is a nice way to avoid caching failed Result instances. Note that IsFailure() only recognizes Domain.Common.Result types, so:

  • Non-Result responses (e.g., plain DTOs) will always be treated as successful and cached.
  • A null response (for reference types) will also be treated as success and cached.

If any handlers can legitimately return non-Result values or null to signal failure, you may want to extend the check (or add a null check) so those cases aren’t cached unintentionally.

src/Web.Api/Extensions/AuthenticationExtensions.cs (1)

31-84: JWT failure responses are well-shaped; add safety guards and tighten key handling

The custom OnChallenge/OnForbidden handlers nicely align JWT failures with your ApiResponse<ErrorInfo> model. A couple of improvements worth considering:

  • On Lines 48–66 and 68–83, add a context.Response.HasStarted check before writing the JSON body to avoid attempting to write to an already-committed response in edge cases.
  • On Line 31, instead of falling back to a hard-coded secret key, consider failing fast if JwtSettings:SecretKey is missing so you don’t accidentally run with a weak default in production.

Example sketch:

-            var secretKey = jwtSettings["SecretKey"] ?? "YourSuperSecretKeyThatIsAtLeast32CharactersLong!!";
+            var secretKey = jwtSettings["SecretKey"]
+                ?? throw new InvalidOperationException("JwtSettings:SecretKey configuration is missing.");

             // Custom response for JWT authentication failures
             options.Events = new JwtBearerEvents
             {
                 OnChallenge = async context =>
                 {
+                    if (context.Response.HasStarted)
+                    {
+                        return;
+                    }
                     context.HandleResponse();
                     // ... existing body ...
                 },

                 OnForbidden = async context =>
                 {
+                    if (context.Response.HasStarted)
+                    {
+                        return;
+                    }
                     // ... existing body ...
                 }
             };
docs/EXCEPTION_GUIDELINES.md (1)

440-452: Exception docs could acknowledge new Result-based error pattern

Given this PR’s broader move from exception-driven control flow to Result-based error handling, it may be worth updating this guideline (around Line 450) to explicitly state when to prefer returning Result over throwing exceptions (e.g., validation/auth/business rule failures vs truly exceptional conditions). That will keep future code aligned with the new error model.

src/Application/Common/Behaviors/LoggingBehavior.cs (1)

25-44: Failure-aware logging is helpful; consider minor refactor and exception story

Using response.IsFailure() and logging the Error.Code/Error.Description for failed Result responses is a solid improvement. Two small follow-ups you might consider:

  • To avoid repeated casts, you could inline the pattern match instead of using the extension here, e.g. if (response is Domain.Common.Result { IsSuccess: false } r) { /* log using r.Error */ } else { /* success log */ }.
  • Since the try/catch is gone, exceptions thrown by handlers will now bypass this behavior and be logged only by higher layers (e.g., GlobalExceptionMiddleware). If you still rely on MediatR outside of the web pipeline, you may want to confirm that those call sites have equivalent exception logging.
src/Web.Api/Controllers/V1/BaseController.cs (1)

14-29: Verify HandleResult adoption in UsersController

Verification shows good adoption in ConversationController (6 uses) and AuthController (5 uses), but UsersController has two manual response constructions that bypass the new pattern:

  • Line 31: Ok(ApiResponse<PaginatedResult<UserSummary>>.CreateSuccess(...))
  • Line 34: BadRequest(ApiResponse<object>.CreateFailure(...))

These should use HandleResult for consistency with other controllers and to benefit from centralized error mapping and status code selection.

HealthController returns non-Result objects (PingResponse, TestResponse) which is appropriate for health check endpoints.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f3ca946 and b404005.

📒 Files selected for processing (37)
  • docs/ARCHITECTURE_OVERVIEW.md (1 hunks)
  • docs/BEHAVIORS_GUIDE.md (4 hunks)
  • docs/EXCEPTION_GUIDELINES.md (1 hunks)
  • file_tree.md (0 hunks)
  • src/Application/Common/Behaviors/AuthorizationBehavior.cs (0 hunks)
  • src/Application/Common/Behaviors/CachingBehavior.cs (2 hunks)
  • src/Application/Common/Behaviors/DomainEventBehavior.cs (1 hunks)
  • src/Application/Common/Behaviors/LoggingBehavior.cs (2 hunks)
  • src/Application/Common/Behaviors/PerformanceBehavior.cs (2 hunks)
  • src/Application/Common/Behaviors/TransactionBehavior.cs (1 hunks)
  • src/Application/Common/Exceptions/BusinessRuleException.cs (0 hunks)
  • src/Application/Common/Exceptions/ConflictException.cs (0 hunks)
  • src/Application/Common/Exceptions/RequestProcessingException.cs (0 hunks)
  • src/Application/Common/Exceptions/SecurityException.cs (0 hunks)
  • src/Application/Common/ResultExtensions.cs (1 hunks)
  • src/Application/Features/Auth/GetProfile/GetProfileQuery.cs (0 hunks)
  • src/Application/Features/Auth/GetProfile/GetProfileQueryHandler.cs (0 hunks)
  • src/Application/Features/Auth/Login/LoginCommandHandler.cs (1 hunks)
  • src/Application/Features/Auth/RefreshAccessToken/RefreshAccessTokenCommandHandler.cs (2 hunks)
  • src/Application/Features/Conversation/GetMessages/GetMessagesQuery.cs (1 hunks)
  • src/Application/Features/User/GetUsers/GetUsersQuery.cs (1 hunks)
  • src/Domain/Common/Error.cs (1 hunks)
  • src/Domain/Common/ErrorType.cs (1 hunks)
  • src/Domain/Common/Result.cs (0 hunks)
  • src/Web.Api/Configurations/Options/JwtOptions.cs (0 hunks)
  • src/Web.Api/Controllers/V1/AuthController.cs (5 hunks)
  • src/Web.Api/Controllers/V1/BaseController.cs (1 hunks)
  • src/Web.Api/Controllers/V1/ConversationController.cs (6 hunks)
  • src/Web.Api/Controllers/V1/UsersController.cs (2 hunks)
  • src/Web.Api/Extensions/ApplicationServiceExtensions.cs (0 hunks)
  • src/Web.Api/Extensions/AuthenticationExtensions.cs (3 hunks)
  • src/Web.Api/Filters/AuthorizationLoggingFilter.cs (1 hunks)
  • src/Web.Api/Filters/CustomAuthorizeFilter.cs (1 hunks)
  • src/Web.Api/Middleware/GlobalExceptionMiddleware.cs (3 hunks)
  • src/Web.Api/Program.cs (2 hunks)
  • src/Web.Api/Services/CurrentUserService.cs (0 hunks)
  • src/Web.Api/Shared/JsonOptions.cs (1 hunks)
💤 Files with no reviewable changes (12)
  • src/Application/Common/Exceptions/RequestProcessingException.cs
  • src/Application/Common/Behaviors/AuthorizationBehavior.cs
  • src/Web.Api/Extensions/ApplicationServiceExtensions.cs
  • src/Application/Common/Exceptions/BusinessRuleException.cs
  • src/Application/Common/Exceptions/SecurityException.cs
  • src/Web.Api/Configurations/Options/JwtOptions.cs
  • src/Web.Api/Services/CurrentUserService.cs
  • src/Application/Features/Auth/GetProfile/GetProfileQuery.cs
  • src/Domain/Common/Result.cs
  • src/Application/Features/Auth/GetProfile/GetProfileQueryHandler.cs
  • src/Application/Common/Exceptions/ConflictException.cs
  • file_tree.md
🧰 Additional context used
🧬 Code graph analysis (11)
src/Web.Api/Extensions/AuthenticationExtensions.cs (2)
src/Web.Api/Middleware/GlobalExceptionMiddleware.cs (1)
  • StatusCode (57-154)
src/Web.Api/Shared/JsonOptions.cs (1)
  • JsonOptions (8-14)
src/Application/Common/Behaviors/DomainEventBehavior.cs (1)
src/Application/Common/ResultExtensions.cs (1)
  • IsFailure (16-23)
src/Application/Common/ResultExtensions.cs (1)
src/Domain/Common/Result.cs (8)
  • Result (5-34)
  • Result (7-17)
  • Result (25-25)
  • Result (27-28)
  • Result (30-30)
  • Result (32-33)
  • Result (36-53)
  • Result (40-44)
src/Web.Api/Program.cs (1)
src/Web.Api/Filters/CustomAuthorizeFilter.cs (1)
  • CustomAuthorizeFilter (13-69)
src/Application/Features/Auth/Login/LoginCommandHandler.cs (2)
src/Domain/Common/Result.cs (8)
  • Result (5-34)
  • Result (7-17)
  • Result (25-25)
  • Result (27-28)
  • Result (30-30)
  • Result (32-33)
  • Result (36-53)
  • Result (40-44)
src/Domain/Common/Error.cs (9)
  • Error (11-16)
  • Error (30-31)
  • Error (39-40)
  • Error (48-49)
  • Error (57-58)
  • Error (66-67)
  • Error (75-76)
  • Error (84-85)
  • Error (93-94)
src/Application/Features/Auth/RefreshAccessToken/RefreshAccessTokenCommandHandler.cs (3)
src/Domain/Entities/RefreshToken.cs (2)
  • RefreshToken (8-82)
  • RefreshToken (62-71)
src/Domain/Common/Result.cs (8)
  • Result (5-34)
  • Result (7-17)
  • Result (25-25)
  • Result (27-28)
  • Result (30-30)
  • Result (32-33)
  • Result (36-53)
  • Result (40-44)
src/Domain/Common/Error.cs (9)
  • Error (11-16)
  • Error (30-31)
  • Error (39-40)
  • Error (48-49)
  • Error (57-58)
  • Error (66-67)
  • Error (75-76)
  • Error (84-85)
  • Error (93-94)
src/Application/Common/Behaviors/CachingBehavior.cs (1)
src/Application/Common/ResultExtensions.cs (1)
  • IsFailure (16-23)
src/Application/Features/Conversation/GetMessages/GetMessagesQuery.cs (2)
src/Domain/Common/Result.cs (8)
  • Result (5-34)
  • Result (7-17)
  • Result (25-25)
  • Result (27-28)
  • Result (30-30)
  • Result (32-33)
  • Result (36-53)
  • Result (40-44)
src/Application/Common/Models/PaginatedResult.cs (1)
  • PaginatedResult (47-53)
src/Application/Common/Behaviors/PerformanceBehavior.cs (1)
src/Application/Common/ResultExtensions.cs (1)
  • IsFailure (16-23)
src/Application/Common/Behaviors/LoggingBehavior.cs (3)
src/Application/Common/ResultExtensions.cs (1)
  • IsFailure (16-23)
src/Domain/Common/Result.cs (8)
  • Result (5-34)
  • Result (7-17)
  • Result (25-25)
  • Result (27-28)
  • Result (30-30)
  • Result (32-33)
  • Result (36-53)
  • Result (40-44)
src/Domain/Common/Error.cs (9)
  • Error (11-16)
  • Error (30-31)
  • Error (39-40)
  • Error (48-49)
  • Error (57-58)
  • Error (66-67)
  • Error (75-76)
  • Error (84-85)
  • Error (93-94)
src/Web.Api/Filters/CustomAuthorizeFilter.cs (1)
src/Web.Api/Middleware/GlobalExceptionMiddleware.cs (1)
  • StatusCode (57-154)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build-and-deploy
🔇 Additional comments (15)
src/Application/Common/Behaviors/TransactionBehavior.cs (1)

20-43: TransactionBehavior implementation is clean and correct.

The behavior properly segregates transactional and non-transactional requests, delegates transaction management to IUnitOfWork, and threads the cancellation token appropriately. The marker interface pattern for ITransactionalCommand is a clean design that avoids runtime type checks. Logging is appropriately placed for debugging transaction lifecycle events.

docs/ARCHITECTURE_OVERVIEW.md (1)

74-74: LGTM! Documentation accurately reflects pipeline changes.

The removal of AuthorizationBehavior from the documented pipeline flow aligns with the PR's transition to controller-level authorization via filters.

src/Application/Common/Behaviors/DomainEventBehavior.cs (1)

23-26: LGTM! Proper guard for failed operations.

Preventing domain event collection and dispatch on failure is correct behavior. This ensures side effects only occur for successful commands.

src/Domain/Common/ErrorType.cs (1)

14-17: LGTM! Error types support enhanced authorization handling.

The addition of Unauthorized, Forbidden, and Security error types properly supports the shift to result-based error responses and aligns with HTTP semantics.

src/Web.Api/Controllers/V1/ConversationController.cs (1)

34-34: LGTM! Consistent centralized result handling.

All controller actions now use HandleResult(result) for consistent response formatting and status code mapping. This reduces duplication and improves maintainability.

Also applies to: 51-51, 66-66, 83-83, 99-99, 115-115

src/Application/Common/Behaviors/PerformanceBehavior.cs (1)

29-30: LGTM! Appropriate guard for performance logging.

Only logging slow requests for successful operations is correct - failed requests may be slow due to errors rather than actual performance issues.

src/Web.Api/Controllers/V1/AuthController.cs (1)

33-33: LGTM! Centralized result handling applied consistently.

All authentication endpoints now use HandleResult(result) for uniform response formatting across authentication flows.

Also applies to: 48-48, 63-63, 85-85, 101-101

src/Web.Api/Shared/JsonOptions.cs (1)

1-14: LGTM! Centralized JSON configuration.

Shared JsonOptions.Default ensures consistent JSON serialization across authentication responses and other API outputs.

src/Web.Api/Controllers/V1/UsersController.cs (1)

4-4: LGTM! Role-based authorization properly applied.

The [Authorize(Roles = "Admin,Manager")] attribute correctly restricts access to Admin and Manager roles at the controller level, aligning with the PR's shift away from pipeline-based authorization.

Also applies to: 14-14

src/Web.Api/Program.cs (1)

26-30: Global CustomAuthorizeFilter registration is correctly wired

Adding CustomAuthorizeFilter at the MVC options level ensures consistent 401/403 shaping across controllers and complements the JWT events configuration. No issues from a wiring perspective.

src/Application/Common/ResultExtensions.cs (1)

1-24: IsFailure extension correctly abstracts Result failure checks

The IsFailure<T> extension cleanly detects failed Domain.Common.Result instances (including Result<T>) and is suitable for use in pipeline behaviors like logging and caching.

src/Application/Features/User/GetUsers/GetUsersQuery.cs (1)

11-32: GetUsersQuery now only encodes caching; ensure authorization is enforced at API boundary

With GetUsersQuery no longer implementing IAuthorizedRequest, the query itself no longer carries explicit authorization requirements; it’s now purely a cacheable query. That’s fine with the new pipeline, but please double-check that all entry points invoking this query are protected via [Authorize]/policies so you don’t accidentally expose user listings via non-authorized callers.

src/Web.Api/Middleware/GlobalExceptionMiddleware.cs (1)

5-5: Shared JsonOptions and simplified exception mapping look good

Using JsonOptions.Default centralizes serialization behavior, and calling GetExceptionDetails with the original exception keeps the mapping consistent with your new Result/Error model. I don’t see functional regressions here.

Also applies to: 40-40, 52-52

src/Application/Features/Auth/RefreshAccessToken/RefreshAccessTokenCommandHandler.cs (1)

32-37: Result-based error handling for refresh token flow is consistent and safe

The new failure branches (invalid/expired token, revoked token, missing user) correctly return typed Result.Failure<RefreshAccessTokenResponse> values using the new Error.Unauthorized/Error.Security factories, and they short-circuit before touching the happy path. The compromised-token case still revokes the chain and saves changes before returning, preserving the security behavior. I don’t see functional regressions here.

Also applies to: 38-47, 49-53, 55-59

src/Web.Api/Filters/CustomAuthorizeFilter.cs (1)

1-67: CustomAuthorizeFilter cleanly standardizes 401/403 responses

The filter correctly distinguishes unauthenticated (ChallengeResult → 401) from unauthorized (ForbidResult → 403) cases, logs useful context (endpoint, method, path, and roles), and returns the standard ApiResponse<…> + ErrorInfo shape. It is properly registered as a global filter in Program.cs:29 and only overrides context.Result when it is a ChallengeResult or ForbidResult, ensuring it does not interfere with other filters.

Comment thread docs/BEHAVIORS_GUIDE.md
Comment on lines +9 to 10
Request → ValidationBehavior → LoggingBehavior → Handler → Response
```
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Docs still reference AuthorizationBehavior/IAuthorizedRequest despite removal

The guide is internally inconsistent and out of sync with the current pipeline:

  • The top pipeline (line 9) shows Request → ValidationBehavior → LoggingBehavior → Handler → Response, but the detailed pipeline (lines 115–143) still includes AuthorizationBehavior.
  • The “Authorization” usage section (lines 161–172) still documents IAuthorizedRequest and AuthorizationRequirement.
  • The comparison table (lines 195–201) advertises “Centralized authorization” via behaviors.

Now that authorization is handled differently (e.g., via filters/JWT policies), this doc should be updated to remove or rewrite these sections so it matches the actual behavior.

Also applies to: 115-143, 161-172, 195-201

🤖 Prompt for AI Agents
In docs/BEHAVIORS_GUIDE.md around lines 9–10, 115–143, 161–172, and 195–201, the
guide still references AuthorizationBehavior, IAuthorizedRequest, and
AuthorizationRequirement even though authorization has been moved to filters/JWT
policies; remove or rewrite all mentions of AuthorizationBehavior and
IAuthorizedRequest, update the top pipeline diagram to match the actual pipeline
(e.g., Request → ValidationBehavior → LoggingBehavior → Handler → Response),
update the detailed pipeline section to remove AuthorizationBehavior, replace
the “Authorization” usage section with a short description of how authorization
is now handled (filters, JWT policy enforcement, and links to relevant docs or
code), and update the comparison table to remove or reword the “Centralized
authorization” claim so the document is consistent with current implementation.

Comment on lines +24 to +32
/// <summary>
/// Dùng cho các trường hợp Lỗi Nghiệp Vụ (Business Logic): dữ liệu đúng định dạng nhưng không thỏa mãn business logic
/// </summary>
/// <param name="code"></param>
/// <param name="description"></param>
/// <returns></returns>
public static Error Problem(string code, string description) =>
new(code, description, ErrorType.Problem);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Align Error XML docs with actual semantics and HTTP mappings

The new factory methods are useful, but a couple of doc/semantics nits:

  • The XML comment for Failure mentions “Ví dụ lỗi 500 Internal Server Error”, while ErrorType.Failure is currently mapped to HTTP 400 in BaseController. Either the mapping or the comment should be adjusted so they agree.
  • The summaries for Unauthorized and Forbidden are almost identical (“người dùng không được phép truy cập”). It would help future usage if the docs explicitly distinguish:
    • Unauthorized → not authenticated (typically 401).
    • Forbidden → authenticated but lacking permissions (typically 403).

These are small doc-level tweaks but will make the error taxonomy much clearer.

Also applies to: 33-41, 42-58, 60-68, 69-77, 78-86, 87-94

🤖 Prompt for AI Agents
In src/Domain/Common/Error.cs around lines 24-32 (and similarly for ranges
33-41, 42-58, 60-68, 69-77, 78-86, 87-94), the XML summaries do not match the
code semantics and HTTP mappings: update the comment for Failure to reflect its
actual HTTP mapping (either change the text to indicate a 400 Bad Request if
BaseController maps ErrorType.Failure to 400, or change the code/mapping if you
prefer 500), and rewrite the Unauthorized and Forbidden summaries to clearly
distinguish authentication vs authorization (Unauthorized = not authenticated,
typically 401; Forbidden = authenticated but lacks permission, typically 403);
apply these doc edits consistently across the listed line ranges so comments
accurately describe each factory method and its HTTP expectation.

taiphanvan2k3 added a commit that referenced this pull request Dec 22, 2025
…ode-from-throwing-exception-to-return-error

feat: Implement custom authorization filter and enhance JWT authentication responses
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant