[PM-21918] update send api models to support new email field
#5895
+22,505
โ55
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,4 @@ | ||
| ๏ปฟusing System.Text.Json; | ||
| using Azure.Messaging.EventGrid; | ||
| using Bit.Api.Models.Response; | ||
| using Bit.Api.Tools.Models.Request; | ||
|
|
@@ -16,6 +13,7 @@ | |
| using Bit.Core.Tools.Repositories; | ||
| using Bit.Core.Tools.SendFeatures; | ||
| using Bit.Core.Tools.SendFeatures.Commands.Interfaces; | ||
| using Bit.Core.Tools.SendFeatures.Queries.Interfaces; | ||
| using Bit.Core.Tools.Services; | ||
| using Bit.Core.Utilities; | ||
| using Microsoft.AspNetCore.Authorization; | ||
|
|
@@ -33,6 +31,9 @@ public class SendsController : Controller | |
| private readonly ISendFileStorageService _sendFileStorageService; | ||
| private readonly IAnonymousSendCommand _anonymousSendCommand; | ||
| private readonly INonAnonymousSendCommand _nonAnonymousSendCommand; | ||
|
|
||
| private readonly ISendOwnerQuery _sendOwnerQuery; | ||
|
|
||
| private readonly ILogger<SendsController> _logger; | ||
| private readonly GlobalSettings _globalSettings; | ||
|
|
||
|
|
@@ -42,6 +43,7 @@ public SendsController( | |
| ISendAuthorizationService sendAuthorizationService, | ||
| IAnonymousSendCommand anonymousSendCommand, | ||
| INonAnonymousSendCommand nonAnonymousSendCommand, | ||
| ISendOwnerQuery sendOwnerQuery, | ||
| ISendFileStorageService sendFileStorageService, | ||
| ILogger<SendsController> logger, | ||
| GlobalSettings globalSettings) | ||
|
|
@@ -51,6 +53,7 @@ public SendsController( | |
| _sendAuthorizationService = sendAuthorizationService; | ||
| _anonymousSendCommand = anonymousSendCommand; | ||
| _nonAnonymousSendCommand = nonAnonymousSendCommand; | ||
| _sendOwnerQuery = sendOwnerQuery; | ||
| _sendFileStorageService = sendFileStorageService; | ||
| _logger = logger; | ||
| _globalSettings = globalSettings; | ||
|
|
@@ -70,7 +73,11 @@ public async Task<IActionResult> Access(string id, [FromBody] SendAccessRequestM | |
|
|
||
| var guid = new Guid(CoreHelpers.Base64UrlDecode(id)); | ||
| var send = await _sendRepository.GetByIdAsync(guid); | ||
| if (send == null) | ||
| { | ||
| throw new BadRequestException("Could not locate send"); | ||
| } | ||
| var sendAuthResult = | ||
| await _sendAuthorizationService.AccessAsync(send, model.Password); | ||
| if (sendAuthResult.Equals(SendAccessResult.PasswordRequired)) | ||
| { | ||
|
|
@@ -86,7 +93,7 @@ public async Task<IActionResult> Access(string id, [FromBody] SendAccessRequestM | |
| throw new NotFoundException(); | ||
| } | ||
|
|
||
| var sendResponse = new SendAccessResponseModel(send); | ||
| if (send.UserId.HasValue && !send.HideEmail.GetValueOrDefault()) | ||
| { | ||
| var creator = await _userService.GetUserByIdAsync(send.UserId.Value); | ||
|
|
@@ -181,33 +188,29 @@ public async Task<ObjectResult> AzureValidateFile() | |
| [HttpGet("{id}")] | ||
| public async Task<SendResponseModel> Get(string id) | ||
| { | ||
| var sendId = new Guid(id); | ||
| var send = await _sendOwnerQuery.Get(sendId, User); | ||
| return new SendResponseModel(send); | ||
| } | ||
|
|
||
| [HttpGet("")] | ||
| public async Task<ListResponseModel<SendResponseModel>> GetAll() | ||
| { | ||
| var sends = await _sendOwnerQuery.GetOwned(User); | ||
| var responses = sends.Select(s => new SendResponseModel(s)); | ||
| var result = new ListResponseModel<SendResponseModel>(responses); | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| [HttpPost("")] | ||
| public async Task<SendResponseModel> Post([FromBody] SendRequestModel model) | ||
| { | ||
| model.ValidateCreation(); | ||
| var userId = _userService.GetProperUserId(User) ?? throw new InvalidOperationException("User ID not found"); | ||
| var send = model.ToSend(userId, _sendAuthorizationService); | ||
| await _nonAnonymousSendCommand.SaveSendAsync(send); | ||
| return new SendResponseModel(send); | ||
| } | ||
|
|
||
| [HttpPost("file/v2")] | ||
|
|
@@ -229,27 +232,27 @@ public async Task<SendFileUploadDataResponseModel> PostFile([FromBody] SendReque | |
| } | ||
|
|
||
| model.ValidateCreation(); | ||
| var userId = _userService.GetProperUserId(User) ?? throw new InvalidOperationException("User ID not found"); | ||
| var (send, data) = model.ToSend(userId, model.File.FileName, _sendAuthorizationService); | ||
| var uploadUrl = await _nonAnonymousSendCommand.SaveFileSendAsync(send, data, model.FileLength.Value); | ||
| return new SendFileUploadDataResponseModel | ||
| { | ||
| Url = uploadUrl, | ||
| FileUploadType = _sendFileStorageService.FileUploadType, | ||
| SendResponse = new SendResponseModel(send) | ||
| }; | ||
| } | ||
|
|
||
| [HttpGet("{id}/file/{fileId}")] | ||
| public async Task<SendFileUploadDataResponseModel> RenewFileUpload(string id, string fileId) | ||
| { | ||
| var userId = _userService.GetProperUserId(User) ?? throw new InvalidOperationException("User ID not found"); | ||
| var sendId = new Guid(id); | ||
| var send = await _sendRepository.GetByIdAsync(sendId); | ||
| var fileData = JsonSerializer.Deserialize<SendFileData>(send?.Data ?? string.Empty); | ||
|
|
||
| if (send == null || send.Type != SendType.File || (send.UserId.HasValue && send.UserId.Value != userId) || | ||
| !send.UserId.HasValue || fileData?.Id != fileId || fileData.Validated) | ||
| { | ||
| // Not found if Send isn't found, user doesn't have access, request is faulty, | ||
| // or we've already validated the file. This last is to emulate create-only blob permissions for Azure | ||
|
|
@@ -260,7 +263,7 @@ public async Task<SendFileUploadDataResponseModel> RenewFileUpload(string id, st | |
| { | ||
| Url = await _sendFileStorageService.GetSendFileUploadUrlAsync(send, fileId), | ||
| FileUploadType = _sendFileStorageService.FileUploadType, | ||
| SendResponse = new SendResponseModel(send), | ||
| }; | ||
| } | ||
|
|
||
|
|
@@ -270,12 +273,16 @@ public async Task<SendFileUploadDataResponseModel> RenewFileUpload(string id, st | |
| [DisableFormValueModelBinding] | ||
| public async Task PostFileForExistingSend(string id, string fileId) | ||
| { | ||
| if (!Request?.ContentType?.Contains("multipart/") ?? true) | ||
| { | ||
| throw new BadRequestException("Invalid content."); | ||
| } | ||
|
|
||
| var send = await _sendRepository.GetByIdAsync(new Guid(id)); | ||
| if (send == null) | ||
| { | ||
| throw new BadRequestException("Could not locate send"); | ||
| } | ||
| await Request.GetFileAsync(async (stream) => | ||
| { | ||
| await _nonAnonymousSendCommand.UploadFileToExistingSendAsync(stream, send); | ||
|
|
@@ -286,36 +293,39 @@ await Request.GetFileAsync(async (stream) => | |
| public async Task<SendResponseModel> Put(string id, [FromBody] SendRequestModel model) | ||
| { | ||
| model.ValidateEdit(); | ||
| var userId = _userService.GetProperUserId(User) ?? throw new InvalidOperationException("User ID not found"); | ||
| var send = await _sendRepository.GetByIdAsync(new Guid(id)); | ||
| if (send == null || send.UserId != userId) | ||
| { | ||
| throw new NotFoundException(); | ||
| } | ||
|
|
||
| await _nonAnonymousSendCommand.SaveSendAsync(model.UpdateSend(send, _sendAuthorizationService)); | ||
| return new SendResponseModel(send); | ||
| } | ||
|
|
||
| [HttpPut("{id}/remove-password")] | ||
|
Contributor
There was a problem hiding this comment. Do we need a remove-emails endpoint?
Contributor
There was a problem hiding this comment. I am actually not sure, did we solution that? I foresee a situation where only a subset of emails might need to be removed from any given send.
Contributor
There was a problem hiding this comment. Yes, we should be able to add emails and remove some or all as desired by the user. The current process of updating a Send should be able to handle that. I'm not sure why we have a remove-password endpoint versus just letting the owner of the Send remove it through editing the Send
Contributor
There was a problem hiding this comment. I found out that the UI offers the option to delete a password without editing a Send on almost all the clients. I have a question out to product and design about whether emails should be treated the same way |
||
| public async Task<SendResponseModel> PutRemovePassword(string id) | ||
| { | ||
| var userId = _userService.GetProperUserId(User) ?? throw new InvalidOperationException("User ID not found"); | ||
| var send = await _sendRepository.GetByIdAsync(new Guid(id)); | ||
| if (send == null || send.UserId != userId) | ||
| { | ||
| throw new NotFoundException(); | ||
|
Contributor
There was a problem hiding this comment. โ Critical: This endpoint creates inconsistent state by only clearing After this operation:
Recommendation: Also clear the send.Password = null;
send.Emails = null;
send.AuthType = Core.Tools.Enums.AuthType.None;This ensures a Send truly has "no authentication" after password removal.
Contributor
There was a problem hiding this comment. @mathewpriya @sukhleenb do either of you have any input why we may have added a dedicated endpoint for removing password based authentication? As opposed to using the existing endpoint to edit the send and remove the password instead? @itsadrago I am definitely not opposed to adding the endpoint if it will be used, but sometimes Claude's comments are a bit silly once any investigation takes place. |
||
| } | ||
|
|
||
| // This endpoint exists because PUT preserves existing Password/Emails when not provided. | ||
| // This allows clients to update other fields without re-submitting sensitive auth data. | ||
| send.Password = null; | ||
| send.AuthType = AuthType.None; | ||
| await _nonAnonymousSendCommand.SaveSendAsync(send); | ||
| return new SendResponseModel(send); | ||
| } | ||
|
|
||
| [HttpDelete("{id}")] | ||
| public async Task Delete(string id) | ||
| { | ||
| var userId = _userService.GetProperUserId(User) ?? throw new InvalidOperationException("User ID not found"); | ||
| var send = await _sendRepository.GetByIdAsync(new Guid(id)); | ||
| if (send == null || send.UserId != userId) | ||
| { | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
โ API Design: There's an asymmetry here - you can remove password authentication via
PUT /sends/{id}/remove-password, but there's no equivalentPUT /sends/{id}/remove-emailsendpoint for removing email authentication.This means:
Question: Is this intentional? If users should be able to remove email authentication, consider adding a symmetric endpoint for consistency.