-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[PM-24279] Add vnext policy endpoint #6253
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 3 commits
bd50261
4dd1a48
7e75497
ff12cb6
42d8213
c636638
5e9b957
a57f268
83d9779
123808b
7da4718
4f5e545
09bb0df
0b82347
f8ddacd
6c3c60e
b37ec5b
5497657
7f78c1c
fa69074
e93f2a2
268d74f
914ef0b
46b9bdf
95c3f8f
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 |
---|---|---|
@@ -0,0 +1,61 @@ | ||
๏ปฟusing System.ComponentModel.DataAnnotations; | ||
using System.Text.Json; | ||
using Bit.Core.AdminConsole.Enums; | ||
using Bit.Core.AdminConsole.Models.Data; | ||
using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models; | ||
using Bit.Core.Context; | ||
using Bit.Core.Utilities; | ||
|
||
namespace Bit.Api.AdminConsole.Models.Request; | ||
|
||
public class SavePolicyRequest | ||
eliykat marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
[Required] | ||
public PolicyRequestModel Policy { get; set; } = null!; | ||
|
||
public Dictionary<string, object>? Metadata { get; set; } | ||
|
||
public async Task<SavePolicyModel> ToSavePolicyModelAsync(Guid organizationId, ICurrentContext currentContext) | ||
{ | ||
var performedBy = new StandardUser(currentContext.UserId!.Value, await currentContext.OrganizationOwner(organizationId)); | ||
|
||
var updatedPolicy = new PolicyUpdate() | ||
{ | ||
Type = Policy.Type!.Value, | ||
OrganizationId = organizationId, | ||
Data = Policy.Data != null ? JsonSerializer.Serialize(Policy.Data) : null, | ||
Enabled = Policy.Enabled.GetValueOrDefault(), | ||
}; | ||
|
||
var metadata = MapToPolicyMetadata(); | ||
|
||
return new SavePolicyModel(updatedPolicy, performedBy, metadata); | ||
} | ||
|
||
private IPolicyMetadataModel MapToPolicyMetadata() | ||
{ | ||
if (Metadata == null) | ||
{ | ||
return new EmptyMetadataModel(); | ||
} | ||
|
||
return Policy?.Type switch | ||
{ | ||
PolicyType.OrganizationDataOwnership => MapToPolicyMetadata<OrganizationModelOwnershipPolicyModel>(), | ||
_ => new EmptyMetadataModel() | ||
}; | ||
Comment on lines
+42
to
+46
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. (non-blocking) I want us to think about how this scales. It's not a huge deal right now because this is the only policy that uses this model. But as policies are added that use this pattern, this is an unexpected place that teams have to know to come and update, and it has some overhead in figuring out how it's passed through to their method. There is also a runtime issue (I think), where your side effect method is going to cast this to get the underlying type (rather than There is an advantage to the 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.
Youโre right about this.
So we can follow this pattern, but weโll run into this problem: currently, it doesnโt look like weโre validating Ideally, if the client passes in invalid data, we should return a 422 with a helpful message. Iโm guessing validation problems never arise because the fields in Policy.Data are optional? For this PRI think the trade-offs are either:
I'm open to any other ideas 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. We should have validation on That said, I understand we're trying to get this shipped, so I have no hard requirement here; whatever's reasonable. |
||
} | ||
|
||
private IPolicyMetadataModel MapToPolicyMetadata<T>() where T : IPolicyMetadataModel, new() | ||
{ | ||
try | ||
{ | ||
var json = JsonSerializer.Serialize(Metadata); | ||
return CoreHelpers.LoadClassFromJsonData<T>(json); | ||
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. @eliykat Currently, if the JSON format doesnโt match the C# DTO, it just returns an empty object of that C# DTO. Is this acceptable behavior, or should we handle it differently? 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. That's OK generally, except that in this case the collection name is required. So we still want some kind of check & throw if the collection name is not provided. 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. The outcome of this discussion will dictate this. 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. Per the other thread, we can return to validation later.
eliykat marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
catch | ||
{ | ||
return new EmptyMetadataModel(); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
๏ปฟ | ||
namespace Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models; | ||
|
||
public record EmptyMetadataModel : IPolicyMetadataModel | ||
{ | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
๏ปฟ | ||
namespace Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models; | ||
|
||
public interface IPolicyMetadataModel | ||
{ | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
๏ปฟ | ||
namespace Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models; | ||
|
||
public class OrganizationModelOwnershipPolicyModel : IPolicyMetadataModel | ||
{ | ||
public OrganizationModelOwnershipPolicyModel() | ||
{ | ||
} | ||
|
||
public OrganizationModelOwnershipPolicyModel(string? defaultUserCollectionName) | ||
{ | ||
DefaultUserCollectionName = defaultUserCollectionName; | ||
} | ||
|
||
public string? DefaultUserCollectionName { get; set; } | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
๏ปฟ | ||
using Bit.Core.AdminConsole.Models.Data; | ||
|
||
namespace Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models; | ||
|
||
public record SavePolicyModel(PolicyUpdate PolicyUpdate, IActingUser? PerformedBy, IPolicyMetadataModel Metadata) | ||
{ | ||
public PolicyUpdate PolicyUpdate { get; init; } = PolicyUpdate; | ||
public IPolicyMetadataModel Metadata { get; init; } = Metadata; | ||
|
||
public IActingUser? PerformedBy { get; init; } = PerformedBy; | ||
} | ||
eliykat marked this conversation as resolved.
Show resolved
Hide resolved
|
Uh oh!
There was an error while loading. Please reload this page.