[PM-24279] Add vnext policy endpoint #6253
Conversation
| try | ||
| { | ||
| var json = JsonSerializer.Serialize(Metadata); | ||
| return CoreHelpers.LoadClassFromJsonData<T>(json); |
There was a problem hiding this comment.
@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.
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.
The outcome of this discussion will dictate this.
There was a problem hiding this comment.
Per the other thread, we can return to validation later.
| if (!_featureService.IsEnabled(FeatureFlagKeys.CreateDefaultLocation)) | ||
| { | ||
| throw new NotFoundException(); | ||
| } |
There was a problem hiding this comment.
You can apply the RequireFeatureAttribute to the method and this will be handled for you.
| if (!await _currentContext.ManagePolicies(orgId)) | ||
| { | ||
| throw new NotFoundException(); | ||
| } |
There was a problem hiding this comment.
...and you can use [Authorize<ManagePoliciesRequirement>] on the route and this will also be handled for you 😎
There was a problem hiding this comment.
Wow this is cool 😎 lol
| if (type != model?.Policy.Type) | ||
| { | ||
| throw new BadRequestException("Mismatched policy type"); | ||
| } |
There was a problem hiding this comment.
I'm not sure why we include the policy type in the route and the request model. Maybe it's time to pick one or the other and just remove this error state entirely.
There was a problem hiding this comment.
This is just me guessing, but it seems like the desire to have the type ID in the route is because the individual policies are treated as resources, so the type IDs are similar to a resource ID, but slightly different since it also depends on the parent’s ID.
Having it in the request body seems to make mapping operations easier.
But yeah, we should stick with one approach. I’m fine either way since they both have their pros and cons.
There was a problem hiding this comment.
If we decide to remove it from the route, I'll make sure to account for the order of the controller methods to prevent any clashes.
| return Policy?.Type switch | ||
| { | ||
| PolicyType.OrganizationDataOwnership => MapToPolicyMetadata<OrganizationModelOwnershipPolicyModel>(), | ||
| _ => new EmptyMetadataModel() | ||
| }; |
There was a problem hiding this comment.
(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 IPolicyMetadataModel), but that assumes that it's been correctly instantiated as the correct type by the api layer. This creates an unexpected dependency.
There is an advantage to the Policy.Data pattern, where the model is passed through as a json blob and only deserialized by the code that needs it when it needs it (in this case, the side effect method).
There was a problem hiding this comment.
this is an unexpected place that teams have to know to come and update
You’re right about this.
There is an advantage to the Policy.Data pattern, where the model is passed through as a json blob and only deserialized by the code that needs it when it needs it (in this case, the side effect method).
So we can follow this pattern, but we’ll run into this problem: currently, it doesn’t look like we’re validating Policy.Data at the controller level. If it fails, it seems like it will fail silently or return a 500 at best.
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 PR
I think the trade-offs are either:
- Use the existing pattern like
Policy.Dataand not have validation, which isn’t great since we have a required field. - Have the controller handle it and return a helpful error message, but this will have to live in the application layer, so it will be another place developers just have to know.
I'm open to any other ideas
There was a problem hiding this comment.
We should have validation on Policy.Data as well, so that is a bit of an unsolved problem. Ideally the json deserialization library would handle this, but I haven't looked into it. I would err on the side of following existing patterns, option 1, then we can return to the validation problem later.
That said, I understand we're trying to get this shipped, so I have no hard requirement here; whatever's reasonable.
| try | ||
| { | ||
| var json = JsonSerializer.Serialize(Metadata); | ||
| return CoreHelpers.LoadClassFromJsonData<T>(json); |
There was a problem hiding this comment.
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.
|
New Issues (2)Checkmarx found the following issues in this Pull Request
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6253 +/- ##
==========================================
+ Coverage 49.51% 49.56% +0.05%
==========================================
Files 1783 1787 +4
Lines 79374 79446 +72
Branches 7054 7062 +8
==========================================
+ Hits 39299 39380 +81
+ Misses 38544 38534 -10
- Partials 1531 1532 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…4279/add-vnext-policy-endpoint
src/Core/AdminConsole/OrganizationFeatures/Policies/Models/SavePolicyModel.cs
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/OrganizationFeatures/Policies/Implementations/SavePolicyCommand.cs
Outdated
Show resolved
Hide resolved
...e/OrganizationFeatures/Policies/PolicyValidators/OrganizationDataOwnershipPolicyValidator.cs
Outdated
Show resolved
Hide resolved
...e/OrganizationFeatures/Policies/PolicyValidators/OrganizationDataOwnershipPolicyValidator.cs
Outdated
Show resolved
Hide resolved
...e/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/OrganizationPolicyValidator.cs
Outdated
Show resolved
Hide resolved
| @@ -49,20 +66,13 @@ private async Task UpsertDefaultCollectionsForUsersAsync(PolicyUpdate policyUpda | |||
|
|
|||
| if (!userOrgIds.Any()) | |||
| { | |||
| logger.LogError("No UserOrganizationIds found for {OrganizationId}", policyUpdate.OrganizationId); | |||
There was a problem hiding this comment.
Removed the error log because this is a valid use case, as an org can enable and disable users. Only confirmed users are applicable.
|
There are a few things I need to check before this is ready:
|
|
| /// <summary> | ||
| /// Please do not use this validator. We're currently in the process of refactoring our policy validator pattern. | ||
| /// This is a stop-gap solution for post-policy-save side effects, but it is not the long-term solution. | ||
| /// </summary> | ||
| public abstract class OrganizationPolicyValidator(IPolicyRepository policyRepository, IEnumerable<IPolicyRequirementFactory<IPolicyRequirement>> factories) |
There was a problem hiding this comment.
Now that this no longer implements IPolicyValidator, I suggest deleting unused abstract methods relating to that interface:
- PolicyType
- RequiredPolicies
- OnSaveSideEffectsAsync
- ValidateAsync
And then deleting the (also unused) override implementations of those in OrganizationDataOwnershipPolicyValidator.
I think this is a small impact on the diff and functionality (for getting this PR merged) but just tidies up some of the loose ends from this PR.
|
|
||
| namespace Bit.Core.AdminConsole.OrganizationFeatures.Policies.PolicyValidators; | ||
|
|
||
| /// <summary> | ||
| /// Please do not use this validator. We're currently in the process of refactoring our policy validator pattern. |
There was a problem hiding this comment.
Suggestion for clarity:
| /// Please do not use this validator. We're currently in the process of refactoring our policy validator pattern. | |
| /// Please do not extend or expand this validator. We're currently in the process of refactoring our policy validator pattern. |
Given that we are in fact going to be using it.
| @@ -18,7 +18,7 @@ public void Customize(IFixture fixture) | |||
| } | |||
| } | |||
|
|
|||
| public class PolicyUpdateAttribute(PolicyType type, bool enabled = true) : CustomizeAttribute | |||
| public class PolicyUpdateAttribute(PolicyType type = PolicyType.FreeFamiliesSponsorshipPolicy, bool enabled = true) : CustomizeAttribute | |||
There was a problem hiding this comment.
This seems like an arbitrary default value, which isn't very intuitive. Why would it default to FreeFamiliesSponsorshipPolicy specifically? It looks like you're only using the default in 1 test - better to be explicit and just specify it there.
As an aside: if you did want this to be optional, then I think the better way would be to leave it to a random autodata value. e.g.
internal class PolicyUpdateCustomization(bool enabled, PolicyType? type = null) : ICustomization
{
public void Customize(IFixture fixture)
{
fixture.Customize<PolicyUpdate>(composer => composer
.With(o => o.Enabled, enabled)
.With(o => o.PerformedBy, new StandardUser(Guid.NewGuid(), false)));
if (type.HasValue)
{
fixture.Customize<PolicyUpdate>(composer => composer
.With(o => o.Type, type.Value));
}
}
}...but even then, random autodata values can also be a footgun; it could accidentally be the OrganizationDataOwnership policyType. Not required in this case I think - just specify the type you want in the test.
| private static IEnumerable<PolicyType> SampleUnsupportedTestCases() | ||
| { | ||
| yield return PolicyType.SingleOrg; | ||
| yield return PolicyType.TwoFactorAuthentication; | ||
| } | ||
|
|
||
| [Theory] | ||
| [BitMemberAutoData(nameof(SampleUnsupportedTestCases))] | ||
| public async Task VNextSaveAsync_NonOrganizationDataOwnershipPolicy_DoesNotExecutePostSaveSideEffects( | ||
| PolicyType policyType, | ||
| Policy currentPolicy, | ||
| [PolicyUpdate] PolicyUpdate policyUpdate) | ||
| { | ||
| // Arrange | ||
| policyUpdate.Type = policyType; | ||
| currentPolicy.Type = policyType; | ||
| currentPolicy.OrganizationId = policyUpdate.OrganizationId; |
There was a problem hiding this comment.
Suggestion (non-blocking - more of an fyi): If your parameterized test data are compile-time constants, you can use BitAutoData. In this case:
[Theory]
[BitAutoData(PolicyType.SingleOrg)]
[BitAutoData(PolicyType.TwoFactorAuthentication)]
public async Task VNextSaveAsync_NonOrganizationDataOwnershipPolicy_DoesNotExecutePostSaveSideEffects(...which is the same, but more concise.
MemberData is more useful when you have non-constant expressions, which you can't use in attribute constructors. For example, this won't work:
[Theory]
[BitAutoData(new Policy { Type = PolicyType.SingleOrg })]
[BitAutoData(new Policy { Type = PolicyType.TwoFactorAuthentication })]
public async Task VNextSaveAsync_NonOrganizationDataOwnershipPolicy_DoesNotExecutePostSaveSideEffects(So that's where you'd construct your objects in a static member and then point BitMemberAutoData at it.
In your case however, I suggest you can probably just use BitAutoData.
| [PolicyUpdate(PolicyType.OrganizationDataOwnership)] PolicyUpdate policyUpdate, | ||
| [Policy(PolicyType.OrganizationDataOwnership, false)] Policy currentPolicy, | ||
| [Policy(PolicyType.OrganizationDataOwnership, false)] Policy postUpdatedPolicy, | ||
| [Policy(PolicyType.OrganizationDataOwnership, false)] Policy previousPolicyState, |
There was a problem hiding this comment.
The postUpdatedPolicy state should match the policyUpdate state. Here the policyUpdate is enabled but the postUpdatedPolicy is not, which doesn't reflect real-world behavior.
Not an issue in your other tests.
| @@ -250,26 +253,28 @@ await sutProvider.GetDependency<ICollectionRepository>() | |||
| public async Task ExecuteSideEffectsAsync_WhenDefaultCollectionsDoesNotExist_DoesNothing( | |||
There was a problem hiding this comment.
Suggestion: this could use more descriptive test name, I had to look it up to see what it's testing.
ExecuteSideEffectsAsync_WhenDefaultCollectionNameIsInvalid_DoesNothing
You could also combine this with the ExecuteSideEffectsAsync_WhenMetadataIsNull_DoesNothing test, which is really a variant of this test case. yield return null.
| (_organization, _) = await OrganizationTestHelpers.SignUpAsync(_factory, plan: PlanType.EnterpriseAnnually2023, | ||
| ownerEmail: _ownerEmail, passwordManagerSeats: 10, paymentMethod: PaymentMethodType.Card); |
There was a problem hiding this comment.
We may as well use the latest enterprise plan - EnterpriseAnnually (no year suffix).
| var data = policy.GetDataModel<MasterPasswordPolicyData>(); | ||
| Assert.Equal(request.Policy.Data["minComplexity"], data.MinComplexity); | ||
| Assert.Equal(request.Policy.Data["minLength"], data.MinLength); | ||
| Assert.Equal(request.Policy.Data["requireUpper"], data.RequireUpper); | ||
| Assert.Equal(request.Policy.Data["requireLower"], data.RequireLower); | ||
| Assert.Equal(request.Policy.Data["requireNumbers"], data.RequireNumbers); | ||
| Assert.Equal(request.Policy.Data["requireSpecial"], data.RequireSpecial); | ||
| Assert.Equal(request.Policy.Data["enforceOnLogin"], data.EnforceOnLogin); |
There was a problem hiding this comment.
🎨 Suggestion (non-blocking): AssertHelper.AssertPropertyEqual can help with this.
| var responseContent = await response.Content.ReadAsStringAsync(); | ||
|
|
||
| var result = JsonSerializer.Deserialize<JsonElement>(responseContent); | ||
| Assert.True(result.GetProperty("enabled").GetBoolean()); | ||
| Assert.Equal((int)policyType, result.GetProperty("type").GetInt32()); |
There was a problem hiding this comment.
You can instantiate a model with response.Content.ReadFromJsonAsync<T>(), which makes assertions easier.
| // Assert | ||
| await AssertResponse(); | ||
|
|
||
| await AssertPolicy(); |
There was a problem hiding this comment.
You should also assert that the default collections were created. See OrganizationTestHelpers in this project for some helper methods to create other OrgUsers. You can also compare to Rui's confirm tests: https://github.com/bitwarden/server/blob/main/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUserControllerTests.cs#L112-L113
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-24279
📔 Objective
This is just the next endpoint, which provides a different interface that the client will consume later. The logic for this endpoint will be fleshed out in future work.
The main purpose of this PR is to introduce the route, request body, and mapper:
There are some concerns that we will not address in this PR. These concerns are documented in this doc and will be addressed in the near future.
📸 Screenshots
My frontend is having issues, so I'm testing this using Postman. Since we’re not hooking up this endpoint yet, this should be safe.
Confirmed that collections were created in the DB.
⏰ 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