PM-18939 refactoring send service to 'cqrs' #5652
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5652 +/- ##
==========================================
- Coverage 46.69% 46.62% -0.07%
==========================================
Files 1609 1619 +10
Lines 73195 73621 +426
Branches 6560 6617 +57
==========================================
+ Hits 34181 34329 +148
- Misses 37578 37854 +276
- Partials 1436 1438 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
New Issues (1)Checkmarx found the following issues in this Pull Request
Fixed Issues (1)Great job! The following issues were fixed in this Pull Request
|
|
|
||
| namespace Bit.Core.Tools.SendFeatures.Commands.Interfaces; | ||
|
|
||
| public interface INonAnonymousSendCommand |
There was a problem hiding this comment.
⛏️ Try to restate this using affirmative language. Define this as what it is as opposed to what it isn't.
🤔 It looks like there's 4 commands implemented on this interface. I'd expect only 1. Is this me not following along with how we use the pattern?
There was a problem hiding this comment.
CQRS doesn't strictly dictate that each command should only have one method. The core principles of CQRS are:
- Separate read operations (queries) from write operations (commands)
- Commands modify state but don't return data
- Queries return data but don't modify state
A lot of .NET folks use MediatR with the CQRS design pattern and typically you will see one command under one interface/class. However, it is valid to have command classes with multiple methods as long as they're all commands (changing state) and logically belong together.
I separated the commands by how they are used in the controller (Anonymous/non-anonymous). I can change this, but I kept them to together since we don't structure our backend code like MediatR.
Also, In NonAnonymousSendCommand.SaveFileSendAsync() I am cheating by returning a string, but didn't want to change the code too much for the 1st refactor with the send stuff you are working on. It feels like we using Controllers to be mediators or orchestrators and strong opinions about throwing only in the controller.
src/Core/Tools/SendFeatures/Services/Interfaces/ISendAuthorizationService.cs
Outdated
Show resolved
Hide resolved
| Task ValidateUserCanSaveAsync(Guid? userId, Send send); | ||
| Task ValidateUserCanSaveAsync_vNext(Guid? userId, Send send); |
There was a problem hiding this comment.
🤔 Is it necessary to support both the original flavor and vNext implementations here? Would a different pattern, such as creating multiple implementations of the interface, work better?
There was a problem hiding this comment.
I kept this code the way it was because of the feature flag, but could change it to use the strategy pattern for better isolation and testing purposes.
There was a problem hiding this comment.
So long as it won't cause other adverse effects, I think I'd like this updated to use the strategy pattern.
| throw new BadRequestException("Can only get a download URL for a file type of Send"); | ||
| } | ||
|
|
||
| var (grantAccess, passwordRequired, passwordInvalid) = _sendAuthorizationService.SendCanBeAccessed(send, password); |
There was a problem hiding this comment.
🌱 This pattern might need to be adjusted since we're moving to an authorization token model. How might this change in a situation where send can't load without a proper claim? (i.e. from a JWT bearer token received in an auth header.)
There was a problem hiding this comment.
We could use IHttpContextAccessor within this service to be able to check the roles of the user. I think this method could have these pieces refactored:
- Move the 'validation' pieces to the ISendValidationService to check user input (i.e lines 38-40 and IsNullOrWhiteSpace checks)
- Verification of the password would be moved the
GetSendFileDownloadUrlAsync - SendCanBeAccessed would just be concerned with the claims.
There was a problem hiding this comment.
We could use IHttpContextAccessor within this service to be able to check the roles of the user.
I'd prefer to use something independent of the execution context (ie. an HTTP request). The controller has access to the HttpContext, for example, and could extract the claim.
Verification of the password would be moved the GetSendFileDownloadUrlAsync
This won't work. The password won't be given to the file download URL. Instead, they'll have a bearer token containing a claim authorizing access to the download.
|
🎟️ Tracking
(https://bitwarden.atlassian.net/browse/PM-18939)
📔 Objective
Refactor current send codebase to align with our version of CQRS
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes