-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[PM-31884] Add access control support to Send Controls policy #7417
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
base: main
Are you sure you want to change the base?
Changes from all commits
2bb7c17
6f51c42
94b7025
7c4c042
bcc2960
1f7caf8
d52424c
235d832
e6bf8e6
b0af868
4a0728c
16cffeb
ea8527e
783b426
f8bbfa0
5022841
9b1b7e7
a6ea853
e6c1f11
ac6710d
c739d52
8bc9f7d
26baa10
b544964
50fdaf2
84d0cd1
f27e358
1f54881
f779c2f
d3cf561
697bae2
b23c233
947c3b5
657ee7d
2bafe52
c43c079
c4e326c
f3a4f16
3a53965
f0d09f5
44dd3cd
0732302
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,8 @@ | ||
| namespace Bit.Core.AdminConsole.Models.Data.Organizations.Policies; | ||
|
|
||
| public enum SendWhoCanAccessType | ||
| { | ||
| Any, | ||
| PasswordProtected, | ||
| SpecificPeople | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,6 +4,9 @@ | |||||||||
| using Bit.Core.AdminConsole.OrganizationFeatures.Policies.Models; | ||||||||||
| using Bit.Core.AdminConsole.OrganizationFeatures.Policies.PolicyUpdateEvents.Interfaces; | ||||||||||
| using Bit.Core.AdminConsole.Repositories; | ||||||||||
| using Bit.Core.Tools.Enums; | ||||||||||
| using Bit.Core.Tools.Repositories; | ||||||||||
| using Bit.Core.Tools.Services; | ||||||||||
|
|
||||||||||
| namespace Bit.Core.AdminConsole.OrganizationFeatures.Policies.PolicyEventHandlers; | ||||||||||
|
|
||||||||||
|
|
@@ -13,7 +16,8 @@ namespace Bit.Core.AdminConsole.OrganizationFeatures.Policies.PolicyEventHandler | |||||||||
| /// </summary> | ||||||||||
| public class SendControlsSyncPolicyEvent( | ||||||||||
| IPolicyRepository policyRepository, | ||||||||||
| TimeProvider timeProvider) : IOnPolicyPostUpdateEvent | ||||||||||
| TimeProvider timeProvider, | ||||||||||
| ISendRepository sendRepository) : IOnPolicyPostUpdateEvent, IPolicyValidationEvent | ||||||||||
| { | ||||||||||
| public PolicyType Type => PolicyType.SendControls; | ||||||||||
|
|
||||||||||
|
|
@@ -37,6 +41,8 @@ await UpsertLegacyPolicyAsync( | |||||||||
| PolicyType.SendOptions, | ||||||||||
| enabled: postUpsertedPolicyState.Enabled && sendControlsPolicyData.DisableHideEmail, | ||||||||||
| policyData: sendOptionsData); | ||||||||||
|
|
||||||||||
| await UpdateSendsByPolicyAsync(postUpsertedPolicyState, sendControlsPolicyData); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private async Task UpsertLegacyPolicyAsync<T>( | ||||||||||
|
|
@@ -60,4 +66,49 @@ private async Task UpsertLegacyPolicyAsync<T>( | |||||||||
|
|
||||||||||
| await policyRepository.UpsertAsync(policy); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| public Task<string> ValidateAsync(SavePolicyModel policyRequest, Policy? currentPolicy) | ||||||||||
| { | ||||||||||
| var dataModel = policyRequest.PolicyUpdate.GetDataModel<SendControlsPolicyData>(); | ||||||||||
| if (dataModel.AllowedDomains is not null && dataModel.WhoCanAccess != SendWhoCanAccessType.SpecificPeople) | ||||||||||
| { | ||||||||||
| return Task.FromResult("Allowed domains can only be set when the required access type is set to specific people"); | ||||||||||
| } | ||||||||||
| return Task.FromResult(string.Empty); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private async Task UpdateSendsByPolicyAsync(Policy postUpsertedPolicyState, SendControlsPolicyData sendControlsPolicyData) | ||||||||||
| { | ||||||||||
| var orgSendIds = await sendRepository.GetIdsByOrganizationIdAsync(postUpsertedPolicyState.OrganizationId); | ||||||||||
| foreach (var sendIdsChunk in orgSendIds.Chunk(50)) | ||||||||||
| { | ||||||||||
| var enabled = new List<Guid>(); | ||||||||||
| var disabled = new List<Guid>(); | ||||||||||
| var sendsChunk = await sendRepository.GetManyByIdsAsync(sendIdsChunk); | ||||||||||
| foreach (var send in sendsChunk) | ||||||||||
| { | ||||||||||
| if ( | ||||||||||
| // If the policy is disabled then we want to re-enable any Sends that were previously disabled | ||||||||||
| postUpsertedPolicyState.Enabled && | ||||||||||
| (sendControlsPolicyData.DisableSend || | ||||||||||
| (sendControlsPolicyData.DisableHideEmail && (send.HideEmail ?? false)) || | ||||||||||
| (sendControlsPolicyData.WhoCanAccess == SendWhoCanAccessType.PasswordProtected && send.AuthType != AuthType.Password) || | ||||||||||
| (sendControlsPolicyData.WhoCanAccess == SendWhoCanAccessType.SpecificPeople && send.AuthType != AuthType.Email) || | ||||||||||
| (sendControlsPolicyData.WhoCanAccess == SendWhoCanAccessType.SpecificPeople && !SendValidationService.SendAllEmailsHaveAllowedDomains(send.Emails, sendControlsPolicyData.AllowedDomains)))) | ||||||||||
| { | ||||||||||
|
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. Claude review suggested that a malformed email could cause a silent abort of the sync logic, what do you think? Are you confident that existing email validation would prevent this kind of situation? If not, here is the recommendation. I am undecided on this, since I recall implementing email validation logic on the clients.
Suggested change
With the addition of a helper method: private static bool IsNonCompliant(Send send, SendControlsPolicyData policyData)
{
if (policyData.DisableSend)
{
return true;
}
if (policyData.DisableHideEmail && (send.HideEmail ?? false))
{
return true;
}
if (policyData.WhoCanAccess == SendWhoCanAccessType.PasswordProtected
&& send.AuthType != AuthType.Password)
{
return true;
}
if (policyData.WhoCanAccess == SendWhoCanAccessType.SpecificPeople)
{
if (send.AuthType != AuthType.Email)
{
return true;
}
try
{
if (!SendValidationService.SendAllEmailsHaveAllowedDomains(send.Emails, policyData.AllowedDomains))
{
return true;
}
}
catch (BadRequestException)
{
// Historical Send rows may contain a malformed email that MailAddress rejects.
// We can't verify such a Send against the allowed-domains list, so treat it as
// non-compliant and disable it rather than aborting the org-wide sweep.
return true;
}
}
return false;
} |
||||||||||
| disabled.Add(send.Id); | ||||||||||
| } else | ||||||||||
| { | ||||||||||
| enabled.Add(send.Id); | ||||||||||
| } | ||||||||||
| } | ||||||||||
| if (enabled.Count > 0) { | ||||||||||
| await sendRepository.UpdateManyDisabledAsync(enabled, false); | ||||||||||
| } | ||||||||||
| if (disabled.Count > 0) | ||||||||||
| { | ||||||||||
| await sendRepository.UpdateManyDisabledAsync(disabled, true); | ||||||||||
| } | ||||||||||
| } | ||||||||||
| } | ||||||||||
| } | ||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| CREATE PROCEDURE [dbo].[Send_ReadByIds] | ||
| @Ids AS [dbo].[GuidIdArray] READONLY | ||
| AS | ||
| BEGIN | ||
| SET NOCOUNT ON | ||
|
|
||
| SELECT | ||
| * | ||
| FROM | ||
| [dbo].[SendView] | ||
| WHERE | ||
| [Id] IN (SELECT * FROM @Ids) | ||
| END |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| CREATE PROCEDURE [dbo].[Send_ReadIdsByOrgId] | ||
| @Id UNIQUEIDENTIFIER | ||
| AS | ||
| BEGIN | ||
| SET NOCOUNT ON | ||
|
|
||
| -- Get the IDs of all users in an org -- | ||
| DECLARE @OrgUserIds AS [GuidIdArray]; | ||
| INSERT INTO @OrgUserIds | ||
| SELECT DISTINCT | ||
| UserId | ||
| FROM | ||
| [dbo].[OrganizationUserView] | ||
| WHERE | ||
| OrganizationId = @Id | ||
|
|
||
| -- Get the IDs of all Sends associated with those users -- | ||
| SELECT | ||
| Id | ||
| FROM | ||
| [dbo].[SendView] | ||
| WHERE | ||
| UserId IN (SELECT [Id] FROM @OrgUserIds) | ||
| END |
|
mcamirault marked this conversation as resolved.
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| CREATE PROCEDURE [dbo].[Send_UpdateDisabledByIds] | ||
| @Ids AS [dbo].[GuidIdArray] READONLY, | ||
| @Disabled BIT | ||
| AS | ||
| BEGIN | ||
| SET NOCOUNT ON | ||
|
|
||
| DECLARE @UserIds [dbo].[GuidIdarray] | ||
|
|
||
| -- Set field | ||
| UPDATE | ||
| [dbo].[Send] | ||
| SET | ||
| [Disabled] = @Disabled, | ||
| [RevisionDate] = GETUTCDATE() | ||
| WHERE | ||
| [Id] IN (SELECT * FROM @Ids) | ||
|
|
||
| INSERT INTO @UserIds | ||
| SELECT DISTINCT | ||
| UserId | ||
| FROM | ||
| [dbo].[Send] | ||
| WHERE | ||
| [Id] IN (SELECT * FROM @Ids) | ||
| AND [UserId] IS NOT NULL | ||
|
|
||
| -- Bump account revision dates | ||
| EXEC [dbo].[User_BumpManyAccountRevisionDates] @UserIds | ||
| END |
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.
β without the suggested change, does the sync logic exactly match the validation logic?
I think the current logic in
SendValidationServicewill allow create and edit operations on Sends when the allowed domain list is null, but that the sync logic will disable every existing Send when the allowed domain list is null or empty.The suggested change applies the same null guard used in
SendValidationServiceto the sync logic.