Skip to content

[Azure AD Plan] Update access for GDAP users who were previously guests #8441

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.

Sign up for GitHub

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

Draft
onbuyuka wants to merge 1 commit into
base: main
Choose a base branch
from
+76 −11
Draft

[Azure AD Plan] Update access for GDAP users who were previously guests #8441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 4 additions & 11 deletions src/System Application/App/Azure AD Plan/src/AzureADPlanImpl.Codeunit.al
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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();
Expand All @@ -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
Copy link
Copy Markdown
Contributor

@nikolakukrika nikolakukrika Jun 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this line should stay as it is coming in as a parameter. If we remove it, we have a dead parameter that is used only in the telemetry.

Copy link
Copy Markdown
Contributor

@nikolakukrika nikolakukrika Jun 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that this PR requires more changes, probably to the caller functions too.

exit;

// Fix for ADO #612420: Removed early-exit checks on SkipUpdateUserAccess and HasPlans
Copy link
Copy Markdown
Contributor

@nikolakukrika nikolakukrika Jun 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets not check in the bug numbers in the code

// 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);

Expand Down
72 changes: 72 additions & 0 deletions src/System Application/Test/Azure AD Plan/src/AzureADPlanTests.Codeunit.al
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -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')]
Expand Down
Loading