ameyer505 / D365FOAdminToolkit

A D365FO administrative toolkit created by and for the community
MIT License
22 stars 9 forks source link

Add filter to exclude Microsoft accounts #29

Closed jofme closed 1 week ago

jofme commented 1 week ago

Hi @ameyer505

I had a glance at #27, and as the error says, there are triggers on the table that check if it is a service account.

CREATE TRIGGER [dbo].[PreventAdminUserDisablementUserInfo]
ON [dbo].[USERINFO]
AFTER UPDATE
AS
BEGIN
    SET NOCOUNT ON;
    SET XACT_ABORT ON;
    IF (UPDATE(enable) and (SELECT count(1) FROM INSERTED I WHERE I.ID='Admin' AND I.ENABLE=0) > 0) --Update
    THROW 51000, 'Admin user cannot be disabled', 1;
END

CREATE TRIGGER [dbo].[PreventPowerPlatformAppUserDisablementUserInfo]
ON [dbo].[USERINFO]
AFTER UPDATE
AS
BEGIN
    SET NOCOUNT ON;
    SET XACT_ABORT ON;
    IF (UPDATE(enable) and (SELECT count(1) FROM INSERTED I WHERE I.ID='PowerPlatformApp' AND I.ENABLE=0) > 0) --Update
    THROW 51000, 'PowerPlatformApp user cannot be disabled!', 1;
END

I am concerned that Microsoft might introduce more service accounts in production environments without notice, and our users forget to populate system accounts in the exclude users form.

I propose we make a small modification to the disable user job that excludes users with isMicrosoftAccount == true. If we look at the security role classes in the standard application, there are several places where they check for this field. The accounts we auto-populate in the list are marked with this.

The downside is that the column is not indexed, and there might be a minor performance penalty for instances with many users.

    while select forupdate id, name, enable from ui where ui.enable == true && ui.isMicrosoftAccount == false

Feel free to close the pull request if you don't see it necessary.

ameyer505 commented 1 week ago

@jofme I think this is a good approach - my follow up question then is with this does it make sense to have a 'Populate Excluded User' button anymore?

I would lean towards no, I'll put in a separate PR to remove that button and update the documentation with the change.

jofme commented 1 week ago

@jofme I think this is a good approach - my follow up question then is with this does it make sense to have a 'Populate Excluded User' button anymore?

I would lean towards no, I'll put in a separate PR to remove that button and update the documentation with the change.

Thanks! Depends... It might be worth investigating whether changing the "Populate Excluded User" button to a query dialog would make sense for the users. For example, if a user wants to exclude an entire support department at once.

ameyer505 commented 1 week ago

@jofme - good point, I do think we can probably make the populate logic dynamic by iterating through the list of available users and checking if the 'isMicrosoftAccount' parameter is enabled for the user and adding them to the excluded list instead of having a static list.

Then if end users want to extend the logic (for example by adding in their support team) they can use chain of command on the method and add their additional logic in.