docs(phase-17): research group expansion in HTML reports
This commit is contained in:
Dev
559
.planning/phases/17-group-expansion-html-reports/17-RESEARCH.md
Normal file
559
.planning/phases/17-group-expansion-html-reports/17-RESEARCH.md
Normal file
@@ -0,0 +1,559 @@
|
|||||||
|
# Phase 17: Group Expansion in HTML Reports - Research
|
||||||
|
|
||||||
|
**Researched:** 2026-04-09
|
||||||
|
**Domain:** SharePoint CSOM group member resolution / HTML export rendering / async pre-resolution service
|
||||||
|
**Confidence:** HIGH
|
||||||
|
|
||||||
|
## Summary
|
||||||
|
|
||||||
|
Phase 17 adds expandable SharePoint group rows to the site-centric permissions HTML report (`HtmlExportService`). In the existing report, a SharePoint group appears as an opaque user-pill in the "Users/Groups" column (rendered from `PermissionEntry.Users`, `PrincipalType = "SharePointGroup"`). Phase 17 makes that pill clickable — clicking expands a hidden sub-row listing the group's members.
|
||||||
|
|
||||||
|
Member resolution is a pre-render step: the ViewModel resolves all group members via CSOM before calling `BuildHtml`, then passes a resolution dictionary into the service. The HTML service itself remains pure (no async I/O). Transitive membership is achieved by recursively resolving any AAD group members found in a SharePoint group via Graph `groups/{id}/transitiveMembers`. When CSOM or Graph cannot resolve members (throttling, insufficient scope), the group pill renders with a "members unavailable" label rather than failing the export.
|
||||||
|
|
||||||
|
The user access audit report (`UserAccessHtmlExportService`) is NOT in scope: that report is user-centric, and groups appear only as `GrantedThrough` text — there are no expandable group rows to add there.
|
||||||
|
|
||||||
|
**Primary recommendation:** Create a new `ISharePointGroupResolver` service that uses CSOM (`ClientContext.Web.SiteGroups.GetByName(name).Users`) plus optional Graph transitive lookup for nested AAD groups. The ViewModel calls this service before export and passes `IReadOnlyDictionary<string, IReadOnlyList<string>> groupMembers` into `HtmlExportService.BuildHtml`. The HTML uses the existing `toggleGroup()` JS pattern with a new `grpmem{idx}` ID namespace for member sub-rows.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
<phase_requirements>
|
||||||
|
## Phase Requirements
|
||||||
|
|
||||||
|
| ID | Description | Research Support |
|
||||||
|
|----|-------------|-----------------|
|
||||||
|
| RPT-01 | User can expand SharePoint groups in HTML reports to see group members | Group pill in site-centric `HtmlExportService` becomes clickable; hidden sub-rows per member revealed via `toggleGroup()`; member data passed as pre-resolved dictionary from ViewModel |
|
||||||
|
| RPT-02 | Group member resolution uses transitive membership to include nested group members | CSOM resolves direct SharePoint group users; for user entries that are AAD groups (login matches `c:0t.c|tenant|` prefix), Graph `groups/{id}/transitiveMembers/microsoft.graph.user` is called recursively; leaf users merged into final member list |
|
||||||
|
</phase_requirements>
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Standard Stack
|
||||||
|
|
||||||
|
### Core
|
||||||
|
| Library | Version | Purpose | Why Standard |
|
||||||
|
|---------|---------|---------|--------------|
|
||||||
|
| Microsoft.SharePoint.Client (CSOM) | Already referenced | `ctx.Web.SiteGroups.GetByName(name).Users` — only supported API for classic SharePoint group members | Graph v1.0 and beta DO NOT support classic SharePoint group membership enumeration; CSOM is the only supported path |
|
||||||
|
| Microsoft.Graph (Graph SDK) | Already referenced | `graphClient.Groups[aadGroupId].TransitiveMembers.GraphUser.GetAsync()` — resolves nested AAD groups transitively | Existing Graph SDK usage throughout codebase (`GraphUserDirectoryService`, `GraphUserSearchService`, `BulkMemberService`) |
|
||||||
|
| `GraphClientFactory` | Project-internal | Creates `GraphServiceClient` with MSAL tokens | Already used in all Graph-calling services; same pattern |
|
||||||
|
| `ExecuteQueryRetryHelper` | Project-internal | CSOM retry with throttle handling | Already used in `PermissionsService`, `BulkMemberService`; handles 429 and 503 |
|
||||||
|
|
||||||
|
### No New NuGet Packages
|
||||||
|
Phase 17 requires zero new NuGet packages. All required APIs are already referenced.
|
||||||
|
|
||||||
|
### Critical API Finding: SharePoint Classic Groups vs Graph
|
||||||
|
**SharePoint classic groups** (e.g. "Site Members", "HR Team") are managed by SharePoint, not Azure AD. Microsoft Graph v1.0 has no endpoint for their membership. The Graph beta `sharePointGroup` resource is for SharePoint Embedded containers only — not for classic on-site groups. The only supported enumeration path is CSOM:
|
||||||
|
|
||||||
|
```csharp
|
||||||
|
// Source: Microsoft CSOM documentation (learn.microsoft.com/sharepoint/dev)
|
||||||
|
var group = ctx.Web.SiteGroups.GetByName("Site Members");
|
||||||
|
ctx.Load(group.Users);
|
||||||
|
await ExecuteQueryRetryHelper.ExecuteQueryRetryAsync(ctx, progress, ct);
|
||||||
|
foreach (var user in group.Users) { ... }
|
||||||
|
```
|
||||||
|
|
||||||
|
A SharePoint group member can itself be an AAD/M365 group (login contains `c:0t.c|tenant|<guid>` pattern). For these nested AAD groups, Graph `transitiveMembers` resolves all leaf users in a single call — satisfying RPT-02.
|
||||||
|
|
||||||
|
## Architecture Patterns
|
||||||
|
|
||||||
|
### Recommended Project Structure Changes
|
||||||
|
|
||||||
|
```
|
||||||
|
SharepointToolbox/
|
||||||
|
├── Services/
|
||||||
|
│ ├── ISharePointGroupResolver.cs ← NEW interface
|
||||||
|
│ └── SharePointGroupResolver.cs ← NEW service
|
||||||
|
├── Services/Export/
|
||||||
|
│ └── HtmlExportService.cs ← BuildHtml overload + group pill rendering
|
||||||
|
└── ViewModels/Tabs/
|
||||||
|
└── PermissionsViewModel.cs ← call resolver before export, pass dict
|
||||||
|
|
||||||
|
SharepointToolbox.Tests/
|
||||||
|
└── Services/
|
||||||
|
├── SharePointGroupResolverTests.cs ← NEW (mostly skip-marked, requires live tenant)
|
||||||
|
└── Export/
|
||||||
|
└── HtmlExportServiceTests.cs ← extend: group pill expansion tests
|
||||||
|
```
|
||||||
|
|
||||||
|
### Pattern 1: Pre-Resolution Architecture (ViewModel orchestrates, service stays pure)
|
||||||
|
|
||||||
|
**What:** ViewModel calls `ISharePointGroupResolver.ResolveGroupsAsync()` to build a `Dictionary<string, IReadOnlyList<string>> groupMembers` keyed by group name (exact match with `PermissionEntry.Users` value for SharePoint group rows). This dict is passed to `BuildHtml`. HTML export service remains synchronous and pure — no async I/O inside string building.
|
||||||
|
|
||||||
|
**Why:** Consistent with how `BrandingService` provides `ReportBranding` to the HTML service — the ViewModel fetches context, export service renders it. Avoids making export services async or injecting CSOM/Graph into what are currently pure string-building classes.
|
||||||
|
|
||||||
|
**Data flow:**
|
||||||
|
```
|
||||||
|
PermissionsViewModel.ExportHtmlAsync()
|
||||||
|
→ collect group names from Results where PrincipalType == "SharePointGroup"
|
||||||
|
→ ISharePointGroupResolver.ResolveGroupsAsync(ctx, clientId, groupNames, ct)
|
||||||
|
→ per group: CSOM SiteGroups.GetByName(name).Users → List<MemberInfo>
|
||||||
|
→ per nested AAD group: Graph transitiveMembers → leaf users
|
||||||
|
→ on throttle/error: return empty list (graceful fallback)
|
||||||
|
→ HtmlExportService.BuildHtml(entries, groupMembers, branding)
|
||||||
|
→ for group pills: if groupMembers contains name → render expandable pill + hidden sub-rows
|
||||||
|
→ else if name not in dict → render plain pill (no expand)
|
||||||
|
→ if dict has empty list → render pill + "members unavailable" sub-row
|
||||||
|
```
|
||||||
|
|
||||||
|
### Pattern 2: ISharePointGroupResolver Interface
|
||||||
|
|
||||||
|
**What:** A focused single-method async service that accepts a `ClientContext` (for CSOM) and `clientId` string (for Graph factory), resolves a set of group names to their transitive leaf-user members.
|
||||||
|
|
||||||
|
```csharp
|
||||||
|
// NEW: Services/ISharePointGroupResolver.cs
|
||||||
|
public interface ISharePointGroupResolver
|
||||||
|
{
|
||||||
|
/// <summary>
|
||||||
|
/// Resolves SharePoint group names to their transitive member display names.
|
||||||
|
/// Returns empty list for any group that cannot be resolved (throttled, not found, etc.).
|
||||||
|
/// Never throws — failures are surfaced as empty member lists.
|
||||||
|
/// </summary>
|
||||||
|
Task<IReadOnlyDictionary<string, IReadOnlyList<ResolvedMember>>> ResolveGroupsAsync(
|
||||||
|
ClientContext ctx,
|
||||||
|
string clientId,
|
||||||
|
IReadOnlyList<string> groupNames,
|
||||||
|
CancellationToken ct);
|
||||||
|
}
|
||||||
|
|
||||||
|
// Simple value record for a resolved leaf member
|
||||||
|
public record ResolvedMember(string DisplayName, string Login);
|
||||||
|
```
|
||||||
|
|
||||||
|
**Why a dict keyed by group name:** The `PermissionEntry.Users` field for a SharePoint group row IS the group display name (set to `member.Title` in `PermissionsService.ExtractPermissionsAsync`). The dict lookup is O(1) at render time.
|
||||||
|
|
||||||
|
### Pattern 3: SharePointGroupResolver — CSOM + Graph Resolution
|
||||||
|
|
||||||
|
**What:** CSOM loads group users. Each user login is inspected for the AAD group prefix pattern (`c:0t.c|tenant|<guid>`). For matching entries, the GUID is extracted and used to call `graphClient.Groups[guid].TransitiveMembers.GraphUser.GetAsync()`. All leaf users merged and de-duplicated.
|
||||||
|
|
||||||
|
```csharp
|
||||||
|
// Source: CSOM pattern from Microsoft docs + existing ExecuteQueryRetryHelper usage
|
||||||
|
public async Task<IReadOnlyDictionary<string, IReadOnlyList<ResolvedMember>>>
|
||||||
|
ResolveGroupsAsync(ClientContext ctx, string clientId,
|
||||||
|
IReadOnlyList<string> groupNames, CancellationToken ct)
|
||||||
|
{
|
||||||
|
var result = new Dictionary<string, IReadOnlyList<ResolvedMember>>(
|
||||||
|
StringComparer.OrdinalIgnoreCase);
|
||||||
|
var graphClient = await _graphClientFactory.CreateClientAsync(clientId, ct);
|
||||||
|
|
||||||
|
foreach (var groupName in groupNames.Distinct(StringComparer.OrdinalIgnoreCase))
|
||||||
|
{
|
||||||
|
ct.ThrowIfCancellationRequested();
|
||||||
|
try
|
||||||
|
{
|
||||||
|
var group = ctx.Web.SiteGroups.GetByName(groupName);
|
||||||
|
ctx.Load(group.Users, users => users.Include(
|
||||||
|
u => u.Title, u => u.LoginName, u => u.PrincipalType));
|
||||||
|
await ExecuteQueryRetryHelper.ExecuteQueryRetryAsync(ctx, null, ct);
|
||||||
|
|
||||||
|
var members = new List<ResolvedMember>();
|
||||||
|
foreach (var user in group.Users)
|
||||||
|
{
|
||||||
|
if (IsAadGroup(user.LoginName))
|
||||||
|
{
|
||||||
|
// Expand nested AAD group transitively via Graph
|
||||||
|
var aadId = ExtractAadGroupId(user.LoginName);
|
||||||
|
var leafUsers = await ResolveAadGroupAsync(graphClient, aadId, ct);
|
||||||
|
members.AddRange(leafUsers);
|
||||||
|
}
|
||||||
|
else
|
||||||
|
{
|
||||||
|
members.Add(new ResolvedMember(
|
||||||
|
user.Title ?? user.LoginName,
|
||||||
|
StripClaims(user.LoginName)));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
result[groupName] = members
|
||||||
|
.DistinctBy(m => m.Login, StringComparer.OrdinalIgnoreCase)
|
||||||
|
.ToList();
|
||||||
|
}
|
||||||
|
catch (Exception ex)
|
||||||
|
{
|
||||||
|
Log.Warning("Could not resolve SP group '{Group}': {Error}", groupName, ex.Message);
|
||||||
|
result[groupName] = Array.Empty<ResolvedMember>(); // graceful fallback
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return result;
|
||||||
|
}
|
||||||
|
|
||||||
|
// AAD group login pattern: "c:0t.c|tenant|<guid>"
|
||||||
|
private static bool IsAadGroup(string login) =>
|
||||||
|
login.StartsWith("c:0t.c|", StringComparison.OrdinalIgnoreCase);
|
||||||
|
|
||||||
|
private static string ExtractAadGroupId(string login) =>
|
||||||
|
login[(login.LastIndexOf('|') + 1)..];
|
||||||
|
```
|
||||||
|
|
||||||
|
### Pattern 4: Graph Transitive Members for Nested AAD Groups
|
||||||
|
|
||||||
|
**What:** Call `graphClient.Groups[aadGroupId].TransitiveMembers.GraphUser.GetAsync()` to get all leaf users recursively in one call. The `/microsoft.graph.user` cast filters to only user objects (excludes sub-groups, devices, etc.).
|
||||||
|
|
||||||
|
```csharp
|
||||||
|
// Source: Graph SDK pattern — consistent with GraphUserDirectoryService.cs PageIterator usage
|
||||||
|
private static async Task<IEnumerable<ResolvedMember>> ResolveAadGroupAsync(
|
||||||
|
GraphServiceClient graphClient, string aadGroupId, CancellationToken ct)
|
||||||
|
{
|
||||||
|
try
|
||||||
|
{
|
||||||
|
var response = await graphClient
|
||||||
|
.Groups[aadGroupId]
|
||||||
|
.TransitiveMembers
|
||||||
|
.GraphUser
|
||||||
|
.GetAsync(config =>
|
||||||
|
{
|
||||||
|
config.QueryParameters.Select = new[] { "displayName", "userPrincipalName" };
|
||||||
|
config.QueryParameters.Top = 999;
|
||||||
|
}, ct);
|
||||||
|
|
||||||
|
if (response?.Value is null) return Enumerable.Empty<ResolvedMember>();
|
||||||
|
|
||||||
|
var members = new List<ResolvedMember>();
|
||||||
|
var pageIterator = PageIterator<User, UserCollectionResponse>.CreatePageIterator(
|
||||||
|
graphClient, response,
|
||||||
|
user =>
|
||||||
|
{
|
||||||
|
if (ct.IsCancellationRequested) return false;
|
||||||
|
members.Add(new ResolvedMember(
|
||||||
|
user.DisplayName ?? user.UserPrincipalName ?? "Unknown",
|
||||||
|
user.UserPrincipalName ?? string.Empty));
|
||||||
|
return true;
|
||||||
|
});
|
||||||
|
await pageIterator.IterateAsync(ct);
|
||||||
|
return members;
|
||||||
|
}
|
||||||
|
catch (Exception ex)
|
||||||
|
{
|
||||||
|
Log.Warning("Could not resolve AAD group '{Id}' transitively: {Error}", aadGroupId, ex.Message);
|
||||||
|
return Enumerable.Empty<ResolvedMember>(); // graceful fallback
|
||||||
|
}
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
### Pattern 5: HTML Rendering — Expandable Group Pill
|
||||||
|
|
||||||
|
**What:** When a `PermissionEntry` has `PrincipalType = "SharePointGroup"` and the group name is in `groupMembers`:
|
||||||
|
- If members list is non-empty: render clickable span with toggle + hidden member sub-rows
|
||||||
|
- If members list is empty (resolution failed): render span + "members unavailable" sub-row
|
||||||
|
- If group name NOT in dict (resolver wasn't called, or group was skipped): render plain pill (existing behavior)
|
||||||
|
|
||||||
|
Uses the SAME `toggleGroup()` JS function already in both `BuildHtml` (standard) and `BuildConsolidatedHtml`. New ID namespace: `grpmem{idx}` (distinct from `ugrp`, `sgrp`, `loc`).
|
||||||
|
|
||||||
|
```html
|
||||||
|
<!-- Expandable group pill — members resolved -->
|
||||||
|
<span class="user-pill group-expandable"
|
||||||
|
onclick="toggleGroup('grpmem0')"
|
||||||
|
style="cursor:pointer">HR Site Members ▼</span>
|
||||||
|
<!-- Hidden member sub-rows follow immediately after the main <tr> -->
|
||||||
|
<tr data-group="grpmem0" style="display:none">
|
||||||
|
<td colspan="7" style="padding-left:2em;font-size:.8rem;color:#555">
|
||||||
|
Alice Smith <alice@contoso.com> •
|
||||||
|
Bob Jones <bob@contoso.com>
|
||||||
|
</td>
|
||||||
|
</tr>
|
||||||
|
|
||||||
|
<!-- Expandable group pill — resolution failed -->
|
||||||
|
<span class="user-pill group-expandable"
|
||||||
|
onclick="toggleGroup('grpmem1')"
|
||||||
|
style="cursor:pointer">Visitors ▼</span>
|
||||||
|
<tr data-group="grpmem1" style="display:none">
|
||||||
|
<td colspan="7" style="padding-left:2em;font-size:.8rem;color:#888;font-style:italic">
|
||||||
|
members unavailable
|
||||||
|
</td>
|
||||||
|
</tr>
|
||||||
|
```
|
||||||
|
|
||||||
|
**CSS addition (inline in BuildHtml):**
|
||||||
|
```css
|
||||||
|
.group-expandable { cursor: pointer; }
|
||||||
|
.group-expandable:hover { opacity: 0.8; }
|
||||||
|
```
|
||||||
|
|
||||||
|
**`toggleGroup()` JS**: Reuse the EXISTING function from `HtmlExportService.BuildHtml` — it is currently absent from `HtmlExportService` (unlike `UserAccessHtmlExportService`). The standard `HtmlExportService` only has a `filterTable()` function. Phase 17 must add `toggleGroup()` to `HtmlExportService`'s inline JS.
|
||||||
|
|
||||||
|
### Pattern 6: PermissionsViewModel — Identify Groups and Call Resolver
|
||||||
|
|
||||||
|
**What:** In `ExportHtmlAsync()`, before calling `_htmlExportService.WriteAsync()`:
|
||||||
|
1. Collect distinct group names from `Results` where `PrincipalType == "SharePointGroup"`
|
||||||
|
2. If no groups → pass empty dict (existing behavior, no CSOM calls)
|
||||||
|
3. If groups exist → call `_sharePointGroupResolver.ResolveGroupsAsync(ctx, clientId, groupNames, ct)`
|
||||||
|
4. Pass resolved dict to `BuildHtml`
|
||||||
|
|
||||||
|
The ViewModel already has `_currentProfile` (for `ClientId`) and `_sessionManager` (for `ClientContext`). A `ClientContext` for the current site is available via `sessionManager.GetOrCreateContextAsync(profile, ct)`.
|
||||||
|
|
||||||
|
**Complication**: The permissions scan may span multiple sites. Groups are site-scoped — a group named "Site Members" may exist on every site. Phase 17 resolves group names using the FIRST site's context (or the selected site if single-site). This is acceptable because group names in the HTML report are also site-scoped in the display — the group name badge appears inline per row.
|
||||||
|
|
||||||
|
**Alternative (simpler)**: Resolve using the primary context from `_currentProfile` (the site collection admin context used for scanning). SharePoint group names are per-site, so for multi-site scans, group names may collide between sites. The simpler approach: resolve using the first available context. This is flagged as an open question.
|
||||||
|
|
||||||
|
### Pattern 7: BuildHtml Signature Extension
|
||||||
|
|
||||||
|
**What:** Add an optional `IReadOnlyDictionary<string, IReadOnlyList<ResolvedMember>>? groupMembers = null` parameter to both `BuildHtml` overloads and `WriteAsync`. Passing `null` (or omitting) preserves 100% existing behavior — existing callers need zero changes.
|
||||||
|
|
||||||
|
```csharp
|
||||||
|
// HtmlExportService.cs — add parameter to both BuildHtml overloads and WriteAsync
|
||||||
|
public string BuildHtml(
|
||||||
|
IReadOnlyList<PermissionEntry> entries,
|
||||||
|
ReportBranding? branding = null,
|
||||||
|
IReadOnlyDictionary<string, IReadOnlyList<ResolvedMember>>? groupMembers = null)
|
||||||
|
|
||||||
|
public async Task WriteAsync(
|
||||||
|
IReadOnlyList<PermissionEntry> entries,
|
||||||
|
string filePath,
|
||||||
|
CancellationToken ct,
|
||||||
|
ReportBranding? branding = null,
|
||||||
|
IReadOnlyDictionary<string, IReadOnlyList<ResolvedMember>>? groupMembers = null)
|
||||||
|
```
|
||||||
|
|
||||||
|
### Anti-Patterns to Avoid
|
||||||
|
|
||||||
|
- **Making `HtmlExportService.BuildHtml` async**: Breaks the pure/testable design. Pre-resolve in ViewModel, pass dict.
|
||||||
|
- **Injecting `ISharePointGroupResolver` into `HtmlExportService`**: Export service should remain infrastructure-free. Resolver belongs in the ViewModel layer.
|
||||||
|
- **Using Graph v1.0 `groups/{id}/members` to get SharePoint classic group members**: Graph does NOT support classic SharePoint group membership. Only CSOM works.
|
||||||
|
- **Calling resolver for every export regardless of whether groups exist**: Always check `Results.Any(r => r.PrincipalType == "SharePointGroup")` first — skip resolver entirely if no group rows present (zero CSOM calls overhead).
|
||||||
|
- **Re-using `ugrp`/`sgrp`/`loc` ID prefixes for group member sub-rows**: Collisions with existing toggleGroup IDs in `UserAccessHtmlExportService`. Use `grpmem{idx}`.
|
||||||
|
- **Blocking the UI thread during group resolution**: Resolution must be `await`-ed in an async command, same pattern as existing `ExportHtmlAsync` in `PermissionsViewModel`.
|
||||||
|
|
||||||
|
## Don't Hand-Roll
|
||||||
|
|
||||||
|
| Problem | Don't Build | Use Instead | Why |
|
||||||
|
|---------|-------------|-------------|-----|
|
||||||
|
| CSOM retry / throttle handling | Custom retry loop | `ExecuteQueryRetryHelper.ExecuteQueryRetryAsync` | Already handles 429/503 with exponential backoff; identical to how `PermissionsService` calls CSOM |
|
||||||
|
| Graph auth token acquisition | Custom HTTP client | `GraphClientFactory.CreateClientAsync(clientId, ct)` | MSAL + Graph SDK already wired; same factory used in `GraphUserDirectoryService` and `BulkMemberService` |
|
||||||
|
| Transitive AAD group expansion | Recursive Graph calls | `groups/{id}/transitiveMembers/microsoft.graph.user` + `PageIterator` | Single Graph call returns all leaf users regardless of nesting depth; `PageIterator` handles pagination |
|
||||||
|
| JavaScript expand/collapse for member rows | New JS function | Existing `toggleGroup(id)` function — add it to `HtmlExportService` | Identical implementation already in `UserAccessHtmlExportService`; just needs to be added to `HtmlExportService` inline JS block |
|
||||||
|
| HTML encoding | Custom escaping | `HtmlExportService.HtmlEncode()` private method (already exists) | Already handles `&`, `<`, `>`, `"`, `'` |
|
||||||
|
|
||||||
|
## Common Pitfalls
|
||||||
|
|
||||||
|
### Pitfall 1: Confusing SharePoint Classic Groups with AAD/M365 Groups
|
||||||
|
**What goes wrong:** Developer calls `graphClient.Groups[id]` for a SharePoint classic group (login `c:0(.s|true` prefix or similar) — this throws a 404 because SharePoint groups are not AAD groups.
|
||||||
|
**Why it happens:** SharePoint groups have login names like `c:0(.s|true` or `c:0t.c|tenant|<spGroupId>` — not the same as AAD group objects.
|
||||||
|
**How to avoid:** Only call Graph for logins that match the AAD group pattern `c:0t.c|tenant|<guid>` (a valid GUID in the final segment). All other members are treated as leaf users directly.
|
||||||
|
**Warning signs:** `ServiceException: Resource not found` from Graph SDK during member resolution.
|
||||||
|
|
||||||
|
### Pitfall 2: `toggleGroup()` Missing from `HtmlExportService` Inline JS
|
||||||
|
**What goes wrong:** The group pill onclick calls `toggleGroup('grpmem0')` but `HtmlExportService` currently only has `filterTable()` in its inline JS — not `toggleGroup()`.
|
||||||
|
**Why it happens:** `toggleGroup()` was added to `UserAccessHtmlExportService` (Phase 7) but never added to `HtmlExportService` (which renders the site-centric permissions report).
|
||||||
|
**How to avoid:** Add `toggleGroup()` to `HtmlExportService`'s `<script>` block as part of Phase 17. Do NOT import from `UserAccessHtmlExportService` — both services are self-contained.
|
||||||
|
**Warning signs:** Clicking a group pill does nothing in the rendered HTML.
|
||||||
|
|
||||||
|
### Pitfall 3: Group Name Case Sensitivity in Dictionary Lookup
|
||||||
|
**What goes wrong:** Group name in `PermissionEntry.Users` is "HR Team" but resolver stores "hr team" — dict lookup returns null, group renders as non-expandable.
|
||||||
|
**Why it happens:** SharePoint group names are case-preserving but comparison varies.
|
||||||
|
**How to avoid:** Use `StringComparer.OrdinalIgnoreCase` for the result dictionary. Collect group names using `.ToHashSet(StringComparer.OrdinalIgnoreCase)` before calling resolver.
|
||||||
|
|
||||||
|
### Pitfall 4: CSOM `progress` Parameter in `ExecuteQueryRetryHelper`
|
||||||
|
**What goes wrong:** `SharePointGroupResolver` calls `ExecuteQueryRetryHelper.ExecuteQueryRetryAsync(ctx, progress, ct)` but has no `IProgress<OperationProgress>` — passing `null` works but requires verifying the helper accepts null.
|
||||||
|
**Why it happens:** The helper signature requires an `IProgress<OperationProgress>` parameter.
|
||||||
|
**How to avoid:** Inspect `ExecuteQueryRetryHelper` signature — if it nullable-accepts null, pass null; otherwise create a no-op `Progress<OperationProgress>(_ => { })`. Checking the existing usage in `PermissionsService` shows `progress` is non-nullable in the interface but `null` can be passed as `IProgress<T>?`.
|
||||||
|
**Warning signs:** NullReferenceException inside retry helper during member resolution.
|
||||||
|
|
||||||
|
### Pitfall 5: Multi-Site Group Name Collision
|
||||||
|
**What goes wrong:** Site A and Site B both have a group named "Members". Resolver resolves using Site A's context — members list for "Members" is from Site A, but the HTML report row may be from Site B.
|
||||||
|
**Why it happens:** Group names are per-site; the permissions scan spans multiple sites.
|
||||||
|
**How to avoid:** Accept this limitation as Phase 17 scope. Document that group resolution uses the first available site context. For a full solution, the dict key would need to be `(siteUrl, groupName)` — defer to a future phase. Add a note in the HTML "members may reflect a specific site's group configuration."
|
||||||
|
**Warning signs:** Users report incorrect member lists for groups with the same name across sites.
|
||||||
|
|
||||||
|
### Pitfall 6: No `ExpandGroup` Option in Simplified HTML Export
|
||||||
|
**What goes wrong:** `HtmlExportService` has TWO `BuildHtml` overloads — one for `IReadOnlyList<PermissionEntry>` (standard) and one for `IReadOnlyList<SimplifiedPermissionEntry>` (simplified). Both need the `groupMembers` parameter.
|
||||||
|
**Why it happens:** The simplified export also renders SharePoint group pills via the same pattern (see `HtmlExportService.cs` lines 268-297).
|
||||||
|
**How to avoid:** Add `groupMembers` parameter to both `BuildHtml` overloads. The `PermissionsViewModel` calls both paths (`IsSimplified` flag). Both must receive the resolved dict.
|
||||||
|
|
||||||
|
## Code Examples
|
||||||
|
|
||||||
|
### Collecting Group Names from Results (ViewModel)
|
||||||
|
|
||||||
|
```csharp
|
||||||
|
// In PermissionsViewModel.ExportHtmlAsync() — before calling WriteAsync
|
||||||
|
var groupNames = Results
|
||||||
|
.Where(r => r.PrincipalType == "SharePointGroup")
|
||||||
|
.SelectMany(r => r.Users.Split(';', StringSplitOptions.RemoveEmptyEntries))
|
||||||
|
.Select(n => n.Trim())
|
||||||
|
.Where(n => n.Length > 0)
|
||||||
|
.Distinct(StringComparer.OrdinalIgnoreCase)
|
||||||
|
.ToList();
|
||||||
|
|
||||||
|
IReadOnlyDictionary<string, IReadOnlyList<ResolvedMember>>? groupMembers = null;
|
||||||
|
if (groupNames.Count > 0 && _groupResolver != null)
|
||||||
|
{
|
||||||
|
var ctx = await _sessionManager.GetOrCreateContextAsync(
|
||||||
|
new TenantProfile { TenantUrl = _currentProfile!.TenantUrl, ClientId = _currentProfile.ClientId },
|
||||||
|
ct);
|
||||||
|
groupMembers = await _groupResolver.ResolveGroupsAsync(ctx, _currentProfile.ClientId, groupNames, ct);
|
||||||
|
}
|
||||||
|
|
||||||
|
await _htmlExportService.WriteAsync(Results, dialog.FileName, ct, branding, groupMembers);
|
||||||
|
```
|
||||||
|
|
||||||
|
### HTML Pill Rendering (HtmlExportService)
|
||||||
|
|
||||||
|
```csharp
|
||||||
|
// In BuildHtml — replace existing pill rendering for SharePointGroup entries
|
||||||
|
for (int i = 0; i < logins.Length; i++)
|
||||||
|
{
|
||||||
|
var login = logins[i].Trim();
|
||||||
|
var name = i < names.Length ? names[i].Trim() : login;
|
||||||
|
var isGroup = entry.PrincipalType == "SharePointGroup";
|
||||||
|
|
||||||
|
if (isGroup && groupMembers != null && groupMembers.TryGetValue(name, out var members))
|
||||||
|
{
|
||||||
|
var memId = $"grpmem{grpMemIdx++}";
|
||||||
|
var arrow = " ▼"; // ▼
|
||||||
|
pillsBuilder.Append(
|
||||||
|
$"<span class=\"user-pill group-expandable\" onclick=\"toggleGroup('{memId}')\"" +
|
||||||
|
$">{HtmlEncode(name)}{arrow}</span>");
|
||||||
|
|
||||||
|
// Emit hidden member sub-rows after this <tr> closes
|
||||||
|
memberSubRows.AppendLine($"<tr data-group=\"{memId}\" style=\"display:none\">");
|
||||||
|
memberSubRows.AppendLine($" <td colspan=\"7\" style=\"padding-left:2em;font-size:.8rem;color:#555\">");
|
||||||
|
if (members.Count == 0)
|
||||||
|
{
|
||||||
|
memberSubRows.Append("<em style=\"color:#888\">members unavailable</em>");
|
||||||
|
}
|
||||||
|
else
|
||||||
|
{
|
||||||
|
memberSubRows.Append(string.Join(" • ", members.Select(m =>
|
||||||
|
$"{HtmlEncode(m.DisplayName)} <{HtmlEncode(m.Login)}>")));
|
||||||
|
}
|
||||||
|
memberSubRows.AppendLine(" </td>");
|
||||||
|
memberSubRows.AppendLine("</tr>");
|
||||||
|
}
|
||||||
|
else
|
||||||
|
{
|
||||||
|
var isExt = login.Contains("#EXT#", StringComparison.OrdinalIgnoreCase);
|
||||||
|
var pillCss = isExt ? "user-pill external-user" : "user-pill";
|
||||||
|
pillsBuilder.Append($"<span class=\"{pillCss}\" data-email=\"{HtmlEncode(login)}\">{HtmlEncode(name)}</span>");
|
||||||
|
}
|
||||||
|
}
|
||||||
|
sb.AppendLine($" <td>{pillsBuilder}</td>");
|
||||||
|
// ... rest of row ...
|
||||||
|
sb.AppendLine("</tr>");
|
||||||
|
sb.Append(memberSubRows); // append member sub-rows immediately after main row
|
||||||
|
memberSubRows.Clear();
|
||||||
|
```
|
||||||
|
|
||||||
|
### CSOM Group User Loading
|
||||||
|
|
||||||
|
```csharp
|
||||||
|
// Source: CSOM pattern — identical to pattern used in BulkMemberService.AddToClassicGroupAsync
|
||||||
|
var group = ctx.Web.SiteGroups.GetByName(groupName);
|
||||||
|
ctx.Load(group.Users, users => users.Include(
|
||||||
|
u => u.Title,
|
||||||
|
u => u.LoginName,
|
||||||
|
u => u.PrincipalType));
|
||||||
|
await ExecuteQueryRetryHelper.ExecuteQueryRetryAsync(ctx, null!, ct);
|
||||||
|
```
|
||||||
|
|
||||||
|
### Graph Transitive Members (AAD nested groups)
|
||||||
|
|
||||||
|
```csharp
|
||||||
|
// Source: Graph SDK pattern — consistent with GraphUserDirectoryService.cs PageIterator usage
|
||||||
|
// GET /groups/{aadGroupId}/transitiveMembers/microsoft.graph.user
|
||||||
|
var response = await graphClient
|
||||||
|
.Groups[aadGroupId]
|
||||||
|
.TransitiveMembers
|
||||||
|
.GraphUser // cast to user objects only — excludes sub-groups, devices
|
||||||
|
.GetAsync(config =>
|
||||||
|
{
|
||||||
|
config.QueryParameters.Select = new[] { "displayName", "userPrincipalName" };
|
||||||
|
config.QueryParameters.Top = 999;
|
||||||
|
}, ct);
|
||||||
|
// Use PageIterator for pagination (same pattern as GraphUserDirectoryService)
|
||||||
|
```
|
||||||
|
|
||||||
|
## State of the Art
|
||||||
|
|
||||||
|
| Old Approach | Current Approach | When Changed | Impact |
|
||||||
|
|--------------|------------------|--------------|--------|
|
||||||
|
| `BuildHtml(entries, branding)` — groups opaque | `BuildHtml(entries, branding, groupMembers?)` — groups expandable | Phase 17 | Existing callers pass nothing; behavior unchanged |
|
||||||
|
| No member resolution service | `ISharePointGroupResolver` via CSOM + Graph transitive | Phase 17 | New DI service registration in `App.xaml.cs` |
|
||||||
|
| `HtmlExportService` has no `toggleGroup()` JS | `toggleGroup()` added to inline JS block | Phase 17 | Required for interactive group expansion |
|
||||||
|
|
||||||
|
**Deprecated / not applicable:**
|
||||||
|
- Graph `sharePointGroup` beta endpoint: Not applicable — targets SharePoint Embedded containers, not classic SharePoint site groups
|
||||||
|
- Graph `groups/{id}/members`: Not applicable for SharePoint classic groups (only for AAD groups)
|
||||||
|
|
||||||
|
## Open Questions
|
||||||
|
|
||||||
|
1. **Multi-site group name collision**
|
||||||
|
- What we know: Groups are site-scoped; same name may exist on multiple sites in a multi-site scan
|
||||||
|
- What's unclear: Should resolution use the first site's context, or resolve per-site?
|
||||||
|
- Recommendation: Use the first site's context for Phase 17. This is a known limitation. If the plan decides per-site resolution is needed, the dict key changes to `(siteUrl, groupName)` — a bounded change.
|
||||||
|
|
||||||
|
2. **`ExecuteQueryRetryHelper` null progress parameter**
|
||||||
|
- What we know: `PermissionsService` passes `progress` (non-null) always
|
||||||
|
- What's unclear: Whether `ExecuteQueryRetryHelper` accepts `null` for the progress parameter
|
||||||
|
- Recommendation: Read `ExecuteQueryRetryHelper` before implementing; if it does not accept null, create `Progress<OperationProgress>(_ => { })` no-op.
|
||||||
|
|
||||||
|
3. **Which simplified export is affected**
|
||||||
|
- What we know: `HtmlExportService` has two `BuildHtml` overloads — one for `PermissionEntry` (standard) and one for `SimplifiedPermissionEntry` (simplified with risk labels)
|
||||||
|
- What's unclear: Should group expansion apply to BOTH, or only the standard report?
|
||||||
|
- Recommendation: Apply to both — `SimplifiedPermissionEntry` wraps `PermissionEntry` and the same group pills are rendered; adding `groupMembers` to the simplified overload is minimal extra work.
|
||||||
|
|
||||||
|
4. **`ResolvedMember` model placement**
|
||||||
|
- What we know: The model is used by both the service and the HTML export
|
||||||
|
- What's unclear: Should it live in `Core.Models` (alongside `UserAccessEntry`, etc.) or in `Services` namespace?
|
||||||
|
- Recommendation: Place in `Core.Models` — consistent with `LocationInfo`, `GraphDirectoryUser`, `GraphUserResult` which are all small value records in that namespace.
|
||||||
|
|
||||||
|
## Validation Architecture
|
||||||
|
|
||||||
|
### Test Framework
|
||||||
|
| Property | Value |
|
||||||
|
|----------|-------|
|
||||||
|
| Framework | xUnit (project: `SharepointToolbox.Tests`) |
|
||||||
|
| Config file | `SharepointToolbox.Tests/SharepointToolbox.Tests.csproj` |
|
||||||
|
| Quick run command | `dotnet test --filter "FullyQualifiedName~HtmlExportServiceTests" --no-build` |
|
||||||
|
| Full suite command | `dotnet test` |
|
||||||
|
|
||||||
|
### Phase Requirements → Test Map
|
||||||
|
|
||||||
|
| Req ID | Behavior | Test Type | Automated Command | File Exists? |
|
||||||
|
|--------|----------|-----------|-------------------|-------------|
|
||||||
|
| RPT-01-a | `BuildHtml` with no `groupMembers` → existing output byte-identical | unit | `dotnet test --filter "FullyQualifiedName~HtmlExportServiceTests"` | ✅ (extend existing) |
|
||||||
|
| RPT-01-b | `BuildHtml` with `groupMembers` for a group → renders clickable pill with toggle id | unit | `dotnet test --filter "FullyQualifiedName~HtmlExportServiceTests"` | ✅ (extend existing) |
|
||||||
|
| RPT-01-c | `BuildHtml` with resolved members → hidden sub-rows with member names | unit | `dotnet test --filter "FullyQualifiedName~HtmlExportServiceTests"` | ✅ (extend existing) |
|
||||||
|
| RPT-01-d | `BuildHtml` with empty member list (failed resolution) → "members unavailable" label | unit | `dotnet test --filter "FullyQualifiedName~HtmlExportServiceTests"` | ✅ (extend existing) |
|
||||||
|
| RPT-01-e | `HtmlExportService` inline JS contains `toggleGroup` function | unit | `dotnet test --filter "FullyQualifiedName~HtmlExportServiceTests"` | ✅ (extend existing) |
|
||||||
|
| RPT-02-a | `SharePointGroupResolver` returns empty list (not throw) when CSOM group not found | unit (mock) | `dotnet test --filter "FullyQualifiedName~SharePointGroupResolverTests"` | ❌ Wave 0 |
|
||||||
|
| RPT-02-b | `SharePointGroupResolver.ResolveGroupsAsync` with live tenant → actual members | skip | `[Fact(Skip="Requires live SP tenant")]` | ❌ Wave 0 |
|
||||||
|
| RPT-02-c | AAD group login detection (`IsAadGroup`) — unit test on pattern | unit | `dotnet test --filter "FullyQualifiedName~SharePointGroupResolverTests"` | ❌ Wave 0 |
|
||||||
|
|
||||||
|
### Sampling Rate
|
||||||
|
- **Per task commit:** `dotnet test --filter "FullyQualifiedName~HtmlExportServiceTests" --no-build`
|
||||||
|
- **Per wave merge:** `dotnet test`
|
||||||
|
- **Phase gate:** Full suite green before `/gsd:verify-work`
|
||||||
|
|
||||||
|
### Wave 0 Gaps
|
||||||
|
- [ ] `SharepointToolbox.Tests/Services/SharePointGroupResolverTests.cs` — covers RPT-02-a, RPT-02-b (skip), RPT-02-c
|
||||||
|
- [ ] `Core/Models/ResolvedMember.cs` — new value record needed before tests can compile
|
||||||
|
- [ ] `Services/ISharePointGroupResolver.cs` — interface stub needed before resolver tests compile
|
||||||
|
- [ ] No new framework install needed — xUnit already configured
|
||||||
|
|
||||||
|
## Sources
|
||||||
|
|
||||||
|
### Primary (HIGH confidence)
|
||||||
|
- Direct inspection of `SharepointToolbox/Services/Export/HtmlExportService.cs` — two `BuildHtml` overloads, user pill rendering, inline JS (no `toggleGroup`), `WriteAsync` signature
|
||||||
|
- Direct inspection of `SharepointToolbox/Services/Export/UserAccessHtmlExportService.cs` — `toggleGroup()` JS implementation (lines 285-289 in C# string, confirmed via reading)
|
||||||
|
- Direct inspection of `SharepointToolbox/Services/PermissionsService.cs` lines 312-335 — how `PrincipalType = "SharePointGroup"` and `GrantedThrough = "SharePoint Group: {name}"` are set; `member.Title` = group display name stored in `Users`
|
||||||
|
- Direct inspection of `SharepointToolbox/Services/BulkMemberService.cs` — CSOM `SiteGroups.GetByName` pattern and `graphClient.Groups[id]` pattern
|
||||||
|
- Direct inspection of `SharepointToolbox/Infrastructure/Auth/GraphClientFactory.cs` — `CreateClientAsync(clientId, ct)` returns `GraphServiceClient`
|
||||||
|
- Direct inspection of `SharepointToolbox/Services/GraphUserDirectoryService.cs` — `PageIterator` pattern for Graph pagination
|
||||||
|
- Direct inspection of `SharepointToolbox/ViewModels/Tabs/PermissionsViewModel.cs` — `_currentProfile`, `_sessionManager`, `ExportHtmlAsync` call sites
|
||||||
|
|
||||||
|
### Secondary (MEDIUM confidence)
|
||||||
|
- [Microsoft Learn: List group transitive members](https://learn.microsoft.com/en-us/graph/api/group-list-transitivemembers?view=graph-rest-1.0) — `GET /groups/{id}/transitiveMembers/microsoft.graph.user` v1.0 endpoint confirmed
|
||||||
|
- [Microsoft Q&A: Graph API cannot fetch classic SharePoint site group members](https://learn.microsoft.com/en-us/answers/questions/5535260/microsoft-graph-api-how-can-i-fetch-them-members-o) — confirms CSOM is the only path for classic SharePoint groups
|
||||||
|
- [Microsoft Q&A: Retrieve SharePoint Site Group Members via API](https://learn.microsoft.com/en-us/answers/questions/5578364/retrieve-sharepoint-site-group-members-via-api) — confirms SharePoint REST `/_api/web/sitegroups/getbyname/users` as alternative (not used here; CSOM is preferred given existing CSOM infrastructure)
|
||||||
|
|
||||||
|
### Tertiary (LOW confidence)
|
||||||
|
- AAD group login format `c:0t.c|tenant|<guid>` — derived from codebase pattern inspection and community examples; not found in official Microsoft docs but consistent with observed SharePoint Online claims encoding
|
||||||
|
|
||||||
|
## Metadata
|
||||||
|
|
||||||
|
**Confidence breakdown:**
|
||||||
|
- Standard stack: HIGH — all dependencies already in project; no new NuGet packages
|
||||||
|
- Architecture: HIGH — all patterns directly observed in existing code; pre-resolution approach mirrors branding service pattern
|
||||||
|
- Pitfalls: HIGH — derived from direct reading of all affected files and confirmed API limitations
|
||||||
|
- Graph API for transitive members: HIGH — official Microsoft Learn docs confirmed
|
||||||
|
- CSOM-only for classic groups: HIGH — multiple official Microsoft Q&A sources confirm Graph limitation
|
||||||
|
- AAD group login prefix pattern: MEDIUM — inferred from codebase + community; not in official reference docs
|
||||||
|
|
||||||
|
**Research date:** 2026-04-09
|
||||||
|
**Valid until:** 2026-05-09 (stable CSOM/Graph APIs; toggleGroup pattern is internal to project)
|
||||||
Reference in New Issue
Block a user