Uptake/rename license configurations#6404
Conversation
Renamed license configurations and updated upgrade code Related work items: #29700
Renamed license configurations and updated upgrade code Related work items: #29700
stkillen
left a comment
There was a problem hiding this comment.
A couple of comments below. Additionally, we need a PR for BaseApp to replace any usages of old functions.
New Base app PR to update old references https://github.com/microsoft/BusinessCentralApps/pull/1828 |
stkillen
left a comment
There was a problem hiding this comment.
Looking at Joost's comments earlier, we should match the regular plan names with the new delegated plan names. Added suggestions for the two plans and then we should rename in upgrade as well.
…ure.com/fenwick/Gold/_git/Yellowstone.BCApps into uptake/rename-license-configurations
|
Build fails with: AA0139 Possible overflow assigning 'Text' to 'Text[50]' |
Resolved |
There was a problem hiding this comment.
Pull request overview
This PR updates Azure AD plan (license) naming and IDs to align delegated/admin plan display names with Microsoft Entra role naming, including an upgrade path to rename existing plans and updating references across the app and tests.
Changes:
- Updates plan installation to create plans with new display names (and new/renamed PlanIds accessors).
- Adds an upgrade step intended to rename delegated/admin plans during upgrade.
- Updates tests and delegated/admin detection logic to use the renamed plans.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| src/System Application/Test/Azure AD Plan/src/PlanConfigurationE2ETest.Codeunit.al | Updates E2E test expectations and setup for renamed delegated plan. |
| src/System Application/Test/Azure AD Plan/src/AzureADPlanTests.Codeunit.al | Updates multiple plan-assignment tests to use renamed delegated/admin plans. |
| src/System Application/App/Azure AD User Management/src/User sync/AzureADUserSyncImpl.Codeunit.al | Updates comments describing admin plan combinations during sync. |
| src/System Application/App/Azure AD User Management/src/AzureADUserMgmtImpl.Codeunit.al | Updates delegated-user detection logic to use renamed plan IDs. |
| src/System Application/App/Azure AD Plan/src/User Details/PlanUserDetails.Codeunit.al | Updates “Is Delegated” derivation to use renamed plan IDs. |
| src/System Application/App/Azure AD Plan/src/PlanUpgradeTag.Codeunit.al | Adds a new upgrade tag accessor for the delegated/admin rename upgrade. |
| src/System Application/App/Azure AD Plan/src/PlanUpgrade.Codeunit.al | Adds/executes a new upgrade routine to rename delegated/admin plans; updates rename logging/classification. |
| src/System Application/App/Azure AD Plan/src/PlanInstaller.Codeunit.al | Updates initial plan creation to insert new display names and IDs. |
| src/System Application/App/Azure AD Plan/src/PlanIds.Codeunit.al | Introduces new PlanIds accessors and obsoletes older ones for renamed delegated plans. |
| src/System Application/App/Azure AD Plan/src/AzureADPlanImpl.Codeunit.al | Updates delegated-role assignment/admin checks to use renamed plan IDs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| RenameOrCreatePlan(PlanIds.GetPremiumISVPlanId(), 'Dynamics 365 Business Central Premium - Embedded'); | ||
| RenameOrCreatePlan(PlanIds.GetViralSignupPlanId(), 'Dynamics 365 Business Central for IWs'); | ||
| RenameOrCreatePlan(PlanIds.GetDelegatedAdminPlanId(), 'Delegated Admin agent - Partner'); | ||
| RenameOrCreatePlan(PlanIds.GetGlobalAdminPlanId(), 'Delegated Admin agent - Partner'); |
There was a problem hiding this comment.
RenamePlansAndDeleteOldPlans renames 'Delegated Admin agent - Partner' using PlanIds.GetGlobalAdminPlanId(). That GUID corresponds to the Global Administrator plan, not the delegated admin plan (DelegatedAdminGUIDTxt). This will rename the wrong plan during upgrade. Use PlanIds.GetDelegatedGlobalAdminPlanId() for the delegated plan rename.
| RenameOrCreatePlan(PlanIds.GetGlobalAdminPlanId(), 'Delegated Admin agent - Partner'); | |
| RenameOrCreatePlan(PlanIds.GetDelegatedGlobalAdminPlanId(), 'Delegated Admin agent - Partner'); |
| case true of | ||
| AzureADGraphUser.IsUserDelegatedAdmin(): | ||
| PlanId := PlanIds.GetDelegatedAdminPlanId(); | ||
| PlanId := PlanIds.GetGlobalAdminPlanId(); |
There was a problem hiding this comment.
Delegated admins are being assigned PlanIds.GetGlobalAdminPlanId() here. GetGlobalAdminPlanId() is the tenant Global Administrator role GUID, not the delegated admin plan GUID (DelegatedAdminGUIDTxt). This will assign the wrong plan to delegated admins and can grant/track the wrong entitlements. Use PlanIds.GetDelegatedGlobalAdminPlanId() for the delegated-admin path.
| PlanId := PlanIds.GetGlobalAdminPlanId(); | |
| PlanId := PlanIds.GetDelegatedGlobalAdminPlanId(); |
| AzureADPlanTestLibraries.CreatePlan(PlanIds.GetGlobalAdminPlanId(), 'Delegated Global Administrator', 9022, '7584DDCA-27B8-E911-BB26-000D3A2B005C'); | ||
|
|
||
| // [Given] The plan is not assigned to the current user | ||
| LibraryAssert.IsFalse(UserPlan.Get(PlanIds.GetDelegatedAdminPlanId(), UserSID), 'Plan should not be assigned to user'); | ||
| LibraryAssert.IsFalse(UserPlan.Get(PlanIds.GetGlobalAdminPlanId(), UserSID), 'Plan should not be assigned to user'); | ||
|
|
||
| // [Given] The current user is a delegated admin | ||
| BindSubscription(AzureADUserTestLibrary); | ||
| AzureADUserTestLibrary.SetIsUserDelegatedAdmin(true); | ||
|
|
||
| // [Given] The plan configuration for the plan is not customized | ||
| LibraryAssert.IsFalse(PlanConfiguration.IsCustomized(PlanIds.GetDelegatedAdminPlanId()), 'Plan configuration should not be customized'); | ||
| LibraryAssert.IsFalse(PlanConfiguration.IsCustomized(PlanIds.GetGlobalAdminPlanId()), 'Plan configuration should not be customized'); |
There was a problem hiding this comment.
Same issue as earlier in this file: this scenario uses GetGlobalAdminPlanId() for the delegated plan. Please switch to GetDelegatedGlobalAdminPlanId() so the test validates delegated-role behavior without conflating it with the real Global Administrator plan.
| AzureADPlanTestLibraries.CreatePlan(PlanIds.GetGlobalAdminPlanId(), 'Delegated Global Administrator', 9022, '7584DDCA-27B8-E911-BB26-000D3A2B005C'); | ||
|
|
||
| // [Given] The plan is not assigned to the current user | ||
| LibraryAssert.IsFalse(UserPlan.Get(PlanIds.GetDelegatedAdminPlanId(), UserSID), 'Plan should not be assigned to user'); | ||
| LibraryAssert.IsFalse(UserPlan.Get(PlanIds.GetGlobalAdminPlanId(), UserSID), 'Plan should not be assigned to user'); | ||
|
|
||
| // [Given] The current user is not a delegated admin | ||
| BindSubscription(AzureADUserTestLibrary); | ||
| AzureADUserTestLibrary.SetIsUserDelegatedAdmin(false); | ||
|
|
||
| // [Given] The plan configuration for the plan is not customized | ||
| LibraryAssert.IsFalse(PlanConfiguration.IsCustomized(PlanIds.GetDelegatedAdminPlanId()), 'Plan configuration should not be customized'); | ||
| LibraryAssert.IsFalse(PlanConfiguration.IsCustomized(PlanIds.GetGlobalAdminPlanId()), 'Plan configuration should not be customized'); |
There was a problem hiding this comment.
Same issue as earlier in this file: this scenario uses GetGlobalAdminPlanId() for the delegated plan and will pass even if delegated admins are incorrectly mapped to the Global Administrator plan. Use GetDelegatedGlobalAdminPlanId() for the delegated admin plan ID.
| /// <summary> | ||
| /// Returns the rename delegated admin plans upgrade tag. | ||
| /// </summary> | ||
| /// <returns>The rename delegated admin plans upgrade tag.</returns> | ||
| internal procedure GetRenameDelegatedAdminPlansUpgradeTag(): Code[250] | ||
| begin | ||
| exit('MS-582117-RenameDelegatedAdminPlans-20260128'); | ||
| end; |
There was a problem hiding this comment.
This new upgrade tag is referenced from Plan Upgrade via HasUpgradeTag/SetUpgradeTag, but it isn't added to RegisterPerDatabaseTags() in this codeunit. Please register it there as well; otherwise the tag list is incomplete and the upgrade tag framework may not preserve/report it correctly.
| RenameOrCreatePlan(PlanIds.GetD365BCAdminPlanId(), 'Delegated Dynamics 365 Business Central Administrator'); | ||
| RenameOrCreatePlan(PlanIds.GetGlobalAdminPlanId(), 'Delegated Global Administrator'); |
There was a problem hiding this comment.
RenameDelegatedAdminPlans renames GetGlobalAdminPlanId() twice (to 'Global Administrator' and then to 'Delegated Global Administrator'). The second rename should target the delegated admin plan GUID (GetDelegatedGlobalAdminPlanId()), otherwise it will overwrite the global admin plan name and never rename the delegated plan. Also note that 'Delegated Dynamics 365 Business Central Administrator' exceeds the Plan.Name length (50) and will be truncated by CopyStr, so the resulting display name will not match the intended role name.
| RenameOrCreatePlan(PlanIds.GetD365BCAdminPlanId(), 'Delegated Dynamics 365 Business Central Administrator'); | |
| RenameOrCreatePlan(PlanIds.GetGlobalAdminPlanId(), 'Delegated Global Administrator'); | |
| RenameOrCreatePlan(PlanIds.GetD365BCAdminPlanId(), 'Delegated D365 Business Central Administrator'); | |
| RenameOrCreatePlan(PlanIds.GetDelegatedGlobalAdminPlanId(), 'Delegated Global Administrator'); |
| /// <summary> | ||
| /// Returns the ID for the Delegated BC Admin agent - Partner plan. | ||
| /// </summary> | ||
| /// <returns>The ID for the Delegated BC Admin agent - Partner plan.</returns> | ||
| procedure GetD365BCAdminPlanId(): Guid | ||
| begin | ||
| exit(BCAdminPartnerGUIDTxt); | ||
| end; |
There was a problem hiding this comment.
The doc comments for GetD365BCAdminPlanId() still refer to the old delegated plan naming ('Delegated BC Admin agent - Partner'). Please update the summary/returns text to match the new delegated BC admin display name introduced in this PR (and keep it consistent with Plan Installer / upgrade naming).
| exit( | ||
| IsPlanAssignedToUser(PlanIds.GetGlobalAdminPlanId(), SecurityID) | ||
| or IsPlanAssignedToUser(PlanIds.GetDelegatedAdminPlanId(), SecurityID) | ||
| or IsPlanAssignedToUser(PlanIds.GetGlobalAdminPlanId(), SecurityID) |
There was a problem hiding this comment.
IsUserAdmin now checks GetGlobalAdminPlanId() twice, which makes the second condition redundant and drops the delegated-admin check that previously existed. Replace the duplicate with IsPlanAssignedToUser(PlanIds.GetDelegatedGlobalAdminPlanId(), SecurityID) (or the appropriate delegated plan ID) so delegated admins are still treated as admins when intended.
| or IsPlanAssignedToUser(PlanIds.GetGlobalAdminPlanId(), SecurityID) | |
| or IsPlanAssignedToUser(PlanIds.GetDelegatedGlobalAdminPlanId(), SecurityID) |
| PlanConfigurationLibrary.AddConfiguration(PlanIds.GetGlobalAdminPlanId(), false); | ||
| PlanConfiguration.AddDefaultPermissionSetToPlan(PlanIds.GetGlobalAdminPlanId(), 'SUPER', NullGuid, 0); |
There was a problem hiding this comment.
PlanIds.GetGlobalAdminPlanId() is the Entra Global Administrator role GUID, not the delegated admin plan GUID ({000...007}). Using it here will create a configuration for the wrong plan, and the page will show 'Global Administrator' rather than 'Delegated Global Administrator'. Use PlanIds.GetDelegatedGlobalAdminPlanId() for the delegated plan throughout this test setup.
| PlanConfigurationLibrary.AddConfiguration(PlanIds.GetGlobalAdminPlanId(), false); | |
| PlanConfiguration.AddDefaultPermissionSetToPlan(PlanIds.GetGlobalAdminPlanId(), 'SUPER', NullGuid, 0); | |
| PlanConfigurationLibrary.AddConfiguration(PlanIds.GetDelegatedGlobalAdminPlanId(), false); | |
| PlanConfiguration.AddDefaultPermissionSetToPlan(PlanIds.GetDelegatedGlobalAdminPlanId(), 'SUPER', NullGuid, 0); |
| AzureADPlanTestLibraries.CreatePlan(PlanIds.GetGlobalAdminPlanId(), 'Delegated Global Administrator', 9022, '7584DDCA-27B8-E911-BB26-000D3A2B005C'); | ||
|
|
||
| // [Given] The plan is not assigned to the current user | ||
| LibraryAssert.IsFalse(UserPlan.Get(PlanIds.GetDelegatedAdminPlanId(), UserSID), 'Plan should not be assigned to user'); | ||
| LibraryAssert.IsFalse(UserPlan.Get(PlanIds.GetGlobalAdminPlanId(), UserSID), 'Plan should not be assigned to user'); | ||
|
|
||
| // [Given] The current user is a delegated admin | ||
| BindSubscription(AzureADUserTestLibrary); | ||
| AzureADUserTestLibrary.SetIsUserDelegatedAdmin(true); | ||
|
|
||
| // [Given] The plan configuration for the plan is not customized | ||
| LibraryAssert.IsFalse(PlanConfiguration.IsCustomized(PlanIds.GetDelegatedAdminPlanId()), 'Plan configuration should not be customized'); | ||
| LibraryAssert.IsFalse(PlanConfiguration.IsCustomized(PlanIds.GetGlobalAdminPlanId()), 'Plan configuration should not be customized'); |
There was a problem hiding this comment.
These tests now create/assert a plan named 'Delegated Global Administrator' using GetGlobalAdminPlanId(). That GUID represents the non-delegated Global Administrator role ({62e9...}) and will hide regressions where delegated admins are mapped to the wrong plan. Use PlanIds.GetDelegatedGlobalAdminPlanId() for the delegated plan ID, and keep GetGlobalAdminPlanId() reserved for the actual Global Administrator plan.
Summary
Work Item(s)
Fixes #6403
AB#582117