Skip to content

Add Active Directory as a rotation target-system kind - #8303

Draft
maxkpower wants to merge 1 commit into
pam/uatfrom
pam/ad-kind
Draft

Add Active Directory as a rotation target-system kind#8303
maxkpower wants to merge 1 commit into
pam/uatfrom
pam/ad-kind

Conversation

@maxkpower

Copy link
Copy Markdown

🎟️ Tracking

📔 Objective

Adds on-premises Active Directory as a PAM rotation target-system kind.

  • PamTargetSystemKind.ActiveDirectory = 3, distinct from Entra, which is Entra ID.
  • rotation-server.allium records the kind and names an AD sAMAccountName as a kind-interpreted account_identity.
  • No migration. dbo.PamTargetSystem.Kind is a bare TINYINT NULL with no check constraint, and the integration test round-trips every kind to hold that.
  • Nothing branches on Kind. It is passed to the connector on the rotation claim, so there is no connector-specific server logic here.

Bottom of a two PR stack. It also gates work in sdk-internal and clients: both currently carry temporary stand-ins for the binding this produces, so this needs to reach main before those can drop them.

📸 Screenshots

@codecov

codecov Bot commented Sep 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.72%. Comparing base (60ca22a) to head (8a34d5f).

Additional details and impacted files
@@           Coverage Diff            @@
##           pam/uat    #8303   +/-   ##
========================================
  Coverage    64.72%   64.72%           
========================================
  Files         2599     2599           
  Lines       111839   111839           
  Branches     10022    10022           
========================================
  Hits         72388    72388           
  Misses       37033    37033           
  Partials      2418     2418           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@maxkpower maxkpower added the ai-review Request a Claude code review label Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the addition of PamTargetSystemKind.ActiveDirectory = 3 across src/Pam.Domain/Enums/PamTargetSystemKind.cs, the PamTargetSystem entity doc, the rotation-server.allium spec, and the unit and integration tests. The member is appended after CustomScript = 2, so persisted values and the numeric wire representation of the existing kinds are unchanged, and [dbo].[PamTargetSystem].[Kind] is a bare TINYINT NULL with no check constraint and no unique-name index that the new loop-per-kind test would trip, which supports the "no migration" claim. Traced Kind end to end — request binding validates it generically via [EnumDataType(typeof(PamTargetSystemKind))], RegisterTargetSystemCommand only enforces the method/kind shape rules, and the value is carried verbatim into PamTargetSystemResponseModel and RotationClaimResponseModel; nothing server-side branches on it, so no kind-specific handling is missing. Test changes match the code change: the command test parameterizes over all four kinds using the repo's established [BitAutoData(<enum literal>)] pattern, and the new integration test round-trips Enum.GetValues<PamTargetSystemKind>() so future kinds are covered without edits.

Code Review Details

No findings.

On-premises Active Directory joins Entra, MSSQL and CustomScript as a
connector kind the access connector can rotate against. It is distinct
from Entra, which is Entra ID.

The server does not branch on Kind: the value is stored and handed to the
daemon on the rotation claim, so no connector-specific server logic is
needed. dbo.PamTargetSystem.Kind is a bare TINYINT NULL with no check
constraint, so the new value needs no migration -- an integration test now
round-trips every kind to hold that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant