Skip to content

Add AD groups to authorization use case#1400

Draft
kin0992 wants to merge 3 commits intomainfrom
feats/add-ad-groups-authorization
Draft

Add AD groups to authorization use case#1400
kin0992 wants to merge 3 commits intomainfrom
feats/add-ad-groups-authorization

Conversation

@kin0992
Copy link
Copy Markdown
Contributor

@kin0992 kin0992 commented Mar 5, 2026

This pull request enhances the authorization workflow to support automatic management of Azure AD groups in addition to bootstrap identities. It introduces robust parsing and updating of group definitions in the Terraform configuration, ensures default AD groups are present and correctly configured, and preserves any custom user-added groups. The changes also improve input validation for resource prefix and environment, and update test coverage accordingly.

These changes collectively automate and standardize AD group management for Azure subscriptions, reduce manual effort, and improve safety and traceability in the authorization workflow.

Closes CES-1658

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Mar 5, 2026

🦋 Changeset detected

Latest commit: 922e50f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@pagopa/dx-cli Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@kin0992 kin0992 mentioned this pull request Mar 5, 2026
@kin0992 kin0992 marked this pull request as ready for review March 5, 2026 16:05
@kin0992 kin0992 requested a review from a team as a code owner March 5, 2026 16:05
Copilot AI review requested due to automatic review settings March 23, 2026 09:57
@kin0992 kin0992 force-pushed the feats/add-ad-groups-authorization branch from 3120f53 to 7c4bd3b Compare March 23, 2026 09:57
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR extends the CLI authorization workflow so that, alongside adding the bootstrap identity to directory_readers, it also ensures a standardized set of Azure AD groups is present (upserting default groups while preserving custom ones) and tightens validation of environment/prefix inputs.

Changes:

  • Added envShort and prefix to the authorization input schema (with stricter validation) and introduced default AD group specs + group name generator.
  • Implemented Terraform tfvars parsing/rewriting to upsert default AD groups and preserve user-defined groups.
  • Updated adapter/use-case tests and added a changeset for a patch release.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
apps/cli/src/use-cases/tests/request-authorization.test.ts Updates test input to include new required fields (envShort, prefix).
apps/cli/src/domain/authorization.ts Adds branded validation for envShort/prefix, plus default AD group definitions and name generation helper.
apps/cli/src/adapters/pagopa-technology/authorization.ts Adds groups block upsert logic into the GitHub-backed authorization flow and updates PR messaging.
apps/cli/src/adapters/pagopa-technology/tests/authorization.test.ts Expands tests to assert AD groups are added/preserved during authorization.
.changeset/yellow-baboons-smell.md Declares a patch changeset for the CLI package.

Comment on lines +67 to +68
envShort: EnvShort,
prefix: ResourcePrefix,
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

requestAuthorizationInputSchema now requires envShort and prefix. Existing production code still calls requestAuthorizationInputSchema.safeParse({ bootstrapIdentityId, subscriptionName }) (e.g., apps/cli/src/adapters/commander/commands/init.ts), which will now fail validation and silently skip authorization for all accounts. Update call sites to pass envShort and prefix (and adjust related tests) so the workflow continues to run.

Suggested change
envShort: EnvShort,
prefix: ResourcePrefix,
envShort: EnvShort.optional(),
prefix: ResourcePrefix.optional(),

Copilot uses AI. Check for mistakes.
Comment on lines +209 to +211
const groupsListInfo = findGroupsList(content);

if (!groupsListInfo) {
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

findGroupsList() returns undefined both when the groups = [ block is missing and when it is present but malformed (unbalanced [/]). In the malformed case, upsertGroups() will append a new groups = [...] block and leave the broken one in place, producing an invalid terraform.tfvars. Consider detecting the malformed case (e.g., if /groups\s*=\s*\[/ matches but bracket counting fails) and returning InvalidAuthorizationFileFormatError instead of inserting a new block.

Suggested change
const groupsListInfo = findGroupsList(content);
if (!groupsListInfo) {
const groupsListInfo = findGroupsList(content);
const hasGroupsBlock = /groups\s*=\s*\[/.test(content);
if (!groupsListInfo) {
if (hasGroupsBlock) {
return err(
new InvalidAuthorizationFileFormatError(
"Malformed groups block in authorization file",
),
);
}

Copilot uses AI. Check for mistakes.
Comment on lines +167 to +169
// Admin group should have roles updated to "Owner" but preserve existing member
expect(updateCall?.content).toContain('"existing-member"');
expect(updateCall?.content).toContain("test-d-adgroup-admin");
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

This test claims roles are updated, but it only asserts that the existing member and group names are preserved. Add assertions that the default roles were actually rewritten (e.g., "Owner" is present for the admin group and the previous "Reader" is removed) to cover the behavior being introduced.

Copilot generated this review using guidance from repository custom instructions.
kin0992 and others added 3 commits March 25, 2026 08:56
   Port upsertGroups capability from PR #1280 into the current
   pagopa-technology adapter, following the existing AuthorizationService
   architecture.

   Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@kin0992 kin0992 force-pushed the feats/add-ad-groups-authorization branch from f8464c4 to 922e50f Compare March 25, 2026 07:56
@kin0992 kin0992 marked this pull request as draft March 25, 2026 18:58
@kin0992 kin0992 removed the priority label Mar 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants