From fc080f17311b479c6e1ef5c86867e676e17810ed Mon Sep 17 00:00:00 2001 From: Onat Buyukakkus <55088871+onbuyuka@users.noreply.github.com> Date: Wed, 3 Jun 2026 16:02:20 +0200 Subject: [PATCH] [Azure AD Plan] Update user access for GDAP users who were previously guests A delegated-admin (GDAP) user who had previously logged in to the environment as a guest had their plan row swapped to the GDAP plan but never had their user access (user groups / permissions) updated, so they did not effectively get the GDAP license. In codeunit 9018 "Azure AD Plan Impl.".AssignPlanToUserWithDelegatedRole, the caller passes SkipUpdateUserAccess = UserLoggedInEnvironment, which is true for any returning user. After deleting the old guest plan and inserting the GDAP plan, the procedure hit two early-exits: if SkipUpdateUserAccess then exit; // true for returning users ... if HasPlans then exit; // true for former guests so OnUpdateUserAccessForSaaS (user-group/permission assignment) never ran. The earlier "if IsPlanAssignedToUser(PlanId, UserSID) then exit" guard already prevents re-processing when the GDAP plan is unchanged, so the access update now only runs when the plan actually changes. Remove the two redundant early-exits (and the now-unused HasPlans) so access is updated on the guest -> GDAP transition. Revives the previously abandoned PR #7490 (closed without merging); same change + regression test GDAPUserWithPreviousGuestPlanGetsAccessUpdated. Fixes AB#612420 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/AzureADPlanImpl.Codeunit.al | 15 ++-- .../src/AzureADPlanTests.Codeunit.al | 72 +++++++++++++++++++ 2 files changed, 76 insertions(+), 11 deletions(-) diff --git a/src/System Application/App/Azure AD Plan/src/AzureADPlanImpl.Codeunit.al b/src/System Application/App/Azure AD Plan/src/AzureADPlanImpl.Codeunit.al index 515f5ae2d6..3dd072d237 100644 --- a/src/System Application/App/Azure AD Plan/src/AzureADPlanImpl.Codeunit.al +++ b/src/System Application/App/Azure AD Plan/src/AzureADPlanImpl.Codeunit.al @@ -662,7 +662,7 @@ codeunit 9018 "Azure AD Plan Impl." PlanIds: Codeunit "Plan Ids"; PlanConfigurationImpl: Codeunit "Plan Configuration Impl."; UserPermissions: Codeunit "User Permissions"; - UserGroupsAdded, ShouldRemoveSuper, HasPlans : Boolean; + UserGroupsAdded, ShouldRemoveSuper : Boolean; PlanId: Guid; begin case true of @@ -682,9 +682,6 @@ codeunit 9018 "Azure AD Plan Impl." Session.LogMessage('0000IC4', StrSubstNo(AssigningPlanForDelegatedRoleTxt, PlanId, not SkipUpdateUserAccess), Verbosity::Normal, DataClassification::SystemMetadata, TelemetryScope::ExtensionPublisher, 'Category', UserSetupCategoryTxt); - // Check if the user have any plans assigned before removing - HasPlans := DoesUserHavePlans(UserSID); - // Delete any existing plans for the user UserPlan.SetRange("User Security ID", UserSID); UserPlan.DeleteAll(); @@ -695,16 +692,12 @@ codeunit 9018 "Azure AD Plan Impl." UserPlan."User Security ID" := UserSID; UserPlan.Insert(); - // Exit if user access should not be updated - if SkipUpdateUserAccess then - exit; - + // Fix for ADO #612420: Removed early-exit checks on SkipUpdateUserAccess and HasPlans + // that prevented access updates when a user's plan changed (e.g., guest to GDAP). + // The IsPlanAssignedToUser guard above already prevents re-processing unchanged plans. if not UserProperty.Get(UserSID) then exit; - if HasPlans then - exit; - // Assign user groups for the user AzureADPlan.OnUpdateUserAccessForSaaS(UserPlan."User Security ID", UserGroupsAdded); diff --git a/src/System Application/Test/Azure AD Plan/src/AzureADPlanTests.Codeunit.al b/src/System Application/Test/Azure AD Plan/src/AzureADPlanTests.Codeunit.al index dfdf47cb10..5524f94123 100644 --- a/src/System Application/Test/Azure AD Plan/src/AzureADPlanTests.Codeunit.al +++ b/src/System Application/Test/Azure AD Plan/src/AzureADPlanTests.Codeunit.al @@ -7,6 +7,7 @@ namespace System.Test.Azure.ActiveDirectory; using System; using System.Azure.Identity; +using System.Security.AccessControl; using System.Security.User; using System.TestLibraries.Azure.ActiveDirectory; using System.TestLibraries.Environment; @@ -17,6 +18,7 @@ using System.TestLibraries.Utilities; codeunit 132912 "Azure AD Plan Tests" { Subtype = Test; + Permissions = tabledata "User Property" = rimd; TestPermissions = Disabled; EventSubscriberInstance = Manual; @@ -697,6 +699,76 @@ codeunit 132912 "Azure AD Plan Tests" LibraryAssert.IsTrue(UserPermissions.IsSuper(UserSID), 'User should be SUPER'); end; + [Test] + [TransactionModel(TransactionModel::AutoRollback)] + [CommitBehavior(CommitBehavior::Ignore)] + [Scope('OnPrem')] + procedure GDAPUserWithPreviousGuestPlanGetsAccessUpdated() + var + UserPlan: Record "User Plan"; + UserProperty: Record "User Property"; + AzureADPlan: Codeunit "Azure AD Plan"; + UserPermissions: Codeunit "User Permissions"; + AzureADPlanTestLibraries: Codeunit "Azure AD Plan Test Library"; + AzureADUserTestLibrary: Codeunit "Azure AD User Test Library"; + PlanConfigurationLibrary: Codeunit "Plan Configuration Library"; + UserPermissionsLibrary: Codeunit "User Permissions Library"; + AzureAdPlanTest: Codeunit "Azure AD Plan Tests"; + PlanIds: Codeunit "Plan Ids"; + UserSID: Guid; + begin + // [Scenario] A GDAP delegated admin user who was previously a guest user gets the GDAP plan + // assigned on login even when SkipUpdateUserAccess = true. + // Fix for ADO #612420: The early-exit conditions in AssignPlanToUserWithDelegatedRole + // prevented the GDAP plan from replacing the guest plan for returning users. + + DeleteAllFromTablePlanAndUserPlan(); + PlanConfigurationLibrary.ClearPlanConfigurations(); + BindSubscription(AzureAdPlanTest); + EnvironmentInformationTestLibrary.SetTestabilitySoftwareAsAService(true); + + // [Given] A SUPER user + UserSID := UserPermissionsLibrary.CreateSuperUser('NEWUSER'); + UserPermissionsLibrary.CreateSuperUser('ANOTHERUSER'); + + // [Given] The Delegated Admin agent - Partner plan exists + AzureADPlanTestLibraries.CreatePlan(PlanIds.GetDelegatedAdminPlanId(), 'Delegated Admin agent - Partner', 9022, '7584DDCA-27B8-E911-BB26-000D3A2B005C'); + + // [Given] The user previously had a guest plan (simulating a former guest user) + AzureADPlanTestLibraries.AssignUserToPlan(UserSID, AzureADPlanTestLibraries.CreatePlan('Guest Plan')); + + // [Given] The user has a UserProperty record (required for access update path) + if not UserProperty.Get(UserSID) then begin + UserProperty.Init(); + UserProperty."User Security ID" := UserSID; + UserProperty."Authentication Object ID" := UserSID; + UserProperty.Insert(); + end; + + // [Given] The current user is a delegated admin + BindSubscription(AzureADUserTestLibrary); + AzureADUserTestLibrary.SetIsUserDelegatedAdmin(true); + + // [Given] The plan configuration is NOT customized (default permissions apply) + // This avoids the RemoveSuperPermissions path which requires a real Windows user + LibraryAssert.IsFalse(AzureADPlan.IsPlanAssignedToUser(PlanIds.GetDelegatedAdminPlanId(), UserSID), 'GDAP plan should not yet be assigned'); + + // [When] The plan is assigned per delegated role with SkipUpdateUserAccess = true + // (simulating a returning user logging in - the caller Run() passes UserLoggedInEnvironment as SkipUpdateUserAccess) + AzureADPlan.AssignPlanToUserWithDelegatedRole(UserSID, true); + + // [Then] The GDAP plan is assigned to the user (the old guest plan was replaced) + // Bug 612420: With the buggy code, the procedure exited early when SkipUpdateUserAccess = true + // and the user had existing plans, preventing the GDAP plan from being assigned. + UserPlan.SetRange("User Security ID", UserSID); + LibraryAssert.AreEqual(1, UserPlan.Count(), 'There should be exactly one plan assignment (guest plan replaced by GDAP plan)'); + LibraryAssert.IsTrue(UserPlan.FindFirst(), 'There should be a plan assigned'); + LibraryAssert.AreEqual(PlanIds.GetDelegatedAdminPlanId(), UserPlan."Plan ID", 'The delegated admin plan should be assigned, replacing the guest plan'); + + // [Then] SUPER is still assigned (plan config is not customized, so SUPER is not removed) + LibraryAssert.IsTrue(UserPermissions.IsSuper(UserSID), 'User should still be SUPER (plan config is not customized)'); + end; + [Test] [TransactionModel(TransactionModel::AutoRollback)] [Scope('OnPrem')]