fix: access control - #57
Merged
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds role-based authorization: new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Pipeline as MediatR Pipeline
participant Auth as AuthorizationBehavior
participant User as ICurrentUserService
participant IdSvc as IIdentityService
participant Handler as Request Handler
participant DB as Database
Client->>Pipeline: Send Command (e.g., UpdateScheduleConfigCommand)
Pipeline->>Auth: Invoke AuthorizationBehavior<TRequest,TResponse>
Auth->>Auth: Inspect [Authorize] attributes on request type
alt No [Authorize]
Auth->>Handler: Call next handler
else Has [Authorize]
Auth->>User: Get current user ID
alt No current user
Auth-->>Client: Throw UnauthorizedAccessException
else Current user exists
Auth->>Auth: Filter attributes with Roles
alt No Roles defined
Auth->>Handler: Call next handler
else Roles defined
loop for each role
Auth->>IdSvc: IsInRoleAsync(userId, role)
end
alt User in any required role
Auth->>Handler: Call next handler
else Not in required roles
Auth-->>Client: Throw ForbiddenAccessException
end
end
end
end
Handler->>DB: Execute command logic
DB-->>Handler: Result
Handler-->>Client: Return response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/Booking/Booking.Application/Common/Behaviors/AuthorizationBehavior.cs (1)
3-3: Remove unused import.
Booking.Application.Doctors.Command.DeleteDoctoris not used in this file.Proposed fix
using Booking.Application.Common.Interfaces; using Booking.Application.Common.Security; -using Booking.Application.Doctors.Command.DeleteDoctor; using MediatR; using System.Reflection;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Booking/Booking.Application/Common/Behaviors/AuthorizationBehavior.cs` at line 3, Remove the unused using directive "using Booking.Application.Doctors.Command.DeleteDoctor;" from AuthorizationBehavior.cs; open the AuthorizationBehavior class in that file, delete that import line, then rebuild to confirm no references remain (ensure no symbols from DeleteDoctor are used in AuthorizationBehavior such as in Handle or constructor).src/Booking/Booking.Application/Doctors/Command/UpdateScheduleConfig/UpdateScheduleConfigCommandHandler.cs (2)
32-33: Inconsistent async usage:FirstOrDefaultshould beFirstOrDefaultAsync.Line 26 correctly uses
FirstOrDefaultAsync, but line 33 uses synchronousFirstOrDefault. For consistency and to avoid blocking the thread, use the async version.♻️ Proposed fix
- var config = _context.DoctorScheduleConfigs - .FirstOrDefault(x => x.DoctorId == request.DoctorId); + var config = await _context.DoctorScheduleConfigs + .FirstOrDefaultAsync(x => x.DoctorId == request.DoctorId, cancellationToken);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Booking/Booking.Application/Doctors/Command/UpdateScheduleConfig/UpdateScheduleConfigCommandHandler.cs` around lines 32 - 33, Replace the synchronous query call with the async variant: change the call on _context.DoctorScheduleConfigs that currently uses FirstOrDefault(x => x.DoctorId == request.DoctorId) to await _context.DoctorScheduleConfigs.FirstOrDefaultAsync(x => x.DoctorId == request.DoctorId) and ensure the containing method (UpdateScheduleConfigCommandHandler.Handle or similar) is async and returns a Task, adding the necessary using for Microsoft.EntityFrameworkCore if missing.
24-30: Misleading variable name:doctorIdholds aDoctorentity, not an ID.The variable
doctorIdactually contains the fullDoctorentity returned byFirstOrDefaultAsync. Consider renaming for clarity.✏️ Suggested rename
- var doctorId = await _context.Doctors + var doctor = await _context.Doctors .AsNoTracking() .FirstOrDefaultAsync(d => d.ApplicationUserId == currentUserId, cancellationToken) ?? throw new ForbiddenAccessException("Current user is not a doctor."); - if (request.DoctorId != doctorId.Id) + if (request.DoctorId != doctor.Id) throw new ForbiddenAccessException("You can only edit your own schedule.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Booking/Booking.Application/Doctors/Command/UpdateScheduleConfig/UpdateScheduleConfigCommandHandler.cs` around lines 24 - 30, The variable named `doctorId` is misleading because it holds a Doctor entity retrieved via `_context.Doctors.AsNoTracking().FirstOrDefaultAsync(...)`; rename it to something like `doctor` (or `currentDoctor`) everywhere in `UpdateScheduleConfigCommandHandler` so comparisons use `doctor.Id` (e.g., `if (request.DoctorId != doctor.Id)`) and the thrown exceptions (`ForbiddenAccessException`) remain unchanged; ensure all references to `doctorId` in this method are updated to the new name to keep intent clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Booking/Booking.API/Program.cs`:
- Around line 45-46: Remove the invalid open-generic registration
AddBehavior(typeof(IPipelineBehavior<,>)) and rely on AddOpenBehaviors(new[] {
typeof(ValidationBehavior<,>), typeof(AuthorizationBehavior<,>) }) to register
the pipeline behaviors; if the original intent was to register a closed/Concrete
implementation, replace the call with the closed-generic overload
AddBehavior<IPipelineBehavior<TRequest,TResponse>, ConcreteImplementation>()
referring to AddBehavior and AddOpenBehaviors and the
ValidationBehavior/AuthorizationBehavior types to locate the code.
In `@src/Booking/Booking.Application/Common/Security/AuthorizeAttribute.cs`:
- Around line 17-18: The Policy property on the AuthorizeAttribute class is
non-nullable but lacks an initializer, causing a compiler warning; change its
declaration from a non-nullable string to a nullable string (i.e., make Policy
of type string?) so it can be null until used, by updating the Policy property
signature in the AuthorizeAttribute class.
In
`@src/Booking/Booking.Application/Doctors/Command/UpdateScheduleConfig/UpdateScheduleConfigCommandHandler.cs`:
- Line 2: Remove the unused using for
Booking.Application.Doctors.Command.DeleteDoctor in the
UpdateScheduleConfigCommandHandler file and add a using for
Booking.Application.Common.Exceptions so the ForbiddenAccessException references
resolve; specifically, delete the import of DeleteDoctor and add the import for
Booking.Application.Common.Exceptions at the top of the file so the
ForbiddenAccessException thrown in UpdateScheduleConfigCommandHandler compiles.
---
Nitpick comments:
In `@src/Booking/Booking.Application/Common/Behaviors/AuthorizationBehavior.cs`:
- Line 3: Remove the unused using directive "using
Booking.Application.Doctors.Command.DeleteDoctor;" from
AuthorizationBehavior.cs; open the AuthorizationBehavior class in that file,
delete that import line, then rebuild to confirm no references remain (ensure no
symbols from DeleteDoctor are used in AuthorizationBehavior such as in Handle or
constructor).
In
`@src/Booking/Booking.Application/Doctors/Command/UpdateScheduleConfig/UpdateScheduleConfigCommandHandler.cs`:
- Around line 32-33: Replace the synchronous query call with the async variant:
change the call on _context.DoctorScheduleConfigs that currently uses
FirstOrDefault(x => x.DoctorId == request.DoctorId) to await
_context.DoctorScheduleConfigs.FirstOrDefaultAsync(x => x.DoctorId ==
request.DoctorId) and ensure the containing method
(UpdateScheduleConfigCommandHandler.Handle or similar) is async and returns a
Task, adding the necessary using for Microsoft.EntityFrameworkCore if missing.
- Around line 24-30: The variable named `doctorId` is misleading because it
holds a Doctor entity retrieved via
`_context.Doctors.AsNoTracking().FirstOrDefaultAsync(...)`; rename it to
something like `doctor` (or `currentDoctor`) everywhere in
`UpdateScheduleConfigCommandHandler` so comparisons use `doctor.Id` (e.g., `if
(request.DoctorId != doctor.Id)`) and the thrown exceptions
(`ForbiddenAccessException`) remain unchanged; ensure all references to
`doctorId` in this method are updated to the new name to keep intent clear.
|
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add new behavior, solve problem: If patient A can take access to data of Patient B. Now they cannot
Summary by CodeRabbit
New Features
Bug Fixes
Documentation