-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[PM-21918] update send api models to support new email field
#5895
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
2d625f6
cc71297
74da4ab
360a3da
7b0b949
1e9023d
3271556
cd0a0ad
fb27708
d7a0282
481f94a
4217dba
b2c0dc8
c29ad65
bc22b1d
c7ba30b
2fec3db
ad4c002
68b6cad
25b0ed6
8a273fc
bfd06c0
b52f79e
4f4d63d
179d724
af39424
7df56e3
94926c5
8c1f7ee
4a179af
a934b43
47512f9
c85bd3e
1e4b1f3
f9bdfc0
1240fa4
cae8b59
f9b700e
6ccb9af
52376e5
f0ddab2
6bfacec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,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 +34,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 +46,7 @@ public SendsController( | |
| ISendAuthorizationService sendAuthorizationService, | ||
| IAnonymousSendCommand anonymousSendCommand, | ||
| INonAnonymousSendCommand nonAnonymousSendCommand, | ||
| ISendOwnerQuery sendOwnerQuery, | ||
| ISendFileStorageService sendFileStorageService, | ||
| ILogger<SendsController> logger, | ||
| GlobalSettings globalSettings) | ||
|
|
@@ -51,6 +56,7 @@ public SendsController( | |
| _sendAuthorizationService = sendAuthorizationService; | ||
| _anonymousSendCommand = anonymousSendCommand; | ||
| _nonAnonymousSendCommand = nonAnonymousSendCommand; | ||
| _sendOwnerQuery = sendOwnerQuery; | ||
| _sendFileStorageService = sendFileStorageService; | ||
| _logger = logger; | ||
| _globalSettings = globalSettings; | ||
|
|
@@ -181,23 +187,19 @@ public async Task<ObjectResult> AzureValidateFile() | |
| [HttpGet("{id}")] | ||
| public async Task<SendResponseModel> Get(string id) | ||
| { | ||
| var userId = _userService.GetProperUserId(User).Value; | ||
| var send = await _sendRepository.GetByIdAsync(new Guid(id)); | ||
| if (send == null || send.UserId != userId) | ||
| { | ||
| throw new NotFoundException(); | ||
| } | ||
|
|
||
| return new SendResponseModel(send, _globalSettings); | ||
| var sendId = new Guid(id); | ||
| var send = await _sendOwnerQuery.Get(sendId, User); | ||
| return new SendResponseModel(send); | ||
| } | ||
|
|
||
| [HttpGet("")] | ||
| public async Task<ListResponseModel<SendResponseModel>> Get() | ||
| { | ||
| var userId = _userService.GetProperUserId(User).Value; | ||
| var sends = await _sendRepository.GetManyByUserIdAsync(userId); | ||
| var responses = sends.Select(s => new SendResponseModel(s, _globalSettings)); | ||
| return new ListResponseModel<SendResponseModel>(responses); | ||
| var sends = await _sendOwnerQuery.GetOwned(User); | ||
| var responses = sends.Select(s => new SendResponseModel(s)); | ||
| var result = new ListResponseModel<SendResponseModel>(responses); | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| [HttpPost("")] | ||
|
|
@@ -207,7 +209,7 @@ public async Task<SendResponseModel> Post([FromBody] SendRequestModel model) | |
| var userId = _userService.GetProperUserId(User).Value; | ||
| var send = model.ToSend(userId, _sendAuthorizationService); | ||
| await _nonAnonymousSendCommand.SaveSendAsync(send); | ||
| return new SendResponseModel(send, _globalSettings); | ||
| return new SendResponseModel(send); | ||
| } | ||
|
|
||
| [HttpPost("file/v2")] | ||
|
|
@@ -236,7 +238,7 @@ public async Task<SendFileUploadDataResponseModel> PostFile([FromBody] SendReque | |
| { | ||
| Url = uploadUrl, | ||
| FileUploadType = _sendFileStorageService.FileUploadType, | ||
| SendResponse = new SendResponseModel(send, _globalSettings) | ||
| SendResponse = new SendResponseModel(send) | ||
| }; | ||
| } | ||
|
|
||
|
|
@@ -260,7 +262,7 @@ public async Task<SendFileUploadDataResponseModel> RenewFileUpload(string id, st | |
| { | ||
| Url = await _sendFileStorageService.GetSendFileUploadUrlAsync(send, fileId), | ||
| FileUploadType = _sendFileStorageService.FileUploadType, | ||
| SendResponse = new SendResponseModel(send, _globalSettings), | ||
| SendResponse = new SendResponseModel(send), | ||
| }; | ||
| } | ||
|
|
||
|
|
@@ -294,7 +296,7 @@ public async Task<SendResponseModel> Put(string id, [FromBody] SendRequestModel | |
| } | ||
|
|
||
| await _nonAnonymousSendCommand.SaveSendAsync(model.ToSend(send, _sendAuthorizationService)); | ||
| return new SendResponseModel(send, _globalSettings); | ||
| return new SendResponseModel(send); | ||
| } | ||
|
|
||
| [HttpPut("{id}/remove-password")] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need a remove-emails endpoint?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
|
@@ -309,7 +311,7 @@ public async Task<SendResponseModel> PutRemovePassword(string id) | |
|
|
||
| send.Password = null; | ||
| await _nonAnonymousSendCommand.SaveSendAsync(send); | ||
| return new SendResponseModel(send, _globalSettings); | ||
| return new SendResponseModel(send); | ||
| } | ||
|
|
||
| [HttpDelete("{id}")] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,35 +10,114 @@ | |
| using Bit.Core.Tools.Services; | ||
| using Bit.Core.Utilities; | ||
|
|
||
| using static System.StringSplitOptions; | ||
|
|
||
| namespace Bit.Api.Tools.Models.Request; | ||
|
|
||
| /// <summary> | ||
| /// A send request issued by a Bitwarden client | ||
| /// </summary> | ||
| public class SendRequestModel | ||
| { | ||
| /// <summary> | ||
| /// Indicates whether the send contains text or file data. | ||
| /// </summary> | ||
| public SendType Type { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Estimated length of the file accompanying the send. <see langword="null"/> when | ||
| /// <see cref="Type"/> is <see cref="SendType.Text"/>. | ||
| /// </summary> | ||
| public long? FileLength { get; set; } = null; | ||
|
|
||
| /// <summary> | ||
| /// Label for the send. | ||
| /// </summary> | ||
| [EncryptedString] | ||
| [EncryptedStringLength(1000)] | ||
| public string Name { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Notes for the send. This is only visible to the owner of the send. | ||
| /// </summary> | ||
| [EncryptedString] | ||
| [EncryptedStringLength(1000)] | ||
| public string Notes { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// A base64-encoded byte array containing the Send's encryption key. This key is | ||
| /// also provided to send recipients in the Send's URL. | ||
| /// </summary> | ||
| [Required] | ||
| [EncryptedString] | ||
| [EncryptedStringLength(1000)] | ||
| public string Key { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// The maximum number of times a send can be accessed before it expires. | ||
| /// When this value is <see langword="null" />, there is no limit. | ||
| /// </summary> | ||
| [Range(1, int.MaxValue)] | ||
| public int? MaxAccessCount { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// The date after which a send cannot be accessed. When this value is | ||
| /// <see langword="null"/>, there is no expiration date. | ||
| /// </summary> | ||
| public DateTime? ExpirationDate { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// The date after which a send may be automatically deleted from the server. | ||
| /// When this is <see langword="null" />, the send may be deleted after it has | ||
| /// exceeded the global send timeout limit. | ||
| /// </summary> | ||
| [Required] | ||
| public DateTime? DeletionDate { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Contains file metadata uploaded with the send. | ||
| /// The file content is uploaded separately. | ||
| /// </summary> | ||
| public SendFileModel File { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Contains text data uploaded with the send. | ||
| /// </summary> | ||
| public SendTextModel Text { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Base64-encoded byte array of a password hash that grants access to the send. | ||
| /// Mutually exclusive with <see cref="Emails"/>. | ||
| /// </summary> | ||
| [StringLength(1000)] | ||
| public string Password { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Comma-separated list of emails that may access the send using OTP | ||
| /// authentication. Mutually exclusive with <see cref="Password"/>. | ||
| /// </summary> | ||
| [StringLength(1024)] | ||
|
||
| public string Emails { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// When <see langword="true"/>, send access is disabled. | ||
| /// Defaults to <see langword="false"/>. | ||
| /// </summary> | ||
| [Required] | ||
| public bool? Disabled { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// When <see langword="true"/> send access hides the user's email address | ||
| /// and displays a confirmation message instead. Defaults to <see langword="false"/>. | ||
| /// </summary> | ||
| public bool? HideEmail { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Transforms the request into a send object. | ||
| /// </summary> | ||
| /// <param name="userId">The user that owns the send.</param> | ||
| /// <param name="sendAuthorizationService">Hashes the send password.</param> | ||
| /// <returns>The send object</returns> | ||
| public Send ToSend(Guid userId, ISendAuthorizationService sendAuthorizationService) | ||
djsmith85 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| var send = new Send | ||
|
|
@@ -50,8 +129,17 @@ public Send ToSend(Guid userId, ISendAuthorizationService sendAuthorizationServi | |
| return send; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Transforms the request into a send object and file data. | ||
| /// </summary> | ||
| /// <param name="userId">The user that owns the send.</param> | ||
| /// <param name="fileName">Name of the file uploaded with the send.</param> | ||
| /// <param name="sendAuthorizationService">Hashes the send password.</param> | ||
| /// <returns>The send object and file data.</returns> | ||
| public (Send, SendFileData) ToSend(Guid userId, string fileName, ISendAuthorizationService sendAuthorizationService) | ||
| { | ||
| // FIXME: This method does two things: creates a send and a send file data. | ||
| // It should only do one thing. | ||
| var send = ToSendBase(new Send | ||
| { | ||
| Type = Type, | ||
|
|
@@ -61,6 +149,13 @@ public Send ToSend(Guid userId, ISendAuthorizationService sendAuthorizationServi | |
| return (send, data); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Update a send object with request content | ||
| /// </summary> | ||
| /// <param name="existingSend">The send to update</param> | ||
| /// <param name="sendAuthorizationService">Hashes the send password.</param> | ||
| /// <returns>The send object</returns> | ||
| // FIXME: rename to `UpdateSend` | ||
| public Send ToSend(Send existingSend, ISendAuthorizationService sendAuthorizationService) | ||
| { | ||
| existingSend = ToSendBase(existingSend, sendAuthorizationService); | ||
|
|
@@ -81,6 +176,12 @@ public Send ToSend(Send existingSend, ISendAuthorizationService sendAuthorizatio | |
| return existingSend; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Validates that the request is internally consistent for send creation. | ||
| /// </summary> | ||
| /// <exception cref="BadRequestException"> | ||
| /// Thrown when the send's expiration date has already expired. | ||
| /// </exception> | ||
| public void ValidateCreation() | ||
| { | ||
| var now = DateTime.UtcNow; | ||
|
|
@@ -94,6 +195,13 @@ public void ValidateCreation() | |
| ValidateEdit(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Validates that the request is internally consistent for send administration. | ||
| /// </summary> | ||
| /// <exception cref="BadRequestException"> | ||
| /// Thrown when the send's deletion date has already expired or when its | ||
| /// expiration occurs after its deletion. | ||
| /// </exception> | ||
| public void ValidateEdit() | ||
| { | ||
| var now = DateTime.UtcNow; | ||
|
|
@@ -134,12 +242,23 @@ private Send ToSendBase(Send existingSend, ISendAuthorizationService authorizati | |
| existingSend.ExpirationDate = ExpirationDate; | ||
| existingSend.DeletionDate = DeletionDate.Value; | ||
| existingSend.MaxAccessCount = MaxAccessCount; | ||
| if (!string.IsNullOrWhiteSpace(Password)) | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(Emails)) | ||
| { | ||
| // normalize encoding | ||
| var emails = Emails.Split(',', RemoveEmptyEntries | TrimEntries); | ||
| existingSend.Emails = string.Join(", ", emails); | ||
|
||
| existingSend.Password = null; | ||
| } | ||
| else if (!string.IsNullOrWhiteSpace(Password)) | ||
| { | ||
| existingSend.Password = authorizationService.HashPassword(Password); | ||
| existingSend.Emails = null; | ||
| } | ||
|
|
||
| existingSend.Disabled = Disabled.GetValueOrDefault(); | ||
| existingSend.HideEmail = HideEmail.GetValueOrDefault(); | ||
|
|
||
| return existingSend; | ||
| } | ||
|
|
||
|
|
@@ -149,8 +268,15 @@ private SendTextData ToSendTextData() | |
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// A send request issued by a Bitwarden client | ||
| /// </summary> | ||
| public class SendWithIdRequestModel : SendRequestModel | ||
| { | ||
| /// <summary> | ||
| /// Identifies the send. When this is <see langword="null" />, the client is requesting | ||
| /// a new send. | ||
| /// </summary> | ||
| [Required] | ||
| public Guid? Id { get; set; } | ||
| } | ||
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.