feat(112): LIST parity + Entra principal

This commit is contained in:
Ahmed Darrazi 2026-02-26 00:48:25 +01:00
parent 7ac53f4cc4
commit 6a9d482c23
16 changed files with 1081 additions and 14 deletions

View File

@ -3,6 +3,7 @@
namespace App\Console\Commands;
use App\Services\Graph\GraphClientInterface;
use App\Services\Graph\GraphContractRegistry;
use Illuminate\Console\Command;
class GraphContractCheck extends Command
@ -11,7 +12,7 @@ class GraphContractCheck extends Command
protected $description = 'Validate Graph contract registry against live endpoints (lightweight probes)';
public function handle(GraphClientInterface $graph): int
public function handle(GraphClientInterface $graph, GraphContractRegistry $registry): int
{
$contracts = config('graph_contracts.types', []);
@ -36,11 +37,13 @@ public function handle(GraphClientInterface $graph): int
continue;
}
$query = array_filter([
$queryInput = array_filter([
'$top' => 1,
'$select' => $select,
'$expand' => $expand,
]);
], static fn ($value): bool => $value !== null && $value !== '' && $value !== []);
$query = $registry->sanitizeQuery($type, $queryInput)['query'];
$response = $graph->request('GET', $resource, [
'query' => $query,

View File

@ -84,7 +84,9 @@ private function fetchRoleDefinitions(array $graphOptions): array
*/
private function fetchRoleAssignments(array $graphOptions): array
{
$response = $this->graphClient->listPolicies('entraRoleAssignments', $graphOptions);
$response = $this->graphClient->listPolicies('entraRoleAssignments', array_merge($graphOptions, [
'expand' => 'principal',
]));
if ($response->failed()) {
throw new RuntimeException('Failed to fetch Entra role assignments from Graph API.');

View File

@ -7,8 +7,19 @@ interface GraphClientInterface
/**
* List policies of a given type.
*
* Supported list query options (sanitized against the contract allowlists for the policy type):
* - `select`: `string|string[]` Optional `$select` fields. Accepts comma-separated string or list of fields.
* - `expand`: `string|string[]` Optional `$expand` expansions. Accepts comma-separated string or list of tokens.
* String input is split on top-level commas only (commas inside balanced parentheses are not separators).
* - `filter`: `string` Optional `$filter`.
* - `top`: `int` Optional `$top`.
* - `platform`: `string` Optional platform filter (contract-specific).
*
* Tenant/auth context options (typically resolved by `MicrosoftGraphOptionsResolver`):
* - `tenant`, `client_id`, `client_secret`, `scope`, `token_url`, `access_token`, `client_request_id`
*
* @param string $policyType Graph policy type identifier
* @param array $options Additional filters (e.g., platform) and tenant/app context
* @param array{select?: string|string[], expand?: string|string[], filter?: string, top?: int, platform?: string, client_request_id?: string, tenant?: string, client_id?: string, client_secret?: string, scope?: string|string[], token_url?: string, access_token?: string} $options
*/
public function listPolicies(string $policyType, array $options = []): GraphResponse;

View File

@ -3,9 +3,14 @@
namespace App\Services\Graph;
use Illuminate\Support\Arr;
use Illuminate\Support\Facades\Log;
class GraphContractRegistry
{
private const MAX_EXPAND_ITEMS = 10;
private const MAX_EXPAND_TOKEN_LENGTH = 200;
public function probePath(string $key, array $replacements = []): ?string
{
$path = config("graph_contracts.probes.$key.path");
@ -181,20 +186,49 @@ public function sanitizeQuery(string $policyType, array $query): array
if (! empty($query['$expand'])) {
$original = $query['$expand'];
$expand = is_array($original)
? $original
: [trim((string) $original)];
$expand = array_values(array_filter($expand, static fn ($value) => $value !== ''));
$filtered = array_values(array_intersect($expand, $allowedExpand));
$allowedExpand = is_array($allowedExpand) ? $allowedExpand : [];
$requested = $this->normalizeExpandInput($original);
if (count($filtered) !== count($expand)) {
$tooLong = array_values(array_filter(
$requested,
static fn (string $token): bool => strlen($token) > self::MAX_EXPAND_TOKEN_LENGTH,
));
$bounded = array_values(array_filter(
$requested,
static fn (string $token): bool => strlen($token) <= self::MAX_EXPAND_TOKEN_LENGTH,
));
if ($tooLong !== []) {
$warnings[] = 'Trimmed overly long $expand fields for capability safety.';
}
$allowed = array_values(array_intersect($bounded, $allowedExpand));
$disallowed = array_values(array_diff($bounded, $allowed));
if ($disallowed !== []) {
$warnings[] = 'Trimmed unsupported $expand fields for capability safety.';
}
if ($filtered === []) {
$truncated = [];
if (count($allowed) > self::MAX_EXPAND_ITEMS) {
$truncated = array_slice($allowed, self::MAX_EXPAND_ITEMS);
$allowed = array_slice($allowed, 0, self::MAX_EXPAND_ITEMS);
$warnings[] = sprintf('Trimmed $expand fields to a maximum of %d items for capability safety.', self::MAX_EXPAND_ITEMS);
}
if ($tooLong !== [] || $disallowed !== [] || $truncated !== []) {
$this->logSanitizedExpand($policyType, $requested, $allowed, [
'too_long' => $tooLong,
'disallowed' => $disallowed,
'max_items' => $truncated,
]);
}
if ($allowed === []) {
unset($query['$expand']);
} else {
$query['$expand'] = implode(',', $filtered);
$query['$expand'] = implode(',', $allowed);
}
}
@ -204,6 +238,124 @@ public function sanitizeQuery(string $policyType, array $query): array
];
}
/**
* Normalize $expand input into an ordered, de-duped list of tokens.
*
* @return array<int, string>
*/
private function normalizeExpandInput(mixed $original): array
{
$tokens = is_array($original)
? $original
: $this->splitOnTopLevelCommas((string) $original);
$normalized = [];
$seen = [];
foreach ($tokens as $token) {
if (! is_string($token)) {
continue;
}
$token = trim($token);
if ($token === '') {
continue;
}
if (isset($seen[$token])) {
continue;
}
$seen[$token] = true;
$normalized[] = $token;
}
return $normalized;
}
/**
* Split comma-separated strings on top-level commas only, preserving commas
* inside balanced parentheses.
*
* @return array<int, string>
*/
private function splitOnTopLevelCommas(string $value): array
{
$depth = 0;
$buffer = '';
$tokens = [];
$length = strlen($value);
for ($index = 0; $index < $length; $index++) {
$character = $value[$index];
if ($character === '(') {
$depth++;
$buffer .= $character;
continue;
}
if ($character === ')') {
$depth = max(0, $depth - 1);
$buffer .= $character;
continue;
}
if ($character === ',' && $depth === 0) {
$tokens[] = $buffer;
$buffer = '';
continue;
}
$buffer .= $character;
}
$tokens[] = $buffer;
return $tokens;
}
/**
* @param array<int, string> $requested
* @param array<int, string> $allowed
* @param array{too_long: array<int, string>, disallowed: array<int, string>, max_items: array<int, string>} $removedByReason
*/
private function logSanitizedExpand(string $policyType, array $requested, array $allowed, array $removedByReason): void
{
$removed = array_values(array_merge(
$removedByReason['too_long'],
$removedByReason['disallowed'],
$removedByReason['max_items'],
));
if ($removed === []) {
return;
}
$context = [
'policy_type' => $policyType,
'query_key' => '$expand',
'requested' => $requested,
'allowed' => $allowed,
'removed' => $removed,
'removed_by_reason' => array_filter($removedByReason, static fn (array $values): bool => $values !== []),
'max_items' => self::MAX_EXPAND_ITEMS,
'max_token_len' => self::MAX_EXPAND_TOKEN_LENGTH,
];
if (app()->isProduction()) {
Log::debug('Graph query sanitized', $context);
return;
}
Log::warning('Graph query sanitized', $context);
}
public function matchesTypeFamily(string $policyType, ?string $odataType): bool
{
if ($odataType === null) {

View File

@ -61,6 +61,7 @@ public function listPolicies(string $policyType, array $options = []): GraphResp
'$top' => $options['top'] ?? null,
'$filter' => $options['filter'] ?? null,
'$select' => $defaultSelect,
'$expand' => $options['expand'] ?? null,
'platform' => $options['platform'] ?? null,
], fn ($value) => $value !== null && $value !== '');

View File

@ -0,0 +1,35 @@
# Specification Quality Checklist: Graph Contracts — LIST $expand Parity Fix
**Purpose**: Validate specification completeness and quality before proceeding to planning
**Created**: 2026-02-25
**Feature**: [spec.md](../spec.md)
## Content Quality
- [x] No implementation details (languages, frameworks, APIs)
- [x] Focused on user value and business needs
- [x] Written for non-technical stakeholders
- [x] All mandatory sections completed
## Requirement Completeness
- [x] No [NEEDS CLARIFICATION] markers remain
- [x] Requirements are testable and unambiguous
- [x] Success criteria are measurable
- [x] Success criteria are technology-agnostic (no implementation details)
- [x] All acceptance scenarios are defined
- [x] Edge cases are identified
- [x] Scope is clearly bounded
- [x] Dependencies and assumptions identified
## Feature Readiness
- [x] All functional requirements have clear acceptance criteria
- [x] User scenarios cover primary flows
- [x] Feature meets measurable outcomes defined in Success Criteria
- [x] No implementation details leak into specification
## Notes
- Items marked incomplete require spec updates before `/speckit.clarify` or `/speckit.plan`
- Validation run: 2026-02-25 — spec/plan/tasks aligned on top-level comma splitting (no splitting inside parentheses), explicit caps (10 items / 200 chars), and deterministic production diagnostics (debug-level).

View File

@ -0,0 +1,40 @@
{
"$schema": "https://json-schema.org/draft/2020-12/schema",
"title": "GraphClientInterface::listPolicies options",
"type": "object",
"additionalProperties": true,
"properties": {
"select": {
"description": "Optional $select. Accepts comma-separated string or array of strings. Sanitized against contract allowed_select.",
"oneOf": [
{ "type": "string" },
{ "type": "array", "items": { "type": "string" } }
]
},
"expand": {
"description": "Optional $expand. Accepts comma-separated string or array of strings. For string input, splitting is top-level only (commas inside balanced parentheses are not separators). Sanitized against contract allowed_expand.",
"oneOf": [
{ "type": "string" },
{ "type": "array", "items": { "type": "string" } }
]
},
"filter": { "type": "string", "description": "Optional $filter." },
"top": { "type": "integer", "minimum": 1, "description": "Optional $top." },
"platform": { "type": "string", "description": "Optional platform filter (contract-specific)." },
"client_request_id": { "type": "string", "description": "Optional client request id for Graph correlation." },
"tenant": { "type": "string", "description": "Optional tenant override (primarily for diagnostics/commands)." },
"access_token": { "type": "string", "description": "Optional delegated access token override." },
"client_id": { "type": "string" },
"client_secret": { "type": "string" },
"scope": {
"description": "Optional scope override.",
"oneOf": [
{ "type": "string" },
{ "type": "array", "items": { "type": "string" } }
]
},
"token_url": { "type": "string" }
}
}

View File

@ -0,0 +1,43 @@
# Data Model — Graph Contracts: LIST `$expand` Parity Fix
## Summary
No database schema changes. This feature changes how list queries are constructed and sanitized for Microsoft Graph.
## Key entities & shapes
### Graph contract (`config/graph_contracts.php`)
Per `policyType` contract keys relevant to this feature:
- `resource` (string): Graph resource path (e.g. `directory/roleAssignments`)
- `allowed_select` (string[]): allowlisted `$select` fields
- `allowed_expand` (string[]): allowlisted `$expand` expansions (exact-match)
### Query options (caller input)
Options are passed as an `array $options` into `GraphClientInterface::listPolicies($policyType, $options)`.
Relevant keys:
- `select` (string|string[]): optional `$select` input
- `expand` (string|string[]): optional `$expand` input
- `filter` (string): optional `$filter`
- `top` (int): optional `$top`
- `platform` (string): optional platform filter for contract-specific endpoints
- plus tenant/auth context from `MicrosoftGraphOptionsResolver`
### Sanitized query (outbound)
`GraphContractRegistry::sanitizeQuery($policyType, $query)` returns:
- `query` (array): sanitized query map, where `$select` and `$expand` are sent as comma-separated strings
- `warnings` (string[]): human-readable warnings for capability safety
### Entra role assignment record
`entraRoleAssignments` list response item is expected to include:
- `roleDefinitionId` (string)
- `principalId` (string)
- `principal` (object, when expanded)
- `displayName` (string)
- `@odata.type` (string)
## State transitions
None. This is a read-only integration-layer capability change.
## Validation rules
- `$expand` values must match `allowed_expand` exactly.
- `$expand` normalization: trim, split on top-level commas only (string input; do not split inside balanced parentheses), drop empty, dedupe.
- Safety caps: max 10 allowed expands; max 200 chars per token.

View File

@ -0,0 +1,127 @@
# Implementation Plan: Graph Contracts — LIST `$expand` Parity Fix
**Branch**: `112-list-expand-parity` | **Date**: 2026-02-25 | **Spec**: `specs/112-list-expand-parity/spec.md`
**Input**: Feature specification from `specs/112-list-expand-parity/spec.md`
## Summary
Bring LIST query capability parity with GET by forwarding caller-provided, contract-allowlisted `$expand` on `GraphClientInterface::listPolicies()`. Improve `$expand` normalization (top-level comma split + trim + dedupe) and enforce safety caps. Fix the Entra Admin Roles report by explicitly requesting `expand=principal` for `entraRoleAssignments`, allowing principal display names to render correctly.
## Technical Context
**Language/Version**: PHP 8.4.x
**Primary Dependencies**: Laravel 12, custom Microsoft Graph integration, Filament v5 (Livewire v4 compliant)
**Storage**: PostgreSQL (Sail) — no schema changes for this feature
**Testing**: Pest v4 (`vendor/bin/sail artisan test --compact`)
**Target Platform**: Web application (Laravel + Filament)
**Project Type**: web
**Performance Goals**: Prevent pathological query option inputs (bounded `$expand`)
**Constraints**: Backwards compatible; no behavior change unless `expand` is explicitly provided
**Scale/Scope**: Tenant-scoped Graph reads; low fan-out but called from reports
## Constitution Check
*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.*
PASS — no violations expected.
- **Read/write separation**: This feature is read-only (query shape change only).
- **Single Contract Path to Graph**: Graph calls remain through `GraphClientInterface`; allowlists remain in `config/graph_contracts.php`.
- **Tenant isolation**: No cross-tenant reads; only changes outbound query parameters for already-tenant-scoped calls.
- **Deterministic capabilities**: Allowlist is exact-match and test-covered; no implicit expansions.
- **Ops/Observability**: No new `OperationRun` usage; diagnostics are logs only and low-noise in production.
Re-check post-design: PASS.
## Project Structure
### Documentation (this feature)
```text
specs/112-list-expand-parity/
├── plan.md
├── research.md
├── data-model.md
├── quickstart.md
├── contracts/
│ └── graph-client-listPolicies-options.schema.json
└── tasks.md
```
### Source Code (repository root)
```text
app/
├── Services/
│ ├── EntraAdminRoles/
│ │ └── EntraAdminRolesReportService.php
│ └── Graph/
│ ├── GraphClientInterface.php
│ ├── GraphContractRegistry.php
│ ├── GraphLogger.php
│ ├── GraphResponse.php
│ └── MicrosoftGraphClient.php
config/
└── graph_contracts.php
tests/
├── Unit/
│ ├── GraphContractRegistryTest.php
│ ├── MicrosoftGraphClientListPoliciesSelectTest.php
│ └── EntraAdminRolesReportServiceTest.php
```
**Structure Decision**: Single Laravel web application. No new packages/modules.
## Phase 0 — Outline & Research (complete)
See `specs/112-list-expand-parity/research.md`.
Key resolved clarifications / decisions:
- `$expand` normalization supports string + array inputs.
- String inputs split on **top-level commas only** (commas inside balanced parentheses are not separators).
- Safety caps: max 10 allowlisted expand tokens; max 200 chars per token.
- Diagnostics: warning in non-prod, debug in prod, structured context.
## Phase 1 — Design & Contracts (complete)
Artifacts:
- `specs/112-list-expand-parity/data-model.md`
- `specs/112-list-expand-parity/contracts/graph-client-listPolicies-options.schema.json`
- `specs/112-list-expand-parity/quickstart.md`
Agent context update:
- Run `.specify/scripts/bash/update-agent-context.sh copilot` after Phase 1 artifacts are current.
## Phase 2 — Implementation Plan (execute next)
1) Update `GraphContractRegistry::sanitizeQuery()`
- Normalize `$expand` input:
- Accept `string|string[]`.
- If string: split on **top-level commas only** (ignore commas inside balanced parentheses); trim whitespace.
- De-dupe tokens; drop empty.
- Enforce `maxTokenLen=200` (drop tokens exceeding the cap).
- Filter via `allowed_expand` exact-match allowlist (no partial matching).
- Enforce `maxItems=10` after allowlist (keep first N allowed).
- Emit diagnostics (warn in non-prod, debug in prod) when tokens are removed/truncated.
2) Update `MicrosoftGraphClient::listPolicies()`
- Accept `expand` in `$options` and forward it into `$queryInput` as `'$expand'`.
- Preserve existing behavior when `expand` is not provided.
3) Fix Entra Admin Roles report
- In `EntraAdminRolesReportService::fetchRoleAssignments()`, pass `expand => 'principal'` alongside resolved tenant graph options.
- Keep the current fallback behavior (`Unknown`) for true upstream missing-data cases.
4) Improve discoverability of list query options
- Expand PHPDoc on `GraphClientInterface::listPolicies()` (and align with `getPolicy()` docs) to describe supported keys and the `expand` input shape.
5) Add regression tests (Pest)
- `GraphContractRegistryTest`: add cases for top-level comma splitting (including commas inside parentheses), dedupe, cap behavior, and non-prod diagnostic log emission.
- `MicrosoftGraphClientListPoliciesSelectTest`: add assertions that an allowlisted LIST `$expand` is present in the outbound query.
- `EntraAdminRolesReportServiceTest`: ensure principal display names are used when returned, and ensure the graph client is invoked with `expand=principal`.
6) Validate
- `vendor/bin/sail bin pint --dirty --format agent`
- Run the focused tests listed in `specs/112-list-expand-parity/quickstart.md`.

View File

@ -0,0 +1,28 @@
# Quickstart — Graph Contracts: LIST `$expand` Parity Fix
## Goal
Verify that LIST requests can forward explicit, allowlisted `$expand`, and that the Entra Admin Roles report requests `principal` so it can render real principal display names.
## Run tests (focused)
From repo root (Sail-first):
- `vendor/bin/sail artisan test --compact tests/Unit/GraphContractRegistryTest.php`
- `vendor/bin/sail artisan test --compact tests/Unit/MicrosoftGraphClientListPoliciesSelectTest.php`
- `vendor/bin/sail artisan test --compact --filter=EntraAdminRoles`
## Manual verification (developer)
- Ensure the `entraRoleAssignments` contract includes `allowed_expand => ['principal']`.
- Ensure the role assignment LIST request includes `$expand=principal` when explicitly requested.
## Usage examples
When listing a policy type where the contract allowlists `principal`:
- `listPolicies('entraRoleAssignments', [..., 'expand' => 'principal'])`
- `listPolicies('entraRoleAssignments', [..., 'expand' => ['principal']])`
Normalization behavior:
- `expand` string input is split on top-level commas only (commas inside balanced parentheses are not separators) and trimmed.
- Values not in the contract allowlist are dropped.
- Max 10 allowlisted expansions are sent; extras are dropped.
## Notes
- No expansions are sent by default. Callers must pass `expand` explicitly.
- Diagnostics are emitted when requested expands are removed or truncated.

View File

@ -0,0 +1,86 @@
# Research — Graph Contracts: LIST `$expand` Parity Fix
## Context
This feature fixes a capability mismatch between LIST vs GET Graph calls:
- `MicrosoftGraphClient::getPolicy()` already supports `$expand` (via query sanitization).
- `MicrosoftGraphClient::listPolicies()` currently does **not** forward `$expand` at all.
Downstream impact: `EntraAdminRolesReportService` lists `entraRoleAssignments` and then reads `principal.displayName`. The contract allowlist permits `principal`, but it is never requested on LIST today, so the report frequently falls back to `"Unknown"`.
## Decisions
### 1) `$expand` input normalization
**Decision**: Accept `expand` as either:
- comma-separated string (e.g. `"principal,roleDefinition($select=id,displayName)"`)
- array of strings (e.g. `["principal", "roleDefinition($select=id,displayName)"]`)
Normalization steps (applied consistently inside `GraphContractRegistry::sanitizeQuery()`):
- Split on **top-level commas only** for string input (commas inside balanced parentheses are not separators).
- Trim whitespace.
- Drop empty tokens.
- Dedupe while preserving first-seen order.
**Rationale**: This matches the spec clarifications and keeps all LIST/GET query-shape logic in the contract registry (single source of truth).
Note: For subquery/select-style expansions, prefer passing an array form (one token per expansion) to avoid ambiguity.
**Alternatives considered**:
- Normalize in `MicrosoftGraphClient` only: rejected because sanitization must remain centralized.
### 2) Capability allowlist enforcement
**Decision**: Keep exact-match allowlist behavior via `array_intersect()`.
**Rationale**: Prevents partial matches and blocks accidental subquery injection. Existing registry behavior is already exact-match.
### 3) Performance/safety caps for `$expand`
**Decision**: Apply a hard cap during normalization:
- `maxItems = 10` allowed expand tokens (keep first 10 allowed after allowlist)
- `maxTokenLen = 200` per token (tokens exceeding are treated as invalid and dropped)
**Rationale**:
- Contract allowlists are small in this repo (most policy types allow 02 expands).
- A small cap prevents pathological caller inputs from bloating URLs and logs.
- `200` aligns with the existing bounded-string pattern used in `InventoryMetaSanitizer::boundedStringList($values, 25, 200)`.
**Alternatives considered**:
- `maxItems = 25` to match `InventoryMetaSanitizer`: rejected as unnecessary for Graph query params.
- Only cap total string length: rejected because token count is easier to reason about and test.
### 4) Diagnostics when sanitize removes/truncates `$expand`
**Decision**: Emit a structured log record when:
- disallowed expand tokens are dropped
- tokens are dropped due to maxTokenLen
- allowed tokens are truncated due to maxItems
Logging behavior:
- Non-production: `Log::warning('Graph query sanitized', [...])`
- Production: `Log::debug('Graph query sanitized', [...])`
Context fields to include:
- `policy_type`
- `query_key` = `'$expand'`
- `requested` (normalized tokens)
- `allowed` (filtered tokens)
- `removed` (dropped tokens)
- `reason` (e.g. `disallowed`, `too_long`, `max_items`)
**Rationale**:
- The spec requires that misuse is detectable in non-production.
- Production should be low-noise.
- This matches existing patterns where services log structured `type`/`reason` context (e.g. inventory dependency extraction warnings).
**Alternatives considered**:
- Rely on `GraphResponse->warnings` only: rejected because `GraphLogger` logs warnings at info level, which can be noisy in production and doesnt guarantee visibility in non-prod.
- Introduce a new event/telemetry system: rejected as out of scope for this fix.
## Testing approach (selected)
- Unit test `GraphContractRegistry::sanitizeQuery()` for `$expand` normalization:
- comma-separated string is split
- disallowed tokens dropped
- max-items behavior keeps first N allowed
- emits `Log::warning` in non-prod when removal occurs
- Unit test `MicrosoftGraphClient::listPolicies()` request shape using `Http::fake()` + `Http::assertSent()`:
- when `expand` is provided and allowlisted, outbound URL includes `%24expand=...`
- Unit/feature test for `EntraAdminRolesReportService`:
- ensures role assignment list call requests `expand=principal`
- ensures payload uses `principal.displayName` when present

View File

@ -0,0 +1,118 @@
# Feature Specification: Graph Contracts — LIST $expand Parity Fix
**Feature Branch**: `112-list-expand-parity`
**Created**: 2026-02-25
**Status**: Draft
**Input**: Platform hardening: make “LIST” query capabilities match “GET” for explicit, allowlisted expansions. Fix Entra Admin Roles report showing deterministic “Unknown” principal names when expanded data is expected.
## Spec Scope Fields *(mandatory)*
- **Scope**: tenant
- **Primary Routes**: Entra Admin Roles report (admin interface)
- **Data Ownership**: No database schema changes. This is an integration-layer query-shape change; contracts are workspace-owned configuration, and fetched directory data is tenant-scoped.
- **RBAC**: No RBAC model changes. Existing access controls for viewing reports remain unchanged.
## Clarifications
### Session 2026-02-25
- Q: What input shape should `expand` accept on LIST query options? → A: Accept `expand` as either comma-separated string or string array (normalize internally).
- Q: When `expand` contains disallowed values, what should happen? → A: Remove disallowed values; emit diagnostic signal (warn in non-production, low-noise debug-level in production).
- Q: What should the performance guard do if callers provide too many expand values / overly long expand input? → A: Keep the first 10 allowed expands (after normalization + allowlist), drop the rest, emit diagnostic. Each token is capped to 200 characters.
## Assumptions & Dependencies
- The upstream directory API supports returning principal details for role assignments when explicitly requested.
- The contract registry remains the single source of truth for which expansions are permitted per policy type.
- The Admin Roles report already has a safe fallback for missing principal details (e.g., showing “Unknown”).
## User Scenarios & Testing *(mandatory)*
### User Story 1 - Admin roles report shows real principal names (Priority: P1)
As an admin, I want the Admin Roles report to display the real person/service principal names for role assignments, so findings are accurate and actionable.
**Why this priority**: The current output can be deterministically wrong (“Unknown”) even when the directory system would provide the principal details, which undermines trust in the report.
**Independent Test**: Fully testable by simulating a tenant role-assignment list retrieval and verifying the report output uses the principal display name when the directory API returns it.
**Acceptance Scenarios**:
1. **Given** the directory API returns role assignments with principal details included, **When** the Admin Roles report is generated, **Then** the report displays the principal display name from the returned data (not “Unknown”).
2. **Given** the directory API returns a role assignment without principal details (true upstream edge case), **When** the report is generated, **Then** the report falls back to “Unknown” for that assignment only.
---
### User Story 2 - Engineers can request explicit LIST expansions safely (Priority: P2)
As a platform engineer, I want list queries to support the same explicit query options as single-item queries (including expansions), so services can request the data shape they need without silent gaps.
**Why this priority**: Without parity, new features can accidentally rely on expanded fields that are never requested on LIST, producing “quietly wrong” data.
**Independent Test**: Testable by inspecting the outgoing list request and confirming expansions are only included when explicitly requested and allowed by the contract.
**Acceptance Scenarios**:
1. **Given** a list request includes an explicit expand value that is allowed by the contract, **When** the outgoing request is constructed, **Then** the request includes that expand value.
2. **Given** a list request includes expand values that are not allowed by the contract, **When** the outgoing request is constructed, **Then** disallowed values are removed and none of them are sent.
---
### User Story 3 - Misuse is detectable in non-production (Priority: P3)
As a maintainer, I want visibility when a caller requests expansions that are removed by sanitization, so I can catch incorrect assumptions early.
**Why this priority**: Sanitization is necessary for safety, but silent removal can hide mistakes during development.
**Independent Test**: Testable by triggering a request with mixed allowed/disallowed expansions and asserting that a non-production warning/diagnostic record is emitted.
**Acceptance Scenarios**:
1. **Given** a request includes at least one disallowed expand value, **When** validation removes it, **Then** a non-production diagnostic signal is emitted containing the policy type and removed values.
### Edge Cases
- Expand is requested but the contract allows no expansions for that policy type → validation removes all values → no expansion is sent.
- Multiple expand values are provided (string or list) → values are trimmed, split on top-level commas (for string), de-duplicated, and normalized.
- Values that include subquery/select syntax must match an allowed value exactly (no partial matches).
## Requirements *(mandatory)*
**Constitution alignment (required):** This feature affects outbound directory API query shapes. It must maintain tenant isolation (no cross-tenant leakage), keep contract registry allowlists authoritative, and add regression tests that prevent query capability divergence.
This feature does not introduce write/change behavior and does not add long-running/queued/scheduled work.
### Functional Requirements
- **FR-112-001 — LIST supports explicit expansions**: List retrieval MUST accept an optional expand input (either comma-separated string or list of strings), normalize it (trim, split on top-level commas for string input, de-duplicate), and forward the resulting allowed expand values to the directory API as a query option. Commas inside balanced parentheses (e.g., subquery/select-style expansions) MUST NOT be treated as separators.
- **FR-112-002 — Explicit-only behavior**: Expansions MUST be sent only when explicitly provided by the caller. Default behavior remains: no expansions.
- **FR-112-003 — Contract allowlist validation**: Expand values MUST be validated against the contract allowlist for the policy type. Disallowed/invalid values MUST NOT be sent.
- **FR-112-004 — Exact-match values**: Allowed expand values are matched as exact values; subquery/select-style expansions are allowed only if the exact value is allowlisted.
- **FR-112-005 — Entra Admin Roles bugfix**: The Admin Roles report MUST explicitly request the principal expansion when listing Entra role assignments so principal display names are available when the upstream system provides them.
- **FR-112-006 — Query options are discoverable**: The supported query option shape for list retrieval (at minimum: select, expand, filter, top, platform) MUST be documented where implementers naturally discover it (e.g., developer-facing docs on the client interface).
### Non-Functional Requirements (Enterprise)
- **NFR-112-001 — Backwards compatible**: Existing call sites that do not pass expansions MUST behave exactly as before.
- **NFR-112-002 — Performance guard**: The system MUST NOT auto-apply expansions from contracts. It MUST apply a hard cap to expand input size. The cap is: at most 10 allowlisted expand tokens, and at most 200 characters per token. If the caller provides more than the cap, the system MUST keep the first 10 allowed expand values (after normalization + allowlist), drop the rest, and emit a diagnostic signal.
- **NFR-112-003 — Observability without spam**: When validation removes disallowed expand values, the system MUST emit a diagnostic signal in non-production environments. In production it MUST be low-noise (debug-level) to avoid log spam.
### Key Entities *(include if feature involves data)*
- **Directory Contract**: A per-policy-type definition of allowed query options, including expansion allowlists.
- **Query Options**: The caller-provided set of optional query inputs for list retrieval (select, expand, filter, top, platform).
- **Entra Role Assignment**: A role assignment record included in the Admin Roles report.
- **Principal**: The expanded identity object associated with a role assignment, including the display name.
## Success Criteria *(mandatory)*
### Measurable Outcomes
- **SC-112-001**: In automated tests, when the upstream response contains a principal display name, the Admin Roles report never renders “Unknown” for those records.
- **SC-112-002**: In the 7 days after release, the share of role-assignment rows shown as “Unknown” is < 1% in normal operation, excluding upstream missing-data cases.
- **SC-112-003**: Automated tests prove that callers who do not request expansions observe no change in outbound list request shape.
### Verification (Regression Guards)
- Automated tests verify that list requests forward allowed expand values and never forward disallowed values.
- Automated tests verify that the Admin Roles report explicitly requests the principal expansion when listing role assignments.

View File

@ -0,0 +1,149 @@
---
description: "Task list for Graph Contracts — LIST $expand Parity Fix"
---
# Tasks: Graph Contracts — LIST `$expand` Parity Fix
**Input**: Design documents from `specs/112-list-expand-parity/`
**Prerequisites**: plan.md (required), spec.md (required for user stories), research.md, data-model.md, contracts/
**Tests**: REQUIRED (Pest) — this feature changes runtime Graph request shapes.
## Phase 1: Setup (Shared)
**Purpose**: Confirm inputs and establish a baseline before changing runtime behavior.
- [X] T001 [Shared] Read implementation plan in specs/112-list-expand-parity/plan.md
- [X] T002 [P] [Shared] Verify $expand allowlist exists for Entra role assignments in config/graph_contracts.php
- [X] T003 [P] [Shared] Identify existing Graph tests to extend in tests/Unit/GraphContractRegistryTest.php and tests/Unit/MicrosoftGraphClientListPoliciesSelectTest.php
---
## Phase 2: Foundational (Blocking Prerequisites)
**Purpose**: Establish pre-change confidence and shared test baseline.
- [X] T004 [Shared] Run baseline focused tests listed in specs/112-list-expand-parity/quickstart.md
**Checkpoint**: Baseline established; safe to implement.
---
## Phase 3: User Story 1 — Admin roles report shows real principal names (Priority: P1) 🎯 MVP
**Goal**: Admin Roles report requests `principal` expansion and renders real principal display names when Graph provides them.
**Independent Test**: Simulate role assignment list retrieval and assert report output uses `principal.displayName` when present, else falls back to `Unknown`.
### Tests for User Story 1
- [X] T005 [P] [US1] Create report service test scaffolding in tests/Unit/EntraAdminRolesReportServiceTest.php (RefreshDatabase + mock GraphClientInterface)
- [X] T006 [US1] Implement tests asserting `entraRoleAssignments` call includes `expand => 'principal'` and payload uses principal display name in tests/Unit/EntraAdminRolesReportServiceTest.php
### Implementation for User Story 1
- [X] T007 [US1] Pass `expand => 'principal'` when fetching assignments in app/Services/EntraAdminRoles/EntraAdminRolesReportService.php
- [X] T008 [US1] Forward caller-provided `expand` into LIST query input in app/Services/Graph/MicrosoftGraphClient.php
- [X] T009 [US1] Normalize + cap `$expand` in app/Services/Graph/GraphContractRegistry.php (split string on top-level commas only; preserve commas inside balanced parentheses; trim, drop empty, dedupe, maxTokenLen=200, maxItems=10 after allowlist)
### Regression tests for User Story 1
- [X] T010 [US1] Add LIST `$expand` request-shape test coverage in tests/Unit/MicrosoftGraphClientListPoliciesSelectTest.php
- [X] T011 [US1] Add `$expand` normalization/cap test coverage in tests/Unit/GraphContractRegistryTest.php
- [X] T012 [US1] Run story tests in tests/Unit/EntraAdminRolesReportServiceTest.php plus Graph tests in tests/Unit/MicrosoftGraphClientListPoliciesSelectTest.php
**Checkpoint**: Entra Admin Roles report can render real principal names when upstream provides them.
---
## Phase 4: User Story 2 — Engineers can request explicit LIST expansions safely (Priority: P2)
**Goal**: LIST supports explicit expansions safely and discoverably (document option shape, allowlist enforcement).
**Independent Test**: Inspect outgoing LIST request and confirm `$expand` is included only when explicitly provided and allowlisted.
### Tests for User Story 2
- [X] T013 [P] [US2] Add test asserting LIST omits `$expand` when `expand` is not provided in tests/Unit/MicrosoftGraphClientListPoliciesSelectTest.php
- [X] T014 [P] [US2] Add tests asserting (a) disallowed expand tokens are removed (exact-match allowlist, no partial matches) and (b) allowlisted subquery/select-style expand tokens that contain commas inside parentheses are preserved as a single token (no naive comma-splitting) in tests/Unit/GraphContractRegistryTest.php
### Implementation for User Story 2
- [X] T015 [US2] Document supported `listPolicies()` options (select/expand/filter/top/platform + input shapes) in app/Services/Graph/GraphClientInterface.php
- [X] T016 [US2] Ensure `GraphContractCheck` query shape remains compatible (select + expand) in app/Console/Commands/GraphContractCheck.php
**Checkpoint**: Engineers can safely request allowlisted expansions on LIST without silent capability mismatch.
---
## Phase 5: User Story 3 — Misuse is detectable in non-production (Priority: P3)
**Goal**: When `$expand` is sanitized (removed or truncated), maintainers get a diagnostic signal in non-production with low noise in production.
**Independent Test**: Trigger mixed allowed/disallowed expand and assert a non-production warning is emitted with policy type + removed values.
### Tests for User Story 3
- [X] T017 [US3] Add Log::spy-based test for non-production diagnostics when `$expand` is sanitized in tests/Unit/GraphContractRegistryTest.php
- [X] T018 [US3] Add production-noise guard test (debug-level or suppressed warn) for `$expand` sanitization in tests/Unit/GraphContractRegistryTest.php
### Implementation for User Story 3
- [X] T019 [US3] Emit structured diagnostic logs when `$expand` is removed/truncated in app/Services/Graph/GraphContractRegistry.php
**Checkpoint**: Sanitization is visible during dev/staging without spamming production logs.
---
## Phase 6: Polish & Cross-Cutting Concerns
**Purpose**: Keep the change safe, consistent, and shippable.
- [X] T020 [Shared] Run formatting on touched files (Pint) after changes in app/Services/Graph/GraphContractRegistry.php
- [X] T021 [Shared] Run focused suite listed in specs/112-list-expand-parity/quickstart.md
---
## Dependencies & Execution Order
### Phase Dependencies
- Setup (Phase 1) → Foundational (Phase 2) → US1 (Phase 3) → US2 (Phase 4) → US3 (Phase 5) → Polish (Phase 6)
### User Story Dependencies
- US1 depends on LIST `$expand` parity (implemented as part of US1 via app/Services/Graph/MicrosoftGraphClient.php + app/Services/Graph/GraphContractRegistry.php).
- US2 extends US1 with discoverability + additional safety regression coverage.
- US3 depends on the sanitization logic (adds diagnostics + tests).
### Parallel Opportunities
- Phase 1: T002 and T003 can run in parallel.
- US1: T005 can be done in parallel with T008/T009 (different files).
- US2: T013 and T014 can run in parallel.
---
## Parallel Example: User Story 1
- In parallel:
- T005 (test scaffolding) in tests/Unit/EntraAdminRolesReportServiceTest.php
- T008 (LIST forwards expand) in app/Services/Graph/MicrosoftGraphClient.php
- T009 (sanitize/normalize/cap expand) in app/Services/Graph/GraphContractRegistry.php
---
## Implementation Strategy
### MVP First (User Story 1 Only)
1. Complete Phase 12
2. Complete US1 (Phase 3)
3. Stop and ship if acceptance + regression tests pass
### Then harden (US2 + US3)
- Add discoverability + extra regression guards (US2)
- Add diagnostics behavior + tests (US3)

View File

@ -0,0 +1,118 @@
<?php
declare(strict_types=1);
use App\Models\Tenant;
use App\Services\EntraAdminRoles\EntraAdminRolesReportService;
use App\Services\Graph\GraphClientInterface;
use App\Services\Graph\GraphResponse;
use Illuminate\Foundation\Testing\RefreshDatabase;
uses(RefreshDatabase::class);
it('requests principal expansion and uses principal display name when present', function () {
$tenant = Tenant::factory()->create();
ensureDefaultProviderConnection($tenant, 'microsoft');
$roleDefinitions = [
[
'id' => 'role-def-1',
'templateId' => '62e90394-69f5-4237-9190-012177145e10',
'displayName' => 'Global Administrator',
'isBuiltIn' => true,
],
];
$roleAssignments = [
[
'id' => 'role-assign-1',
'roleDefinitionId' => 'role-def-1',
'principalId' => 'principal-1',
'directoryScopeId' => '/',
'principal' => [
'@odata.type' => '#microsoft.graph.user',
'displayName' => 'Ada Lovelace',
],
],
];
$calls = [];
$this->mock(GraphClientInterface::class, function ($mock) use (&$calls, $roleDefinitions, $roleAssignments) {
$mock->shouldReceive('listPolicies')
->twice()
->andReturnUsing(function (string $policyType, array $options) use (&$calls, $roleDefinitions, $roleAssignments): GraphResponse {
$calls[] = [$policyType, $options];
if ($policyType === 'entraRoleDefinitions') {
return new GraphResponse(true, $roleDefinitions);
}
return new GraphResponse(true, $roleAssignments);
});
});
$result = app(EntraAdminRolesReportService::class)->generate($tenant);
expect($calls)->toHaveCount(2);
expect($calls[0][0])->toBe('entraRoleDefinitions');
expect($calls[0][1])->not->toHaveKey('expand');
expect($calls[1][0])->toBe('entraRoleAssignments');
expect($calls[1][1]['expand'] ?? null)->toBe('principal');
expect($calls[1][1])->toMatchArray(array_merge($calls[0][1], [
'expand' => 'principal',
]));
expect($result->payload['high_privilege'])->toHaveCount(1);
expect($result->payload['high_privilege'][0]['principal_display_name'])->toBe('Ada Lovelace');
});
it('falls back to Unknown when principal details are missing upstream', function () {
$tenant = Tenant::factory()->create();
ensureDefaultProviderConnection($tenant, 'microsoft');
$roleDefinitions = [
[
'id' => 'role-def-1',
'templateId' => '62e90394-69f5-4237-9190-012177145e10',
'displayName' => 'Global Administrator',
'isBuiltIn' => true,
],
];
$roleAssignments = [
[
'id' => 'role-assign-1',
'roleDefinitionId' => 'role-def-1',
'principalId' => 'principal-1',
'directoryScopeId' => '/',
],
];
$calls = [];
$this->mock(GraphClientInterface::class, function ($mock) use (&$calls, $roleDefinitions, $roleAssignments) {
$mock->shouldReceive('listPolicies')
->twice()
->andReturnUsing(function (string $policyType, array $options) use (&$calls, $roleDefinitions, $roleAssignments): GraphResponse {
$calls[] = [$policyType, $options];
if ($policyType === 'entraRoleDefinitions') {
return new GraphResponse(true, $roleDefinitions);
}
return new GraphResponse(true, $roleAssignments);
});
});
$result = app(EntraAdminRolesReportService::class)->generate($tenant);
expect($calls)->toHaveCount(2);
expect($calls[0][0])->toBe('entraRoleDefinitions');
expect($calls[0][1])->not->toHaveKey('expand');
expect($calls[1][0])->toBe('entraRoleAssignments');
expect($calls[1][1]['expand'] ?? null)->toBe('principal');
expect($result->payload['high_privilege'])->toHaveCount(1);
expect($result->payload['high_privilege'][0]['principal_display_name'])->toBe('Unknown');
});

View File

@ -2,12 +2,13 @@
use App\Services\Graph\GraphContractRegistry;
use App\Services\Graph\GraphResponse;
use Illuminate\Support\Facades\Log;
beforeEach(function () {
config()->set('graph_contracts.types.deviceConfiguration', [
'resource' => 'deviceManagement/deviceConfigurations',
'allowed_select' => ['id', 'displayName'],
'allowed_expand' => ['assignments'],
'allowed_expand' => ['assignments', 'roleDefinition($select=id,displayName)'],
'type_family' => [
'#microsoft.graph.deviceConfiguration',
'#microsoft.graph.windows10CustomConfiguration',
@ -44,6 +45,92 @@
expect($warnings)->not->toBeEmpty();
});
it('splits $expand strings on top-level commas and preserves commas inside parentheses', function () {
$result = $this->registry->sanitizeQuery('deviceConfiguration', [
'$expand' => 'assignments, roleDefinition($select=id,displayName)',
]);
expect($result['query']['$expand'])->toBe('assignments,roleDefinition($select=id,displayName)');
});
it('dedupes and drops empty $expand tokens', function () {
$result = $this->registry->sanitizeQuery('deviceConfiguration', [
'$expand' => 'assignments, assignments,,',
]);
expect($result['query']['$expand'])->toBe('assignments');
});
it('caps $expand token length and maximum allowed items', function () {
$longToken = str_repeat('a', 201);
$allowedTokens = [];
foreach (range(1, 11) as $number) {
$allowedTokens[] = "token{$number}";
}
config()->set('graph_contracts.types.deviceConfiguration.allowed_expand', array_merge(['assignments', $longToken], $allowedTokens));
$result = $this->registry->sanitizeQuery('deviceConfiguration', [
'$expand' => array_merge([$longToken, 'assignments'], $allowedTokens),
]);
expect($result['query']['$expand'])->toBe(implode(',', array_merge(['assignments'], array_slice($allowedTokens, 0, 9))));
});
it('drops disallowed $expand tokens with exact-match allowlists', function () {
$result = $this->registry->sanitizeQuery('entraRoleAssignments', [
'$expand' => 'principal,principal($select=displayName)',
]);
expect($result['query']['$expand'])->toBe('principal');
});
it('emits non-production diagnostics when $expand is sanitized', function () {
Log::spy();
$result = $this->registry->sanitizeQuery('entraRoleAssignments', [
'$expand' => 'principal,badExpand',
]);
expect($result['query']['$expand'])->toBe('principal');
Log::shouldHaveReceived('warning')
->once()
->withArgs(function (string $message, array $context): bool {
return $message === 'Graph query sanitized'
&& ($context['policy_type'] ?? null) === 'entraRoleAssignments'
&& ($context['query_key'] ?? null) === '$expand'
&& in_array('badExpand', $context['removed'] ?? [], true);
});
});
it('uses debug-level diagnostics in production when $expand is sanitized', function () {
Log::spy();
$originalEnvironment = app()->environment();
app()->detectEnvironment(fn () => 'production');
$result = $this->registry->sanitizeQuery('entraRoleAssignments', [
'$expand' => 'principal,badExpand',
]);
expect($result['query']['$expand'])->toBe('principal');
Log::shouldHaveReceived('debug')
->once()
->withArgs(function (string $message, array $context): bool {
return $message === 'Graph query sanitized'
&& ($context['policy_type'] ?? null) === 'entraRoleAssignments'
&& ($context['query_key'] ?? null) === '$expand'
&& in_array('badExpand', $context['removed'] ?? [], true);
});
Log::shouldNotHaveReceived('warning');
app()->detectEnvironment(fn () => $originalEnvironment);
});
it('matches derived types within family', function () {
expect($this->registry->matchesTypeFamily('deviceConfiguration', '#microsoft.graph.windows10CustomConfiguration'))->toBeTrue();
expect($this->registry->matchesTypeFamily('deviceConfiguration', '#microsoft.graph.androidCompliancePolicy'))->toBeFalse();

View File

@ -156,3 +156,70 @@
expect($firstRequest->url())->toContain('/deviceManagement/configurationPolicies');
expect($secondRequest->url())->toBe($nextLink);
});
it('forwards allowlisted $expand when explicitly provided', function () {
Http::fake([
'graph.microsoft.com/*' => Http::response(['value' => []], 200),
]);
$logger = mock(GraphLogger::class);
$logger->shouldReceive('logRequest')->zeroOrMoreTimes()->andReturnNull();
$logger->shouldReceive('logResponse')->zeroOrMoreTimes()->andReturnNull();
$client = new MicrosoftGraphClient(
logger: $logger,
contracts: app(GraphContractRegistry::class),
);
$client->listPolicies('entraRoleAssignments', [
'access_token' => 'test-token',
'expand' => 'principal',
]);
Http::assertSent(function (Request $request) {
$url = $request->url();
if (! str_contains($url, '/roleManagement/directory/roleAssignments')) {
return false;
}
parse_str((string) parse_url($url, PHP_URL_QUERY), $query);
expect($query['$expand'] ?? null)->toBe('principal');
return true;
});
});
it('omits $expand when not explicitly provided', function () {
Http::fake([
'graph.microsoft.com/*' => Http::response(['value' => []], 200),
]);
$logger = mock(GraphLogger::class);
$logger->shouldReceive('logRequest')->zeroOrMoreTimes()->andReturnNull();
$logger->shouldReceive('logResponse')->zeroOrMoreTimes()->andReturnNull();
$client = new MicrosoftGraphClient(
logger: $logger,
contracts: app(GraphContractRegistry::class),
);
$client->listPolicies('entraRoleAssignments', [
'access_token' => 'test-token',
]);
Http::assertSent(function (Request $request) {
$url = $request->url();
if (! str_contains($url, '/roleManagement/directory/roleAssignments')) {
return false;
}
parse_str((string) parse_url($url, PHP_URL_QUERY), $query);
expect($query)->not->toHaveKey('$expand');
return true;
});
});