diff --git a/.planning/research/ARCHITECTURE.md b/.planning/research/ARCHITECTURE.md index 320e9c9..6687f69 100644 --- a/.planning/research/ARCHITECTURE.md +++ b/.planning/research/ARCHITECTURE.md @@ -1,443 +1,438 @@ # Architecture Patterns -**Domain:** C#/WPF MVVM desktop app — SharePoint Online MSP admin tool -**Feature scope:** Report branding (MSP/client logos in HTML) + User directory browse mode -**Researched:** 2026-04-08 -**Confidence:** HIGH — based on direct codebase inspection, not assumptions +**Project:** SharePoint Toolbox v2.3 — Tenant Management & Report Enhancements +**Researched:** 2026-04-09 +**Scope:** Integration of four new features into the existing MVVM/DI architecture --- ## Existing Architecture (Baseline) +The app uses a clean layered architecture. Understanding the layers is prerequisite to placing new features correctly. + ``` Core/ - Models/ — TenantProfile, AppSettings, domain records (all POCOs/records) - Messages/ — WeakReferenceMessenger value message types - Helpers/ — Static utility classes + Models/ — Pure data records and enums (no dependencies) + Helpers/ — Static utility methods + Messages/ — WeakReferenceMessenger message types Infrastructure/ - Auth/ — MsalClientFactory, GraphClientFactory (MSAL PCA per-tenant + Graph SDK bridge) - Persistence/ — ProfileRepository, SettingsRepository, TemplateRepository (JSON, atomic write-then-replace) - Logging/ — LogPanelSink (Serilog sink to in-app RichTextBox) + Auth/ — MsalClientFactory, GraphClientFactory, SessionManager wiring + Persistence/ — JSON-backed repositories (ProfileRepository, BrandingRepository, etc.) Services/ - Export/ — Concrete HTML/CSV export services per domain (no interface, consumed directly) - *.cs — Domain services with IXxx interfaces + *.cs — Interface + implementation pairs (feature business logic) + Export/ — HTML and CSV export services per feature area ViewModels/ - FeatureViewModelBase.cs — Abstract base: RunCommand, CancelCommand, ProgressValue, StatusMessage, - GlobalSites, WeakReferenceMessenger registration - MainWindowViewModel.cs — Toolbar: tenant picker, Connect, global site picker, broadcasts TenantSwitchedMessage - Tabs/ — One ViewModel per tab, all extend FeatureViewModelBase - ProfileManagementViewModel.cs — Profile CRUD dialog VM + FeatureViewModelBase — Abstract base: RunCommand, CancelCommand, progress, WeakReferenceMessenger + Tabs/ — One ViewModel per tab + ProfileManagementViewModel — Tenant profile CRUD + logo management Views/ - Dialogs/ — ProfileManagementDialog, SitePickerDialog, ConfirmBulkOperationDialog, FolderBrowserDialog - Tabs/ — One UserControl per tab (XAML + code-behind) - -App.xaml.cs — Generic Host IServiceCollection DI registration for all layers + Tabs/ — XAML views, pure DataBinding + Dialogs/ — Modal dialogs (ProfileManagementDialog, SitePickerDialog, etc.) ``` -### Key Patterns Already Established +### Key Architectural Invariants (must not be broken) -| Pattern | How It Works | -|---------|-------------| -| Tenant switching | `MainWindowViewModel.OnSelectedProfileChanged` broadcasts `TenantSwitchedMessage` via `WeakReferenceMessenger`; each tab VM overrides `OnTenantSwitched(profile)` | -| Global site propagation | `GlobalSitesChangedMessage` received in `FeatureViewModelBase.OnGlobalSitesReceived` | -| HTML export | Concrete service class (e.g. `UserAccessHtmlExportService`), `BuildHtml(entries)` returns a string, `WriteAsync(entries, path, ct)` writes it. No interface. Pure data-in, HTML-out. | -| JSON persistence | Repository pattern: constructor takes `string filePath`, atomic write via `.tmp` + round-trip JSON validation before `File.Move`, `SemaphoreSlim` write lock. | -| DI registration | All in `App.xaml.cs RegisterServices()`. Export services and ViewModels are `AddTransient`; shared infrastructure is `AddSingleton`. | -| Dialog factory | View code-behind sets `ViewModel.OpenXxxDialog = () => new XxxDialog(...)` — keeps dialogs out of ViewModel layer | -| People-picker search | `IGraphUserSearchService.SearchUsersAsync(clientId, query, maxResults, ct)` calls Graph `/users?$filter=startsWith(...)` with `ConsistencyLevel: eventual` | -| Test constructor | `UserAccessAuditViewModel` has a `internal` 3-param constructor without export services — test pattern to replicate for new injections | +1. **SessionManager is the sole holder of ClientContext.** All services receive it via constructor injection; none store it. +2. **GraphClientFactory.CreateClientAsync(clientId)** produces a GraphServiceClient scoped to a specific tenant's PCA from MsalClientFactory. +3. **FeatureViewModelBase** provides RunCommand/CancelCommand/progress wiring. All tab VMs extend it. +4. **WeakReferenceMessenger** carries cross-cutting signals: `TenantSwitchedMessage`, `GlobalSitesChangedMessage`. VMs react in `OnTenantSwitched` / `OnGlobalSitesChanged`. +5. **BulkOperationRunner.RunAsync** is the shared continue-on-error runner for all multi-item operations. +6. **HTML export services** are independent per-feature classes under `Services/Export/`; they receive `ReportBranding?` and call `BrandingHtmlHelper.BuildBrandingHeader()`. +7. **DI registration** is in `App.xaml.cs → RegisterServices`. New services register there. --- -## Feature 1: Report Branding (MSP/Client Logos in HTML Reports) +## Feature 1: App Registration via Graph API -### What It Needs +### What It Does +During profile create/edit, attempt to register a new Azure AD app on the target tenant (auto path), or instruct the user through manual steps (guided fallback path). -- **MSP logo** — one global image, shown in every HTML report from every tenant -- **Client logo** — one image per tenant, shown in reports for that tenant only -- **Storage** — base64-encoded strings in JSON (no separate image files — preserves atomic save semantics and single-data-folder design) -- **Embedding** — `data:image/...;base64,...` `` tag injected into the HTML header (maintains self-contained HTML invariant — zero external file references) -- **User action** — file picker → read bytes → detect MIME type → convert to base64 → store in JSON → preview in UI +### Graph API Constraint (HIGH confidence) +Creating an application registration via `POST /applications` requires the caller to hold `Application.ReadWrite.All`. This is an admin-consent-required delegated permission. The existing GraphClientFactory uses `.default` scope, which only acquires permissions already pre-consented on the PCA's app registration. This means: -### New Components (create from scratch) +- **The Toolbox's own client app registration (the one the MSP registered to run this tool) must have `Application.ReadWrite.All` delegated and admin-consented** before the auto path can work. +- If that permission is absent, the Graph call returns 403. The auto path must catch `ODataError` with status 403 and fall through to guided fallback automatically. +- The guided fallback shows the MSP admin step-by-step instructions for creating the app registration manually in the Azure portal and entering the resulting ClientId. + +### New Service: `IAppRegistrationService` / `AppRegistrationService` + +**Location:** `Services/AppRegistrationService.cs` + `Services/IAppRegistrationService.cs` + +**Responsibilities:** +- `RegisterAppAsync(GraphServiceClient, string tenantName, CancellationToken)` — Creates the app registration and optional service principal on the target tenant. Returns `AppRegistrationResult` (success + new ClientId, or failure reason). +- `RemoveAppAsync(GraphServiceClient, string clientId, CancellationToken)` — Deletes the app object by clientId. Also cleans up service principal. + +**Required Graph calls (inside `AppRegistrationService`):** +1. `POST /applications` — create the app with required `requiredResourceAccess` (SharePoint delegated scopes) +2. `POST /servicePrincipals` — create service principal for the new app so it can receive admin consent +3. `DELETE /applications/{id}` for removal +4. `DELETE /servicePrincipals/{id}` for service principal cleanup + +### New Model: `AppRegistrationResult` -**`Core/Models/BrandingSettings.cs`** ```csharp -public class BrandingSettings -{ - public string? MspLogoBase64 { get; set; } - public string? MspLogoMimeType { get; set; } // "image/png", "image/jpeg", etc. -} +// Core/Models/AppRegistrationResult.cs +public record AppRegistrationResult( + bool Success, + string? ClientId, // set when Success=true + string? ApplicationId, // object ID, needed for deletion + string? FailureReason // set when Success=false +); ``` -Belongs in Core/Models alongside AppSettings. Kept separate — branding may grow independently of general app settings. -**`Core/Models/ReportBranding.cs`** +### Integration Point: `ProfileManagementViewModel` + +This is the only ViewModel that changes. `ProfileManagementViewModel` already receives `GraphClientFactory`. Add: + +- `IAppRegistrationService` injected via constructor +- `RegisterAppCommand` (IAsyncRelayCommand) — triggers auto-registration, falls back to guided mode on 403 +- `RemoveAppCommand` (IAsyncRelayCommand) — available when `SelectedProfile != null && SelectedProfile.ClientId != null` +- `IsRegistering` observable bool for busy state +- `AppRegistrationStatus` observable string for feedback + +**Data flow:** +``` +ProfileManagementViewModel.RegisterAppCommand + → GraphClientFactory.CreateClientAsync(currentMspClientId) // uses MSP's own clientId + → AppRegistrationService.RegisterAppAsync(graphClient, tenantName) + → POST /applications, POST /servicePrincipals + → returns AppRegistrationResult + → on success: populate NewClientId, surface "Copy ClientId" affordance + → on 403: set guided fallback mode (show instructions panel) + → on other error: set ValidationMessage +``` + +No new ViewModel is needed. The guided fallback is a conditional UI panel in `ProfileManagementDialog.xaml` controlled by a new `IsGuidedFallbackVisible` bool property on `ProfileManagementViewModel`. + +### DI Registration (App.xaml.cs) + ```csharp -public record ReportBranding( - string? MspLogoBase64, - string? MspLogoMimeType, - string? ClientLogoBase64, - string? ClientLogoMimeType); +services.AddTransient(); ``` -Lightweight data transfer record assembled at export time from BrandingSettings + current TenantProfile. Not persisted directly — constructed on demand. -**`Infrastructure/Persistence/BrandingRepository.cs`** -Same pattern as `SettingsRepository`: constructor takes `string filePath`, atomic write-then-replace, `SemaphoreSlim` write lock. File: `%AppData%\SharepointToolbox\branding.json`. +`ProfileManagementViewModel` registration remains `AddTransient`; the new interface is added to its constructor. -**`Services/BrandingService.cs`** +--- + +## Feature 2: Auto-Take Ownership on Access Denied + +### What It Does +A global toggle in Settings. When enabled, if any SharePoint operation returns an access-denied error, the app automatically adds the authenticated account as a site collection administrator using the tenant admin API, then retries the operation. + +### Tenant Admin API Mechanism (HIGH confidence from PnP Framework source) +PnP Framework's `Tenant` class (in `Microsoft.Online.SharePoint.TenantAdministration`) exposes site management. The pattern already used in `SiteListService` (which clones to the `-admin` URL) is exactly the right entry point. + +To add self as admin: ```csharp -public class BrandingService -{ - public Task GetBrandingAsync(); - public Task SetMspLogoAsync(string filePath); // reads file, detects MIME, converts to base64, saves - public Task ClearMspLogoAsync(); -} +var tenant = new Tenant(adminCtx); +tenant.SetSiteAdmin(siteUrl, loginName, isAdmin: true); +adminCtx.ExecuteQueryAsync(); ``` -Thin orchestration, same pattern as `SettingsService`. MSP logo only — client logo is managed via `ProfileService` (it belongs to `TenantProfile`). +This does NOT require having access to the site — only SharePoint Admin role on the tenant, which the interactive login flow already acquires. -### Modified Components +### New Setting Property: `AppSettings.AutoTakeOwnership` -**`Core/Models/TenantProfile.cs`** — Add two nullable string properties: ```csharp -public string? ClientLogoBase64 { get; set; } -public string? ClientLogoMimeType { get; set; } +// Core/Models/AppSettings.cs — ADD property +public bool AutoTakeOwnership { get; set; } = false; ``` -This is backward-compatible. `ProfileRepository` uses `JsonSerializer` with `PropertyNameCaseInsensitive: true` — missing JSON fields deserialize to null without error. Existing `profiles.json` files continue to load correctly. -**All HTML export services** — Add `ReportBranding? branding = null` optional parameter to every `BuildHtml()` overload. When non-null and at least one logo is present, inject a branding header div between `` open and `

`: +This persists in `settings.json` automatically via `SettingsRepository`. + +### New Service: `ISiteOwnershipService` / `SiteOwnershipService` + +**Location:** `Services/SiteOwnershipService.cs` + `Services/ISiteOwnershipService.cs` + +**Responsibility:** One method: +```csharp +Task AddCurrentUserAsSiteAdminAsync( + TenantProfile profile, + string siteUrl, + CancellationToken ct); +``` + +Uses `SessionManager` to get the authenticated context, clones to the admin URL (same pattern as `SiteListService.DeriveAdminUrl`), constructs `Tenant`, and calls `SetSiteAdmin`. + +### Integration Point: `ExecuteQueryRetryHelper` or Caller Wrap + +Rather than modifying `ExecuteQueryRetryHelper` (which is stateless and generic), the retry-with-ownership logic belongs in a per-operation wrapper: + +1. Calls the operation +2. Catches `ServerException` with "Access Denied" message +3. If `AppSettings.AutoTakeOwnership == true`, calls `SiteOwnershipService.AddCurrentUserAsSiteAdminAsync` +4. Retries exactly once +5. If retry also fails, propagates the error with a message indicating ownership was attempted + +**Recommended placement:** A new static helper `SiteAccessRetryHelper` in `Core/Helpers/`, wrapping CSOM executeQuery invocations in `PermissionsService`, `UserAccessAuditService`, and `SiteListService`. Each of these services already has an `IProgress` parameter and `CancellationToken` — the helper signature matches naturally. + +### SettingsViewModel Changes + +- Add `AutoTakeOwnership` observable bool property +- Wire to new `SettingsService.SetAutoTakeOwnershipAsync(bool)` method +- Bind to a checkbox in `SettingsView.xaml` + +### DI Registration + +```csharp +services.AddTransient(); +``` + +--- + +## Feature 3: Expand Groups in HTML Reports + +### What It Does +In the permissions HTML report, SharePoint group entries (where `PrincipalType == "SharePointGroup"`) currently show the group name as a single user pill. When expanded (click on the group), the report shows the individual group members. + +### Data Model Change + +`PermissionEntry` is a `record`. Group member data must be captured at scan time because the HTML report is self-contained offline — no live API calls from the browser are possible. + +**Approach: Resolve at scan time.** During `PermissionsService.ExtractPermissionsAsync`, when `principalType == "SharePointGroup"`, load group members via CSOM and store them in a new optional field on `PermissionEntry`. + +**Model change — additive, backward-compatible:** +```csharp +public record PermissionEntry( + // ... all existing parameters unchanged ... + string? GroupMembers = null // semicolon-joined login names; null when not a group or not expanded +); +``` +Using a default parameter keeps all existing constructors and test data valid without changes. + +### New Scan Option + +```csharp +// Core/Models/ScanOptions.cs — ADD parameter with default +public record ScanOptions( + bool IncludeInherited, + bool ScanFolders, + int FolderDepth, + bool IncludeSubsites, + bool ExpandGroupMembers = false // NEW — defaults off +); +``` + +### Service Changes: `PermissionsService` + +In `ExtractPermissionsAsync`, when `principalType == "SharePointGroup"` and `options.ExpandGroupMembers == true`: +```csharp +ctx.Load(ra.Member, m => m.Users.Include(u => u.LoginName, u => u.Title)); +await ExecuteQueryRetryHelper.ExecuteQueryRetryAsync(ctx, progress, ct); +var groupMembers = string.Join(";", ra.Member.Users.Select(u => u.LoginName)); +``` +This adds one CSOM round-trip per SharePoint group entry. Performance note: default is `false`. + +### HTML Export Changes: `HtmlExportService` + +When rendering user pills for an entry with `GroupMembers != null`, render the group name as an HTML5 `
/` expandable block. The `
/` element requires zero JavaScript, is self-contained, and is universally supported in all modern browsers (Chrome, Edge, Firefox, Safari) since 2016. ```html -
- - MSP - Client -
+
+ Members Group Name +
+ alice@contoso.com + bob@contoso.com +
+
``` -When `branding` is null (existing callers) the block is omitted entirely. No behavior change for callers that do not pass branding. +`UserAccessHtmlExportService` gets the same treatment in the "Granted Through" column where group access is reported. -Affected services (all in `Services/Export/`): -- `HtmlExportService` (two `BuildHtml` overloads — `PermissionEntry` and `SimplifiedPermissionEntry`) -- `UserAccessHtmlExportService` -- `StorageHtmlExportService` (two `BuildHtml` overloads — with and without `FileTypeMetric`) -- `SearchHtmlExportService` -- `DuplicatesHtmlExportService` +### ViewModel Changes: `PermissionsViewModel` -**ViewModels that call HTML export** — All `ExportHtmlAsync` methods need to resolve branding before calling the export service. The ViewModel calls `BrandingService.GetBrandingAsync()` and reads `_currentProfile.ClientLogoBase64` to assemble a `ReportBranding`, then passes it to `BuildHtml`. - -Affected ViewModels: `PermissionsViewModel`, `UserAccessAuditViewModel`, `StorageViewModel`, `SearchViewModel`, `DuplicatesViewModel`. Each gets `BrandingService` injected via constructor. - -**`ViewModels/Tabs/SettingsViewModel.cs`** — Add MSP logo management: -```csharp -[ObservableProperty] private string? _mspLogoPreviewBase64; -public RelayCommand BrowseMspLogoCommand { get; } -public RelayCommand ClearMspLogoCommand { get; } -``` -On browse: open `OpenFileDialog` (filter: PNG, JPG, GIF) → call `BrandingService.SetMspLogoAsync(path)` → reload and refresh `MspLogoPreviewBase64`. - -**`Views/Tabs/SettingsView.xaml`** — Add a "Report Branding — MSP Logo" section: -- `` bound to `MspLogoPreviewBase64` via a base64-to-BitmapSource converter -- "Browse Logo" button → `BrowseMspLogoCommand` -- "Clear" button → `ClearMspLogoCommand` -- Note label: "Applies to all reports" - -**Client logo placement:** Client logo belongs to a `TenantProfile`, not to global settings. The natural place to manage it is `ProfileManagementDialog` (already handles profile CRUD). Add logo fields there rather than in SettingsView. - -**`ViewModels/ProfileManagementViewModel.cs`** — Add client logo management per profile: -```csharp -[ObservableProperty] private string? _clientLogoPreviewBase64; -public RelayCommand BrowseClientLogoCommand { get; } -public RelayCommand ClearClientLogoCommand { get; } -``` -On browse: read image bytes → base64 → set on the being-edited `TenantProfile` object before saving. Uses `ProfileService.AddProfileAsync` / rename pipeline that already exists. - -**`Views/Dialogs/ProfileManagementDialog.xaml`** — Add client logo fields to the add/edit profile form (same pattern as SettingsView branding section). - -### Data Flow: Report Branding - -``` -User picks MSP logo (SettingsView "Browse Logo" button) - → SettingsViewModel.BrowseMspLogoCommand - → OpenFileDialog in View code-behind or VM (follow existing BrowseFolder pattern) - → BrandingService.SetMspLogoAsync(path) - → File.ReadAllBytesAsync → Convert.ToBase64String - → detect MIME from extension (.png → image/png, .jpg/.jpeg → image/jpeg, .gif → image/gif) - → BrandingRepository.SaveAsync(BrandingSettings) - → ViewModel refreshes MspLogoPreviewBase64 - -User runs export (e.g. ExportHtmlCommand in UserAccessAuditViewModel) - → BrandingService.GetBrandingAsync() → BrandingSettings - → reads _currentProfile.ClientLogoBase64, _currentProfile.ClientLogoMimeType - → new ReportBranding(mspBase64, mspMime, clientBase64, clientMime) - → UserAccessHtmlExportService.BuildHtml(entries, branding) - → injects data URIs in header when base64 is non-null - → writes HTML file -``` +Add `ExpandGroupMembers` observable bool. Include in `ScanOptions` construction in `RunOperationAsync`. Add checkbox to `PermissionsView.xaml`. --- -## Feature 2: User Directory Browse Mode +## Feature 4: Report Entry Consolidation Toggle -### What It Needs +### What It Does +When a user appears in multiple SharePoint groups that all have access to the same object, they generate multiple `PermissionEntry` rows. The consolidation toggle merges rows for the same (Object, User) combination, joining permission levels and grant sources. -The existing `UserAccessAuditView` has a people-picker: search box → Graph API `startsWith` filter → autocomplete dropdown → add to `SelectedUsers`. Directory browse mode is an alternative to the search box: show a paginated, filterable list of all tenant users, allow multi-select, bulk-add to `SelectedUsers`. +### Where Consolidation Lives -This is purely additive. The underlying audit logic (`IUserAccessAuditService`, `RunOperationAsync`, `SelectedUsers` collection, export commands) is completely unchanged. +This is a pure post-processing transformation on the already-collected `IReadOnlyList`. It requires no new service, no CSOM calls, no Graph calls. -### New Components (create from scratch) +**Location:** New static helper class in `Core/Helpers/`: -**`Core/Models/PagedUserResult.cs`** ```csharp -public record PagedUserResult( - IReadOnlyList Users, - string? NextPageToken); // null = last page -``` - -**`Services/IGraphUserDirectoryService.cs`** -```csharp -public interface IGraphUserDirectoryService +// Core/Helpers/PermissionConsolidator.cs +public static class PermissionConsolidator { - Task GetUsersPageAsync( - string clientId, - string? filter = null, - string? pageToken = null, - int pageSize = 100, - CancellationToken ct = default); + public static IReadOnlyList Consolidate( + IReadOnlyList entries); } ``` -**`Services/GraphUserDirectoryService.cs`** -Reuses `GraphClientFactory` (already injected elsewhere). Calls `graphClient.Users.GetAsync()` without the `startsWith` constraint used in search — uses `$top=100` with cursor-based paging via Graph's `@odata.nextLink`. Returns `PagedUserResult` so callers control pagination. Uses `ConsistencyLevel: eventual` + `$count=true` (same as existing search service). +**Consolidation key:** `(ObjectType, Title, Url, UserLogin)` — one row per (object, user) pair across all login tokens in a semicolon-delimited `UserLogins` field. -### Modified Components +**Merge logic:** +- `PermissionLevels`: union of distinct values (semicolon-joined) +- `GrantedThrough`: all distinct grant sources joined (e.g., "Direct Permissions; SharePoint Group: X") +- `HasUniquePermissions`: true if any source entry has it true +- `Users`, `UserLogins`: from the first occurrence (same person) +- `PrincipalType`: from the first occurrence -**`ViewModels/Tabs/UserAccessAuditViewModel.cs`** — Add browse mode state: +`PermissionEntry` is a `record` — `PermissionConsolidator.Consolidate()` produces new instances, never mutates. Consistent with the existing pattern in `PermissionsViewModel` where `Results` is replaced wholesale. + +**For `SimplifiedPermissionEntry`:** Consolidation applies to `PermissionEntry` first; `SimplifiedPermissionEntry.WrapAll()` then operates on the consolidated list. No changes to `SimplifiedPermissionEntry` needed. + +### ViewModel Changes: `PermissionsViewModel` + +Add `ConsolidateEntries` observable bool property. In `RunOperationAsync`, after collecting `allEntries`: ```csharp -[ObservableProperty] private bool _isBrowseModeActive; -[ObservableProperty] private ObservableCollection _directoryUsers = new(); -[ObservableProperty] private string _directoryFilter = string.Empty; -[ObservableProperty] private bool _isLoadingDirectory; -[ObservableProperty] private bool _hasMoreDirectoryPages; - -private string? _directoryNextPageToken; - -public IAsyncRelayCommand LoadDirectoryCommand { get; } -public IAsyncRelayCommand LoadMoreDirectoryCommand { get; } -public RelayCommand> AddDirectoryUsersCommand { get; } +if (ConsolidateEntries) + allEntries = PermissionConsolidator.Consolidate(allEntries).ToList(); ``` -`partial void OnIsBrowseModeActiveChanged(bool value)` → when `value == true`, fire `LoadDirectoryCommand` to populate page 1. +The export commands (`ExportCsvCommand`, `ExportHtmlCommand`) already consume `Results`, so consolidated data flows into all export formats automatically. No export service changes required for this feature. -`partial void OnDirectoryFilterChanged(string value)` → debounce 300ms (same pattern as `OnSearchQueryChanged`), re-fire `LoadDirectoryCommand` with new filter, clear `_directoryNextPageToken`. +--- -The `IGraphUserDirectoryService` is added to the constructor. The internal test constructor (currently 3 params) gets a 4-param overload adding the directory service with a null-safe default, or a new explicit test constructor. - -**`App.xaml.cs RegisterServices()`** — Add: -```csharp -services.AddTransient(); -``` -The `UserAccessAuditViewModel` transient registration picks up the new injection automatically (DI resolves by type). - -**`UserAccessAuditViewModel.OnTenantSwitched`** — Also clear `DirectoryUsers`, reset `_directoryNextPageToken`, `HasMoreDirectoryPages`, `IsLoadingDirectory`. - -**`Views/Tabs/UserAccessAuditView.xaml`** — Add to the top of the left panel: -- Mode toggle: two `RadioButton`s or `ToggleButton`s bound to `IsBrowseModeActive` -- "Search" panel: existing `GroupBox` shown when `IsBrowseModeActive == false` -- "Browse" panel: new `GroupBox` shown when `IsBrowseModeActive == true`, containing: - - Filter `TextBox` bound to `DirectoryFilter` - - `ListView` with `SelectionMode="Extended"` bound to `DirectoryUsers`, `SelectionChanged` handler in code-behind - - "Add Selected" `Button` → `AddDirectoryUsersCommand` - - "Load more" `Button` shown when `HasMoreDirectoryPages == true` → `LoadMoreDirectoryCommand` - - Loading indicator (existing `IsSearching` pattern, but for `IsLoadingDirectory`) -- Show/hide panels via `DataTrigger` on `IsBrowseModeActive` - -**`Views/Tabs/UserAccessAuditView.xaml.cs`** — Add `SelectionChanged` handler to pass `ListView.SelectedItems` (as `IList`) to `AddDirectoryUsersCommand`. Follow the existing `SearchResultsListBox_SelectionChanged` pattern. - -### Data Flow: Directory Browse Mode +## Component Dependency Map ``` -User clicks "Browse" mode toggle - → IsBrowseModeActive = true - → OnIsBrowseModeActiveChanged fires LoadDirectoryCommand - → GraphUserDirectoryService.GetUsersPageAsync(clientId, filter: null, pageToken: null, 100, ct) - → Graph GET /users?$select=displayName,userPrincipalName,mail&$top=100&$orderby=displayName - → returns PagedUserResult { Users = [...100 items], NextPageToken = "..." } - → DirectoryUsers = new collection of returned users - → HasMoreDirectoryPages = (NextPageToken != null) - → _directoryNextPageToken = returned token +NEW COMPONENT DEPENDS ON (existing unless marked new) +────────────────────────────────────────────────────────────────────────── +AppRegistrationResult (model) — none +AppSettings.AutoTakeOwnership AppSettings (existing model) +ScanOptions.ExpandGroupMembers ScanOptions (existing model) +PermissionEntry.GroupMembers PermissionEntry (existing record) -User types in DirectoryFilter - → debounce 300ms - → LoadDirectoryCommand re-fires with filter - → DirectoryUsers replaced with filtered page 1 +PermissionConsolidator PermissionEntry (existing) -User selects users in ListView + clicks "Add Selected" - → AddDirectoryUsersCommand(selectedItems) - → for each: if not already in SelectedUsers by UPN → SelectedUsers.Add(user) +IAppRegistrationService — +AppRegistrationService GraphServiceClient (existing via GraphClientFactory) + Microsoft.Graph SDK (existing) -User clicks "Load more" - → LoadMoreDirectoryCommand - → GetUsersPageAsync(clientId, currentFilter, _directoryNextPageToken, 100, ct) - → DirectoryUsers items appended (not replaced) - → _directoryNextPageToken updated +ISiteOwnershipService — +SiteOwnershipService SessionManager (existing) + TenantProfile (existing) + Tenant CSOM class (existing via PnP Framework) + SiteListService.DeriveAdminUrl pattern (existing) + +SettingsService (modified) AppSettings (existing + new field) + +PermissionsService (modified) ScanOptions.ExpandGroupMembers (new field) + ExecuteQueryRetryHelper (existing) + +HtmlExportService (modified) PermissionEntry.GroupMembers (new field) + BrandingHtmlHelper (existing) + +ProfileManagementViewModel (mod) IAppRegistrationService (new) +PermissionsViewModel (modified) ExpandGroupMembers, ConsolidateEntries, PermissionConsolidator +SettingsViewModel (modified) AutoTakeOwnership, SettingsService new method ``` --- -## Component Boundary Summary +## Suggested Build Order -### New Components (create) +Dependencies flow upward; each step can be tested before the next begins. -| Component | Layer | Type | Purpose | -|-----------|-------|------|---------| -| `BrandingSettings` | Core/Models | class | MSP logo storage (base64 + MIME type) | -| `ReportBranding` | Core/Models | record | Data passed to `BuildHtml` overloads at export time | -| `BrandingRepository` | Infrastructure/Persistence | class | JSON load/save for `BrandingSettings` | -| `BrandingService` | Services | class | Orchestrates logo file read / MIME detect / base64 convert / save | -| `PagedUserResult` | Core/Models | record | Page of `GraphUserResult` items + next-page token | -| `IGraphUserDirectoryService` | Services | interface | Contract for paginated tenant user enumeration | -| `GraphUserDirectoryService` | Services | class | Graph API user listing with cursor pagination | +### Step 1: Model additions +No external dependencies. All existing tests continue to pass. +- `AppRegistrationResult` record (new file) +- `AppSettings.AutoTakeOwnership` bool property (default false) +- `ScanOptions.ExpandGroupMembers` bool parameter (default false) +- `PermissionEntry.GroupMembers` optional string parameter (default null) -Total new files: 7 +### Step 2: Pure-logic helper +Fully unit-testable with no services. +- `PermissionConsolidator` in `Core/Helpers/` -### Modified Components (extend) +### Step 3: New services +Depend only on existing infrastructure (SessionManager, GraphClientFactory). +- `ISiteOwnershipService` + `SiteOwnershipService` +- `IAppRegistrationService` + `AppRegistrationService` -| Component | Change | Risk | -|-----------|--------|------| -| `TenantProfile` | + 2 nullable logo props | LOW — JSON backward-compatible | -| `HtmlExportService` | + optional `ReportBranding?` on 2 overloads | LOW — optional param, existing callers unaffected | -| `UserAccessHtmlExportService` | + optional `ReportBranding?` | LOW | -| `StorageHtmlExportService` | + optional `ReportBranding?` on 2 overloads | LOW | -| `SearchHtmlExportService` | + optional `ReportBranding?` | LOW | -| `DuplicatesHtmlExportService` | + optional `ReportBranding?` | LOW | -| `PermissionsViewModel` | + inject `BrandingService`, use in ExportHtmlAsync | LOW | -| `UserAccessAuditViewModel` | + inject `BrandingService` + `IGraphUserDirectoryService`, browse mode state/commands | MEDIUM | -| `StorageViewModel` | + inject `BrandingService`, use in ExportHtmlAsync | LOW | -| `SearchViewModel` | + inject `BrandingService`, use in ExportHtmlAsync | LOW | -| `DuplicatesViewModel` | + inject `BrandingService`, use in ExportHtmlAsync | LOW | -| `SettingsViewModel` | + inject `BrandingService`, MSP logo commands + preview property | LOW | -| `ProfileManagementViewModel` | + client logo browse/preview/clear | LOW | -| `SettingsView.xaml` | + branding section with logo preview + buttons | LOW | -| `ProfileManagementDialog.xaml` | + client logo fields | LOW | -| `UserAccessAuditView.xaml` | + mode toggle + browse panel in left column | MEDIUM | -| `App.xaml.cs RegisterServices()` | + 3 new registrations | LOW | +### Step 4: SettingsService extension +Thin method addition, no structural change. +- `SetAutoTakeOwnershipAsync(bool)` on existing `SettingsService` -Total modified files: 17 +### Step 5: PermissionsService modification +- Group member CSOM load in `ExtractPermissionsAsync` (guarded by `ExpandGroupMembers`) +- Access-denied retry using `SiteOwnershipService` (guarded by `AutoTakeOwnership`) + +### Step 6: Export service modifications +- `HtmlExportService.BuildHtml`: `
/` rendering for `GroupMembers` +- `UserAccessHtmlExportService.BuildHtml`: same for group access entries + +### Step 7: ViewModel modifications +- `SettingsViewModel`: `AutoTakeOwnership` property wired to `SettingsService` +- `PermissionsViewModel`: `ExpandGroupMembers`, `ConsolidateEntries`, updated `ScanOptions` +- `ProfileManagementViewModel`: `IAppRegistrationService` injection, `RegisterAppCommand`, `RemoveAppCommand`, guided fallback state + +### Step 8: View/XAML additions +- `SettingsView.xaml`: AutoTakeOwnership checkbox +- `PermissionsView.xaml`: ExpandGroupMembers checkbox, ConsolidateEntries checkbox +- `ProfileManagementDialog.xaml`: Register App button, Remove App button, guided fallback panel + +### Step 9: DI wiring (App.xaml.cs) +- Register `IAppRegistrationService`, `ISiteOwnershipService` +- `ProfileManagementViewModel` constructor change is picked up automatically (AddTransient) --- -## Build Order (Dependency-Aware) +## New vs. Modified Summary -The two features are independent of each other. Phases can run in parallel if worked by two developers; solo they should follow top-to-bottom order. +| Component | Status | Layer | +|-----------|--------|-------| +| `AppRegistrationResult` | NEW | Core/Models | +| `AppSettings.AutoTakeOwnership` | MODIFIED | Core/Models | +| `ScanOptions.ExpandGroupMembers` | MODIFIED | Core/Models | +| `PermissionEntry.GroupMembers` | MODIFIED | Core/Models | +| `PermissionConsolidator` | NEW | Core/Helpers | +| `IAppRegistrationService` | NEW | Services | +| `AppRegistrationService` | NEW | Services | +| `ISiteOwnershipService` | NEW | Services | +| `SiteOwnershipService` | NEW | Services | +| `SettingsService.SetAutoTakeOwnershipAsync` | MODIFIED | Services | +| `PermissionsService.ExtractPermissionsAsync` | MODIFIED | Services | +| `HtmlExportService.BuildHtml` | MODIFIED | Services/Export | +| `UserAccessHtmlExportService.BuildHtml` | MODIFIED | Services/Export | +| `ProfileManagementViewModel` | MODIFIED | ViewModels | +| `PermissionsViewModel` | MODIFIED | ViewModels/Tabs | +| `SettingsViewModel` | MODIFIED | ViewModels/Tabs | +| `ProfileManagementDialog.xaml` | MODIFIED | Views/Dialogs | +| `PermissionsView.xaml` | MODIFIED | Views/Tabs | +| `SettingsView.xaml` | MODIFIED | Views/Tabs | +| `App.xaml.cs RegisterServices` | MODIFIED | Root | -### Phase A — Data Models (no dependencies) -1. `Core/Models/BrandingSettings.cs` (new) -2. `Core/Models/ReportBranding.cs` (new) -3. `Core/Models/PagedUserResult.cs` (new) -4. `Core/Models/TenantProfile.cs` — add nullable logo props (modification) - -All files are POCOs/records. Unit-testable in isolation. No risk. - -### Phase B — Persistence + Service Layer -5. `Infrastructure/Persistence/BrandingRepository.cs` (new) — depends on BrandingSettings -6. `Services/BrandingService.cs` (new) — depends on BrandingRepository -7. `Services/IGraphUserDirectoryService.cs` (new) — depends on PagedUserResult -8. `Services/GraphUserDirectoryService.cs` (new) — depends on GraphClientFactory (already exists) - -Unit tests for BrandingService (mock repository) and GraphUserDirectoryService (mock Graph client) can be written at this phase. - -### Phase C — HTML Export Service Extensions -9. All 5 `Services/Export/*HtmlExportService.cs` modifications — add optional `ReportBranding?` param - -These are independent of each other. Tests: verify that passing `null` branding produces identical HTML to current output (regression), and that passing a branding record injects the expected `` tags. - -### Phase D — ViewModel Integration (branding) -10. `SettingsViewModel.cs` — add MSP logo commands + preview -11. `ProfileManagementViewModel.cs` — add client logo commands + preview -12. `PermissionsViewModel.cs` — add BrandingService injection, use in ExportHtmlAsync -13. `StorageViewModel.cs` — same -14. `SearchViewModel.cs` — same -15. `DuplicatesViewModel.cs` — same -16. `App.xaml.cs` — register BrandingRepository, BrandingService - -Steps 12-15 follow an identical pattern and can be batched together. - -### Phase E — ViewModel Integration (directory browse) -17. `UserAccessAuditViewModel.cs` — add IGraphUserDirectoryService injection, browse mode state/commands - -Note: UserAccessAuditViewModel also gets BrandingService at this phase (from Phase D pattern). Do both together to avoid touching the constructor twice. - -### Phase F — View Layer (branding UI) -18. `SettingsView.xaml` — add MSP branding section -19. `ProfileManagementDialog.xaml` — add client logo fields - -Requires a base64-to-BitmapSource `IValueConverter` (add to `Views/Converters/`). This is a common WPF pattern — implement once, reuse in both views. - -### Phase G — View Layer (directory browse UI) -20. `UserAccessAuditView.xaml` — add mode toggle + browse panel -21. `UserAccessAuditView.xaml.cs` — add SelectionChanged handler for directory ListView - -This is the highest-risk UI change: the left panel is being restructured. Do this last, after all ViewModel behavior is proven by unit tests. +**No new tabs. No new XAML files. No new dialog windows required.** All four features extend existing surfaces. --- -## Anti-Patterns to Avoid +## Critical Integration Notes -### Storing Logo Images as Separate Files -**Why bad:** Breaks the single-data-folder design. Reports become non-self-contained if they reference external paths. Atomic save semantics break. -**Instead:** Base64-encode into JSON. Logo thumbnails are typically 10-200KB. Base64 overhead (~33%) is negligible. +### App Registration: Permission Prerequisite +The auto-registration path requires `Application.ReadWrite.All` to be granted and admin-consented on the MSP's own client app registration. The tool cannot bootstrap this permission itself. The guided fallback path is the safe default — auto path is an enhancement for pre-prepared deployments. Catch `ODataError` with `ResponseStatusCode == 403` to trigger the fallback automatically. -### Adding an `IHtmlExportService` Interface Just for Branding -**Why bad:** The existing pattern is 5 concrete classes with no interfaces, consumed directly by ViewModels. Adding an interface for a parameter change creates ceremony without value. -**Instead:** Add `ReportBranding? branding = null` as optional parameter. Existing callers compile unchanged. +### Auto-Ownership: Retry Once, Not Infinitely +Retry exactly once per site. If the second attempt fails (account lacks tenant admin rights), propagate the original error with a clear message indicating that ownership take-over was attempted. Log both attempts via `ILogger`. -### Loading All Tenant Users at Once -**Why bad:** Enterprise tenants regularly have 20,000-100,000 users. A full load blocks the UI for 30+ seconds and allocates hundreds of MB. -**Instead:** `PagedUserResult` pattern — page 1 on mode toggle, "Load more" button, server-side filter applied to DirectoryFilter text. +### Group Expansion: Scan Performance Impact +Loading group members adds one CSOM round-trip per unique SharePoint group encountered. The `ExpandGroupMembers` toggle must default to `false` and be labeled clearly in the UI (e.g., "Expand group members in report (slower scan)"). On tenants with many groups across many sites, this could multiply scan time significantly. -### Async in ViewModel Constructor -**Why bad:** DI constructs ViewModels synchronously on the UI thread. Async work in constructors requires fire-and-forget which loses exceptions. -**Instead:** `partial void OnIsBrowseModeActiveChanged` fires `LoadDirectoryCommand` when browse mode activates. Constructor only wires up commands and state. +### Consolidation: Records Are Immutable +`PermissionEntry` is a `record`. `PermissionConsolidator.Consolidate()` produces new record instances — no mutation. Consistent with how `Results` is already replaced wholesale in `PermissionsViewModel`. -### Client Logo in `AppSettings` or `BrandingSettings` -**Why bad:** Client logos are per-tenant. `AppSettings` and `BrandingSettings` are global. Mixing them makes per-profile deletion awkward and serialization structure unclear. -**Instead:** `ClientLogoBase64` + `ClientLogoMimeType` directly on `TenantProfile` (serialized in `profiles.json`). MSP logo goes in `branding.json` via `BrandingRepository`. +### HTML `
/` Compatibility +Self-contained HTML reports target any modern browser. `
/` is fully supported without JavaScript since 2016 across all major browsers. This is the correct choice over adding onclick JS toggle logic. -### Changing `BuildHtml` Signatures to Required Parameters -**Why bad:** All 5 HTML export services currently have callers without branding. Making the parameter required is a breaking change forcing simultaneous updates across 5 VMs. -**Instead:** `ReportBranding? branding = null` is optional. Inject only where branding is desired. Existing call sites remain unchanged. - ---- - -## Scalability Considerations - -| Concern | Impact | Mitigation | -|---------|--------|------------| -| Logo storage size in JSON | PNG logos base64-encoded: 10-200KB per logo. `profiles.json` grows by at most that per tenant | Acceptable — config files, not bulk data | -| HTML report file size | +2-10KB per logo (base64 inline) | Negligible — reports are already 100-500KB | -| Directory browse load time | 100-user pages from Graph: ~200-500ms per page | Loading indicator, pagination. Acceptable UX. | -| Large tenants (50k+ users) | Full load would take minutes and exceed memory budgets | Pagination via `PagedUserResult` prevents this entirely | -| ViewModel constructor overhead | BrandingService adds one lazy JSON read at first export | Not at construction — no startup impact | +### No Breaking Changes to Existing Tests +All model changes use optional parameters with defaults. Existing test data and constructors remain valid. `PermissionConsolidator` and `SiteOwnershipService` are new testable units that can use the existing `InternalsVisibleTo` pattern for test access. --- ## Sources -All findings are based on direct inspection of the codebase at `C:/Users/dev/Documents/projets/Sharepoint/SharepointToolbox/`. No external research needed — this is an integration architecture document for a known codebase. - -Key files examined: -- `Core/Models/TenantProfile.cs`, `AppSettings.cs` -- `Infrastructure/Persistence/ProfileRepository.cs`, `SettingsRepository.cs` -- `Infrastructure/Auth/GraphClientFactory.cs` -- `Services/SettingsService.cs`, `ProfileService.cs` -- `Services/GraphUserSearchService.cs`, `IGraphUserSearchService.cs` -- `Services/Export/HtmlExportService.cs`, `UserAccessHtmlExportService.cs`, `StorageHtmlExportService.cs` -- `ViewModels/FeatureViewModelBase.cs`, `MainWindowViewModel.cs` -- `ViewModels/Tabs/UserAccessAuditViewModel.cs`, `SettingsViewModel.cs` -- `Views/Tabs/UserAccessAuditView.xaml`, `SettingsView.xaml` -- `App.xaml.cs` +- Microsoft Graph permissions reference: https://learn.microsoft.com/en-us/graph/permissions-reference +- Graph API grant permissions programmatically: https://learn.microsoft.com/en-us/graph/permissions-grant-via-msgraph +- PnP Core SDK site security (SetSiteCollectionAdmins): https://pnp.github.io/pnpcore/using-the-sdk/admin-sharepoint-security.html +- PnP Framework TenantExtensions source: https://github.com/pnp/PnP-Sites-Core/blob/master/Core/OfficeDevPnP.Core/Extensions/TenantExtensions.cs diff --git a/.planning/research/FEATURES.md b/.planning/research/FEATURES.md index d0e5dc9..533c994 100644 --- a/.planning/research/FEATURES.md +++ b/.planning/research/FEATURES.md @@ -1,211 +1,534 @@ # Feature Landscape -**Domain:** MSP IT admin desktop tool — SharePoint audit report branding + user directory browse -**Milestone:** v2.2 — Report Branding & User Directory -**Researched:** 2026-04-08 -**Overall confidence:** HIGH (verified via official Graph API docs + direct codebase inspection) +**Domain:** MSP IT admin desktop tool — Tenant Management & Report Enhancements +**Milestone:** v2.3 +**Researched:** 2026-04-09 +**Overall confidence:** HIGH (verified via official Graph API docs, PnP docs, and direct codebase inspection) --- ## Scope Boundary -This file covers only the two net-new features in v2.2: -1. HTML report branding (MSP logo + client logo per tenant) -2. User directory browse mode in the user access audit tab +This file covers only the five net-new features in v2.3: + +1. Automated app registration on target tenant (with guided fallback) +2. App removal from target tenant +3. Auto-take ownership of SharePoint sites on access denied (global toggle) +4. Expand groups in HTML reports (clickable to show members) +5. Report consolidation toggle (merge duplicate user entries across locations) Everything else is already shipped. Dependencies on existing code are called out explicitly. --- -## Feature 1: HTML Report Branding +## Feature 1: Automated App Registration on Target Tenant + +### What it is + +During profile creation/editing, the app can register itself as an Azure AD application on the +target tenant. This eliminates the current manual step where admins must open Entra portal, create +an app registration, copy the Client ID, and paste it into the profile form. + +### How it works (technical) + +Graph API app registration is a two-phase operation when performed programmatically: + +**Phase 1 — Create the Application object:** +`POST /applications` with `displayName` and optionally `requiredResourceAccess` (permission +declarations). Returns `appId` (client ID) and `id` (object ID). Requires delegated permission +`Application.ReadWrite.All` (least privilege for delegated scenarios), or the calling user must +hold a role of Application Developer, Cloud Application Administrator, or higher. + +**Phase 2 — Create the Service Principal:** +`POST /servicePrincipals` with `appId` from Phase 1. This is a required explicit step when +registering via Graph API — the portal creates the SP automatically, the API does not. +Requires the same `Application.ReadWrite.All` delegated permission. + +**Phase 3 — Grant admin consent for required permissions:** +`POST /servicePrincipals/{resourceId}/appRoleAssignedTo` for each application permission +(SharePoint, Graph scopes needed). The calling user must hold Cloud Application Administrator +or Global Administrator to grant tenant-wide consent. Requires delegated permissions +`Application.Read.All` + `AppRoleAssignment.ReadWrite.All`. + +**Phase 4 — Store credentials (optional):** +`POST /applications/{id}/addPassword` to create a client secret. The `secretText` is only +returned once at creation — must be stored immediately in the profile. Alternatively, the app +can use a MSAL public client flow (interactive login), which does not require a client secret. + +**Guided fallback:** If the calling user lacks Application.ReadWrite.All or admin consent cannot +be granted programmatically (e.g., tenant has restricted app consent policies), the automated +path fails. The fallback shows step-by-step instructions + a deep-link to the Entra portal app +registration wizard, with the `appId` field pre-fill-ready when the admin returns. ### Table Stakes -Features an MSP admin expects without being asked. If missing, the reports feel unfinished and -unprofessional to hand to a client. - | Feature | Why Expected | Complexity | Notes | |---------|--------------|------------|-------| -| MSP global logo in report header | Every white-label MSP tool shows the MSP's own brand on deliverables | Low | Single image stored in AppSettings or a dedicated branding settings section | -| Client (per-tenant) logo in report header | MSP reports are client-facing; client should see their own logo next to the MSP's | Medium | Stored in TenantProfile; 2 sources: import from file or pull from tenant | -| Logo renders in self-contained HTML (no external URL) | Reports are often emailed or archived; external URLs break offline | Low | Base64-encode and embed as `data:image/...;base64,...` inline in `` block entirely when no logo is set | -| Consistent placement across all HTML export types | App already ships 5+ HTML exporters; logos must appear in all of them | Medium | Extract a shared header-builder method or inject a branding context into each export service | +| Create app registration on target tenant via Graph API | MSPs manage 10-50 tenants; manual Entra portal steps per-tenant is the biggest onboarding friction | High | 4 API calls; requires `Application.ReadWrite.All` + admin consent grant scope in the calling token | +| Return and store the Client ID automatically | The resulting `appId` must be wired into the TenantProfile as the registered clientId | Low | Phase 1 response body contains `appId`; persist to TenantProfile.ClientId | +| Guided fallback (manual instructions + portal deep-link) if automated path fails | Tenant admin consent policies may block programmatic app creation | Medium | Detect 403/insufficient_scope errors; render a modal with numbered steps and a link to `https://entra.microsoft.com/#blade/Microsoft_AAD_RegisteredApps/CreateApplicationBlade` | +| Progress/status feedback during multi-step registration | 4 API calls; each can fail independently | Low | Use existing OperationProgress pattern; surface per-step status in the UI | ### Differentiators -Features not expected by default, but add meaningful value once table stakes are covered. - | Feature | Value Proposition | Complexity | Notes | |---------|-------------------|------------|-------| -| Auto-pull client logo from Microsoft Entra tenant branding | Zero-config for tenants that already have a banner logo set in Entra ID | Medium | Graph API: `GET /organization/{id}/branding/localizations/default/bannerLogo` returns raw image bytes. Least-privileged scope is `User.Read` (delegated, already in use). Returns empty body or 404 when not configured — must handle gracefully. | -| Report timestamp and tenant display name in header | Contextualizes archived reports without needing to inspect the filename | Low | TenantProfile.TenantUrl already available; display name derivable from domain | +| Pre-configure required Graph/SharePoint permissions in the app manifest | Avoids admin having to manually tick permissions in Entra portal after creation | Medium | Include `requiredResourceAccess` in POST /applications body, targeting Graph SP (appId `00000003-0000-0000-c000-000000000000`) and SharePoint SP (appId `00000003-0000-0ff1-ce00-000000000000`) | +| Verify existing registration before creating a new one | Prevents duplicate registrations on re-run or retry | Low | `GET /applications?$filter=displayName eq '{name}'` before POST; surface existing one if found | ### Anti-Features -Do not build these. They add scope without proportionate MSP value. - | Anti-Feature | Why Avoid | What to Do Instead | |--------------|-----------|-------------------| -| Color theme / CSS customization per tenant | Complexity explodes — per-tenant CSS is a design system problem, not an admin tool feature | Stick to a single professional neutral theme; logo is sufficient branding | -| PDF export with embedded logo | PDF generation requires a third-party library (iTextSharp, QuestPDF, etc.) adding binary size to the 200 MB EXE | Document in release notes that users can print-to-PDF from browser | -| Animated or SVG logo support | MIME handling complexity; SVG in data-URIs introduces XSS risk | Support PNG/JPG/GIF only; reject SVG at import time | -| Logo URL field (hotlinked) | Reports break when URL becomes unavailable; creates external dependency for a local-first tool | Force file import with base64 embedding | +| Store client secrets in the profile JSON | Secrets at rest in a local JSON file are a liability; the app uses delegated (interactive) auth, not app-only | Use MSAL interactive delegated flow; no client secret needed at runtime | +| Certificate-based credential management | Cert lifecycle (expiry, rotation) is out of scope for an MSP admin tool | Interactive user auth handles token refresh automatically | +| Silent background retry on consent failures | The calling user may not have the right role; silent retry without user action would spin indefinitely | Detect error class, surface actionable UI immediately | ### Feature Dependencies ``` -AppSettings + MspLogoBase64 (string?, nullable) -TenantProfile + ClientLogoBase64 (string?, nullable) - + ClientLogoSource (enum: None | Imported | AutoPulled) -Shared branding helper → called by HtmlExportService, UserAccessHtmlExportService, - StorageHtmlExportService, DuplicatesHtmlExportService, - SearchHtmlExportService -Auto-pull code path → Graph API call via existing GraphClientFactory -Logo import UI → WPF OpenFileDialog -> File.ReadAllBytes -> Convert.ToBase64String - -> stored in profile JSON via existing ProfileRepository +Existing: + GraphClientFactory → provides authenticated GraphServiceClient for target tenant + TenantProfile.ClientId → stores the resulting appId after registration + ProfileManagementDialog → hosts the registration trigger button + OperationProgress → used for per-step status display + +New: + IAppRegistrationService / AppRegistrationService + → CreateApplicationAsync(tenantId, displayName) : Task + → CreateServicePrincipalAsync(appId) : Task + → GrantAdminConsentAsync(servicePrincipalId) : Task (app role assignments) + → RemoveApplicationAsync(appId) : Task (Feature 2) + AppRegistrationFallbackDialog (new WPF dialog) + → renders numbered steps when automated path fails + → deep-link button to Entra portal ``` -**Key existing code note:** All 5+ HTML export services currently build their `` independently -with no shared header. Branding requires one of: -- (a) a `ReportBrandingContext` record passed into each exporter's `BuildHtml` method, or -- (b) a `HtmlReportHeaderBuilder` static/injectable helper all exporters call. - -Option (b) is lower risk — it does not change method signatures that existing unit tests already call. +**Key existing code note:** GraphClientFactory already acquires delegated tokens with the +tenant's registered clientId. For the registration flow specifically, the app needs a token +scoped to the *management* tenant (where Entra lives), not just SharePoint/Graph read scopes. +The MSAL PCA must request `Application.ReadWrite.All` and `AppRoleAssignment.ReadWrite.All` +for the registration step — these are broader than the app's normal operation scopes and will +trigger a new consent prompt if not previously consented. ### Complexity Assessment | Sub-task | Complexity | Reason | |----------|------------|--------| -| AppSettings + TenantProfile model field additions | Low | Trivial nullable-string fields; JSON serialization already in place | -| Settings UI: MSP logo upload + preview | Low | WPF OpenFileDialog + BitmapImage from base64, standard pattern | -| ProfileManagementDialog: client logo upload per tenant | Low | Same pattern as MSP logo | -| Shared HTML header builder with logo injection | Low-Medium | One helper; replaces duplicated header HTML in 5 exporters | -| Auto-pull from Entra `bannerLogo` endpoint | Medium | Async Graph call; must handle 404, empty stream, no branding configured | -| Localization keys EN/FR for new labels | Low | ~6-10 new keys; 220+ already managed | +| POST /applications + POST /servicePrincipals | Medium | Two sequential calls; error handling at each step | +| Grant admin consent (appRoleAssignment per permission) | High | Must look up resource SP IDs, match appRole GUIDs by permission name; 4-6 role assignments needed | +| TenantProfile.ClientId persistence after registration | Low | Existing JSON serialization; add one field | +| UI: Register button in ProfileManagementDialog | Low | Button + status label; hooks into existing async command pattern | +| Guided fallback modal | Medium | Error detection logic + WPF dialog with instructional content | +| Localization EN/FR | Low | ~12-16 new keys | +| Unit tests for AppRegistrationService | High | Requires mocking Graph SDK Application/ServicePrincipal/AppRoleAssignment calls; 15-20 test cases | --- -## Feature 2: User Directory Browse Mode +## Feature 2: App Removal from Target Tenant + +### What it is + +Inverse of Feature 1. When a tenant profile is deleted or when the admin explicitly removes the +registration, the app deletes the Azure AD application object from the target tenant. + +### How it works (technical) + +`DELETE /applications/{id}` — soft-deletes the application object (moved to deleted items for +30 days). Requires `Application.ReadWrite.All` delegated. The `{id}` here is the object ID +(not the `appId`/client ID) — must be resolved first via +`GET /applications?$filter=appId eq '{clientId}'`. + +Optionally: `DELETE /directory/deletedItems/{id}` for permanent deletion (requires +`Application.ReadWrite.All`; irreversible within the 30-day window — avoid). ### Table Stakes -Features an admin expects when a "browse all users" mode is offered alongside the existing search. - | Feature | Why Expected | Complexity | Notes | |---------|--------------|------------|-------| -| Full directory listing (all member users, paginated) | Browse implies seeing everyone, not just name-search hits | Medium | Graph `GET /users` with `$top=100`, follow `@odata.nextLink` until null. Max page size is 999 but 100 pages give better progress feedback | -| Searchable/filterable within the loaded list | Once loaded, admins filter locally without re-querying | Low | In-memory filter on DisplayName, UPN, Mail — same pattern used in PermissionsView DataGrid | -| Sortable columns (Name, UPN) | Standard expectation for any directory table | Low | WPF DataGrid column sorting, already used in other tabs | -| Select user from list to run access audit | The whole point — browse replaces the people-picker for users the admin cannot spell | Low | Bind selected item; reuse the existing IUserAccessAuditService pipeline unchanged | -| Loading indicator with progress count | Large tenants (5k+ users) take several seconds to page through | Low | Existing OperationProgress pattern; show "Loaded X users..." counter | -| Toggle between Browse mode and Search (people-picker) mode | Search is faster for known users; browse is for discovery | Low | RadioButton or ToggleButton in the tab toolbar; visibility-toggle two panels | - -### Differentiators - -| Feature | Value Proposition | Complexity | Notes | -|---------|-------------------|------------|-------| -| Filter by account type (member vs guest) | MSPs care about guest proliferation; helps scope audit targets | Low | Graph returns `userType` field; add a toggle filter. Include in `$select` | -| Department / Job Title columns | Helps identify the right user in large tenants with common names | Low-Medium | Include `department`, `jobTitle` in `$select`; optional columns in DataGrid | -| Session-scoped directory cache | Avoids re-fetching full tenant list on every tab visit | Medium | Store list in ViewModel or session-scoped service; invalidate on TenantSwitchedMessage | +| Remove app registration when profile is deleted | Avoid Entra app sprawl in client tenants; clean exit | Medium | Resolve object ID by appId, then DELETE /applications/{id} | +| Confirmation prompt before removal | Deletion is irreversible within the session; accidental removal would break other automations using the same app | Low | Modal confirm dialog with clientId displayed | +| Graceful handling when app no longer exists | Re-run, manual deletion in portal, or already removed | Low | Handle 404 as success (idempotent delete) | ### Anti-Features | Anti-Feature | Why Avoid | What to Do Instead | |--------------|-----------|-------------------| -| Eager load on tab open | Large tenants (10k+ users) block UI and risk Graph throttling on every tab navigation | Lazy-load on explicit "Load Directory" button click; show a clear affordance | -| Delta query / incremental sync | Delta queries are for maintaining a local replica over time; wrong pattern for a one-time audit session | Single paginated GET per session; add a Refresh button | -| Multi-user bulk select for simultaneous audit | The audit pipeline is per-user by design; multi-user requires a fundamentally different results model | Out of scope; single-user selection only | -| Export the user directory to CSV | That is an identity reporting feature (AdminDroid et al.), not an access audit feature | Out of scope for this milestone | -| Show disabled accounts by default | Disabled users do not have active SharePoint access; pollutes the list for audit purposes | Default `$filter=accountEnabled eq true`; optionally expose a toggle | +| Permanent hard-delete from deletedItems | 30-day soft-delete is a safety net; no MSP needs immediate permanent removal | Soft-delete only (default DELETE /applications behavior) | +| Auto-remove on profile deletion without prompt | Silent data destruction is never acceptable | Always require explicit user confirmation | ### Feature Dependencies ``` -New IGraphDirectoryService + GraphDirectoryService - → GET /users?$select=displayName,userPrincipalName,mail,jobTitle,department,userType - &$filter=accountEnabled eq true - &$top=100 - → Follow @odata.nextLink in a loop until null - → Uses existing GraphClientFactory (DI, unchanged) +Existing: + AppRegistrationService (from Feature 1) + + RemoveApplicationAsync(clientId) : Task + → GET /applications?$filter=appId eq '{clientId}' → resolve object ID + → DELETE /applications/{objectId} -UserAccessAuditViewModel additions: - + IsBrowseMode (bool property, toggle) - + DirectoryUsers (ObservableCollection or new DirectoryUserEntry model) - + DirectoryFilterText (string, filters in-memory) - + LoadDirectoryCommand (async, cancellable) - + IsDirectoryLoading (bool) - + SelectedDirectoryUser → feeds into existing audit execution path - -TenantSwitchedMessage handler in ViewModel: clear DirectoryUsers, reset IsBrowseMode - -UserAccessAuditView.xaml: - + Toolbar toggle (Search | Browse) - + Visibility-collapsed people-picker panel when in browse mode - + New DataGrid panel for browse mode +ProfileManagementDialog + → Remove Registration button (separate from Delete Profile) + → or confirmation step during profile deletion flow ``` -**Key existing code note:** `GraphUserSearchService` does filtered search only (`startsWith` filter + -`ConsistencyLevel: eventual`). Directory listing is a different call pattern — no filter, plain -pagination without `ConsistencyLevel`. A separate `GraphDirectoryService` is cleaner than extending -the existing service; search and browse have different cancellation and retry needs. - ### Complexity Assessment | Sub-task | Complexity | Reason | |----------|------------|--------| -| IGraphDirectoryService + GraphDirectoryService (pagination loop) | Low-Medium | Standard Graph paging; same GraphClientFactory in DI | -| ViewModel additions (browse toggle, load command, filter, loading state) | Medium | New async command with progress, cancellation on tenant switch | -| View XAML: toggle + browse DataGrid panel | Medium | Visibility-toggle two panels; DataGrid column definitions | -| In-memory filter + column sort | Low | DataGrid pattern already used in PermissionsView | -| Loading indicator integration | Low | OperationProgress + IsLoading used by every tab | -| Localization keys EN/FR | Low | ~8-12 new keys | -| Unit tests for GraphDirectoryService | Low | Same mock pattern as GraphUserSearchService tests | -| Unit tests for ViewModel browse mode | Medium | Async load command, pagination mock, filter behavior | +| RemoveApplicationAsync (resolve then delete) | Low-Medium | Two calls; 404 idempotency handling | +| Confirmation dialog | Low | Reuse existing ConfirmationDialog pattern | +| Wire into profile deletion flow | Low | Existing profile delete command; add optional app removal step | + +--- + +## Feature 3: Auto-Take Ownership on Access Denied (Global Toggle) + +### What it is + +When the scanner hits a site with "Access Denied", and the global toggle is on, the app +automatically adds the scanning account as a Site Collection Administrator for that site, retries +the scan, and (optionally) removes itself afterward. The admin controls this with a global on/off. + +### How it works (technical) + +**Option A — PnP Framework (preferred):** +`context.Web.Context.Site.Owner = user` combined with +`Tenant.SetSiteProperties(siteUrl, owners: loginName)` or +`SPOTenantContext.SetSiteAdmin(siteUrl, loginName, isAdmin: true)` — all available via +`PnP.Framework` which is already a project dependency. + +The calling account must hold the SharePoint Administrator role at the tenant level (not just +site admin) to add itself as site collection admin to a site it is currently denied from. + +**Option B — Graph API:** +`POST /sites/{siteId}/permissions` with `roles: ["owner"]` — grants the service principal owner +access to a specific site. This works for application permissions with Sites.FullControl.All +but requires additional Graph permission scopes not currently in use. + +**Recommended:** PnP Framework path (Option A) because: +- PnP.Framework is already a dependency (no new package) +- The app already uses delegated PnP context for all SharePoint operations +- `Tenant.SetSiteAdmin` is a single method call, well-understood in the MSP ecosystem +- Graph site permissions path requires Sites.FullControl.All which is a very broad app permission + +**Self-healing sequence:** +1. Site scan returns 401/403 +2. If toggle is ON: call `SetSiteAdmin(siteUrl, currentUserLogin, isAdmin: true)` +3. Retry the scan operation +4. If auto-remove-after is ON: call `SetSiteAdmin(siteUrl, currentUserLogin, isAdmin: false)` +5. Log the takeover action (site, timestamp, user) for audit trail + +### Table Stakes + +| Feature | Why Expected | Complexity | Notes | +|---------|--------------|------------|-------| +| Global toggle in Settings to enable/disable auto-ownership | Some MSPs want this; others consider it too aggressive for compliance reasons | Low | Boolean field in AppSettings; surfaced in Settings tab | +| Take ownership on 401/403 and retry the scan | The core capability; without the retry it is pointless | Medium | Error interception in the scan pipeline; conditional branch | +| Audit log of takeover actions | Compliance requirement — admin must know which sites were temporarily owned | Low | Extend existing Serilog logging; optionally surface in the scan results list | +| Respect the toggle per-scan-run (not retroactive) | Some scans are read-only audits; the toggle state at run-start should be captured | Low | Capture AppSettings.AutoTakeOwnership at scan start; pass through as scan context | + +### Differentiators + +| Feature | Value Proposition | Complexity | Notes | +|---------|-------------------|------------|-------| +| Auto-remove ownership after scan completes | Least-privilege principle; the scanning account should not retain admin rights beyond the scan | Medium | Track which sites were auto-granted; remove in a finally block or post-scan cleanup step | +| Per-scan results column showing "Ownership Taken" flag | Transparency — admin sees which sites required escalation | Low | Add a flag to the ScanResultItem model; render as icon/badge in the results DataGrid | + +### Anti-Features + +| Anti-Feature | Why Avoid | What to Do Instead | +|--------------|-----------|-------------------| +| Auto-take enabled by default | Too aggressive for compliance-conscious MSPs; could violate client change-control policies | Default OFF; require explicit opt-in | +| Permanently retain ownership | Violates least-privilege; creates audit exposure for the MSP | Always remove after scan unless admin explicitly retains | +| Silent ownership changes with no audit trail | Undiscoverable by the client tenant's own admins | Log every takeover with timestamp and account UPN | + +### Feature Dependencies + +``` +Existing: + AppSettings + AutoTakeOwnership (bool, default false) + + AutoRemoveOwnershipAfterScan (bool, default true) + IPnPContextFactory → provides PnP context for Tenant-level operations + PermissionsScanService → where 401/403 errors currently surface per site + ScanResultItem → add OwnershipTakenFlag (bool) + +New: + ISiteOwnershipService / SiteOwnershipService + → TakeOwnershipAsync(siteUrl, loginName) : Task + → RemoveOwnershipAsync(siteUrl, loginName) : Task + → Uses PnP.Framework Tenant.SetSiteAdmin + +ScanContext record (or existing scan parameters) + → Carry AutoTakeOwnership bool captured at scan-start +``` + +**Key existing code note:** The existing BulkOperationRunner pattern handles per-item continue-on- +error. The ownership-takeover path should not be grafted into BulkOperationRunner directly; +instead it wraps the per-site scan call with a retry decorator that intercepts 401/403 and +invokes SiteOwnershipService before retrying. + +### Complexity Assessment + +| Sub-task | Complexity | Reason | +|----------|------------|--------| +| AppSettings fields + Settings UI toggle | Low | Trivial bool fields; existing settings pattern | +| ISiteOwnershipService + PnP SetSiteAdmin calls | Low-Medium | Well-known PnP API; success/failure handling | +| Error interception + retry in scan pipeline | Medium | Must not break existing error reporting; retry must not loop on non-permissions errors | +| Auto-remove in finally block after scan | Medium | Must track which sites were granted and clean up even if scan errors | +| Audit log / results column | Low | Extend existing model + DataGrid | +| Localization EN/FR | Low | ~8-10 new keys | +| Unit tests | High | Retry logic + cleanup path requires careful mock setup; ~15 test cases | + +--- + +## Feature 4: Expand Groups in HTML Reports + +### What it is + +In HTML permission reports, security groups currently appear as a flat entry (e.g., "IT Team — Edit"). +With this feature, each group row has an expand/collapse toggle that shows its members inline, +without leaving the report page. The expanded members list is embedded at report-generation time +(not lazy-loaded via an API call when the report is opened). + +### How it works (technical) + +At report generation time, for each permission entry that is a group: +1. Resolve group membership: `GET /groups/{id}/members?$select=displayName,userPrincipalName` +2. Embed member data inline in the HTML as a hidden `` or `
` with a stable CSS class +3. Emit a small inline `