docs: complete v2.3 project research (STACK, FEATURES, ARCHITECTURE, PITFALLS)
Research covers all five v2.3 features: automated app registration, app removal, auto-take ownership, group expansion in HTML reports, and report consolidation toggle. No new NuGet packages required. Build order and phase implications documented. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Dev
@@ -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,...` `<img>` 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<IAppRegistrationService, AppRegistrationService>();
|
||||
```
|
||||
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<BrandingSettings> 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 `<body>` open and `<h1>`:
|
||||
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<OperationProgress>` 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<ISiteOwnershipService, SiteOwnershipService>();
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 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 `<details>/<summary>` expandable block. The `<details>/<summary>` element requires zero JavaScript, is self-contained, and is universally supported in all modern browsers (Chrome, Edge, Firefox, Safari) since 2016.
|
||||
|
||||
```html
|
||||
<div class="brand-header" style="display:flex;align-items:center;gap:16px;padding:16px 24px 0;">
|
||||
<!-- only rendered if logo present -->
|
||||
<img src="data:{mimeType};base64,{base64}" style="max-height:60px;max-width:200px;" alt="MSP" />
|
||||
<img src="data:{mimeType};base64,{base64}" style="max-height:60px;max-width:200px;" alt="Client" />
|
||||
</div>
|
||||
<details class="group-expand">
|
||||
<summary class="user-pill group-pill">Members Group Name</summary>
|
||||
<div class="group-members">
|
||||
<span class="user-pill">alice@contoso.com</span>
|
||||
<span class="user-pill">bob@contoso.com</span>
|
||||
</div>
|
||||
</details>
|
||||
```
|
||||
|
||||
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:
|
||||
- `<Image>` 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 <img> 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<PermissionEntry>`. 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<GraphUserResult> Users,
|
||||
string? NextPageToken); // null = last page
|
||||
```
|
||||
|
||||
**`Services/IGraphUserDirectoryService.cs`**
|
||||
```csharp
|
||||
public interface IGraphUserDirectoryService
|
||||
// Core/Helpers/PermissionConsolidator.cs
|
||||
public static class PermissionConsolidator
|
||||
{
|
||||
Task<PagedUserResult> GetUsersPageAsync(
|
||||
string clientId,
|
||||
string? filter = null,
|
||||
string? pageToken = null,
|
||||
int pageSize = 100,
|
||||
CancellationToken ct = default);
|
||||
public static IReadOnlyList<PermissionEntry> Consolidate(
|
||||
IReadOnlyList<PermissionEntry> 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<GraphUserResult> _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<IList<GraphUserResult>> 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<IGraphUserDirectoryService, GraphUserDirectoryService>();
|
||||
```
|
||||
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<GraphUserResult>`) 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`: `<details>/<summary>` 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 `<img>` 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 `<details>/<summary>` Compatibility
|
||||
Self-contained HTML reports target any modern browser. `<details>/<summary>` 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
|
||||
|
||||
Reference in New Issue
Block a user