microsoft / ALAppExtensions

Repository for collaboration on Microsoft AL application add-on and localization extensions for Microsoft Dynamics 365 Business Central.
MIT License
768 stars 611 forks source link

Security Groups for OnPrem with AAD Authentication #23669

Closed ptrk-tr closed 2 months ago

ptrk-tr commented 1 year ago

Hi,

just a quick question: BC22 introduced the new Security Groups in the System Application to replace the existing User Groups in BC.

In BC22.1 this functionality does not work for OnPrem installations using Azure AD Authentication, is there any plan to make this work in the future before User Groups get replaced completely?

Following Call Stack will be run when trying to get the existing Security Groups from Azure in this context:

and then in the Initialize of CanQueryGraph() the first check is:

if not EnvironmentInformation.IsSaaS() then
            exit(false);

Which will always return false here, stopping execution of any code after this.

larswestman commented 1 year ago

I just upgraded a customer from BC14 to 22.1 and they rely heavily on user groups. They also use AAD authentication, so this will need to be fixed before the user groups are removed. It was a bit of a dissapointment to them that they couldn't manage the user/permission group mapping in AAD since they now have to do that also in BC.

joelsigurd commented 10 months ago

@JesperSchulz Any update on this issue? We have the same scenario (On-Prem with Azure AD authentication) and are experiencing exactly the same problem.

JesperSchulz commented 10 months ago

I will have @IhorHandziuk make an official statement when he is back in the office next week!

joelsigurd commented 9 months ago

@JesperSchulz @IhorHandziuk How about that statement? We need to know if we have to abandon using Security Groups.

JesperSchulz commented 9 months ago

We've had a lot of internal discussions to which extend we can set aside time to enable security groups for on-prem. Unfortunately it will be hard to find time to add support at this point in time. However, you are not the only ones looking into this limitation. Amongst a few others, @miljance has been looking into this as well. I wonder if this could be completed as an open source project amongst interested parties?

miljance commented 9 months ago

@JesperSchulz Currently we are analysing the impact on our OnPrem customer. A lot of them are in the queue for the update. I would say some of them are authenticating over AAD because of the multifactor authentication and all of those that did update did already hit the limitation. Currently we did not decide to invest in correcting this issue but if we do so I will be glad to contribute. For now, we are notifying the customers what is the limitation and that event though security groups sound cool, authenticating over AAD is not currently supported.

joelsigurd commented 9 months ago

I have done further investigating in to the problem. This is what I have tried and what I have found out:

This leads me to believe that if the if not EnvironmentInformation.IsSaaS() check is changed to something like if not (EnvironmentInformation.IsSaaS() or IsOnPrem()) the Security Groups lookup would work for both Cloud and OnPrem.

Another alternative that would be very simple for you to implement, would be to give us an IntegrationEvent at the start of the IsSaas() procedure, that lets developers override the return value.

miljance commented 9 months ago

That means that with this approach windows groups are enabled with Azure AD Authentication. I have asked @IhorHandziuk and @JesperSchulz and they would approve a pull request for this issue under following circumstances:

@joelsigurd That means that approach you have provided is not the desired end-state. At least that is how it sounds.

larswestman commented 9 months ago

One issue with using AAD security groups is that the redirect URI can become to long if the user has membership in many groups (and groups with long names will make it worse). When the redirect URI exceeds 256 chars the login to BC will fail. I don't know if this is an on-prem issue only though...

ptrk-tr commented 9 months ago

Make AAD security groups work OnPrem when the user is AAD authenticated

My first idea was using the IsAccessControlServiceAuthentication procedure in Codeunit 9801 or the underlying DotNet version, but looking into the CustomSettings.config 'AccessControlService' could also be used for Windows Azure Access Control Service as authentication type. Now the authentication documentation doesn't mention that type of authentication so I don't know if it's still something to check for.

Add some extra logic to make sure we don't mix and match Windows groups and AAD groups in the same environment (maybe just adding some dialog that tells the admin that they need to delete Windows groups first if they attempt to add AAD security groups the environment with Windows groups)

How would those get mixed and matched? Isn't Windows Authentication handled differently in the GetAvailableGroups() procedure in Codeunit 9871? As in, wouldn't they only get matched if the authentication type of the instance itself gets changed? But maybe I'm missing something here.

Reference from BC22.1:

if IsWindowsAuthentication() then
  foreach LocalWindowsGroupName in NavUserAccountHelper.GetLocalWindowsGroups() do begin
      SecurityGroupBuffer."Group ID" := CopyStr(SID(LocalWindowsGroupName), 1, MaxStrLen(SecurityGroupBuffer."Group ID"));
      if not GetDisallowedWindowsGroupIds().Contains(SecurityGroupBuffer."Group ID") then begin
          SecurityGroupBuffer.Code := CopyStr(CreateGuid(), 1, MaxStrLen(SecurityGroupBuffer.Code));
          SecurityGroupBuffer."Group Name" := CopyStr(LocalWindowsGroupName, 1, MaxStrLen(SecurityGroupBuffer."Group Name"));
          SecurityGroupBuffer.Insert();
      end;
  end
else begin
  InitializeAadGroups();

  foreach AadGroupId in AadGroups.Keys do begin
      UserProperty.SetRange("Authentication Object ID", AadGroupId);
      if UserProperty.IsEmpty() then begin
          SecurityGroupBuffer.Code := CopyStr(CreateGuid(), 1, MaxStrLen(SecurityGroupBuffer.Code));
          SecurityGroupBuffer."Group ID" := CopyStr(AadGroupId, 1, MaxStrLen(SecurityGroupBuffer."Group ID"));
          SecurityGroupBuffer."Group Name" := CopyStr(AadGroups.Get(AadGroupId), 1, MaxStrLen(SecurityGroupBuffer."Group Name"));
          SecurityGroupBuffer.Insert();
      end;
  end;
  end;
joelsigurd commented 9 months ago

Looking at the authentication documentation provided above I don't see the reason to having a check on IsSaaS().

If a user is trying to create Security Groups, and the IsWindowsAuthentication() check is false, isn't the user guaranteed to be trying to create AAD Security Groups?

joelsigurd commented 6 months ago

@JesperSchulz Is this still not prioritized in the Business Central development team?

At the moment we are using a hybrid solution with AAD for authentication, and Windows AD + User Groups for managing permissions. This has us concerned since User Groups are marked as Obsolete and planned for removal in BC25. Is this still the plan? As I see it there is no other option than using User Groups if you want to authenticate with AAD on Business Central On-Prem.

JesperSchulz commented 6 months ago

You should not loose any functionality when user groups are deprecated. Try tagging along on this Yammer / Viva Engage thread: https://www.yammer.com/dynamicsnavdev/#/threads/show?threadId=2515780681072640.

For convenience, copying the latest statement: permission sets support all the operations and scenarios that user groups did. There is no loss of functionality, and most actions have a direct mapping. If you have some examples scenarios becoming difficult to achieve, please share them.

JesperSchulz commented 2 months ago

This issue should not get tracked in this repository. It's tracked both in bcideas and is discussed in various Viva Engage threads. I'm hence closing this issue.

This repository is only for Business Central extensibility requests or code contributions towards the 1st party Business Central apps.