From fa36d139d3a11ebc813db135d1b62f5f604878e7 Mon Sep 17 00:00:00 2001 From: Daniel Bohlin Date: Sun, 10 May 2026 19:49:10 +0200 Subject: [PATCH 1/3] feat: invited-member-edit-fix complete --- plan/feature.md | 118 ------------------------------------------------ plan/plan.md | 44 ------------------ 2 files changed, 162 deletions(-) delete mode 100644 plan/feature.md delete mode 100644 plan/plan.md diff --git a/plan/feature.md b/plan/feature.md deleted file mode 100644 index 0252696..0000000 --- a/plan/feature.md +++ /dev/null @@ -1,118 +0,0 @@ -# Feature: Stable Member.Key + name promotion + user self-edit - -Filed in `Requests.md` 2026-05-10 ("TeamComponent inline name-edit broken for invited members"). Scope expanded during planning to include (a) name promotion on invitation accept, and (b) user self-edit of their own member row that propagates to `User.Name`. - -## Problem - -Invited (not-yet-accepted) team members are commonly created with `Member.Key = null` (the canonical pattern in the sample's `CreateTeamMember`). Two downstream effects: - -1. **UI** — the inline name-edit gate uses `_editingMemberKey == context.Key`. When `_editingMemberKey` is reset to `null` after Cancel/Save, `null == null` matches indefinitely → edit mode sticks; opening one invited row's editor puts every invited row into edit mode at once. -2. **Server** — `TeamRepository.SetMemberNameAsync` does `team.Members.Single(x => x.Key == userKey)` and `.Where(x => x.Key != userKey)`. Multiple invited members (all `Key=null`) cause `.Single` to throw and `.Where` to strip every null-keyed sibling on save. - -Separately, the admin-entered invitation name (`Member.Name`) is the only place a new user's display name is captured before the user signs in. Today, on accept, that name stays as a per-team override on `Member.Name` and is never promoted to `User.Name`. If the IdP doesn't supply a name, the user shows up as "Unknown" everywhere except the inviting team. - -A third gap: today only callers with `team:manage` can edit the inline name. A user has no way to update their own display name from the team UI — they'd need a separate "edit profile" page that doesn't exist. Editing your own name should be a self-service action that updates the global `User.Name` (the truth) and clears the per-team override in the team where the edit happened. - -## Goal - -1. Eliminate the null-`Member.Key` anomaly: invited members get a stable, generated key from creation. -2. On accept, promote the admin-entered `Member.Name` into `User.Name` if the user has no name yet, then clear the per-team override. The IdP claim wins when present; admin's invitation name is the fallback; per-team override goes away once the user has a real identity. -3. Allow users to edit their own member row. Self-edit always overwrites `User.Name` and clears `Member.Name` in the current team. Other teams retain whatever override their admins set. - -## Scope - -In-scope: - -1. **Auto-generate `Member.Key` on invitation** — `TeamServiceRepositoryBase.AddTeamMemberAsync`: if `memberModel.Key` is null/empty, assign `Guid.NewGuid().ToString()`. Symmetric to existing `Invitation` and `State` defaults in the same method. -2. **Promote Member.Name → User.Name on accept (only-if-empty rule)** — at acceptance, if `User.Name` is currently null/empty, seed it with the captured `Member.Name`. Always clear `Member.Name` on accept regardless. -3. **User self-edit own row** — show the inline name-edit pencil on the current user's own member row regardless of `team:manage` (so any user can edit their own name). On save: always overwrite `User.Name`, always clear `Member.Name` in the current team. Admin-edit on other rows is unchanged (writes per-team override). -4. **UI defensive gate (legacy data)** — `TeamComponent.razor`: change `_editingMemberKey == context.Key` → `!string.IsNullOrEmpty(_editingMemberKey) && _editingMemberKey == context.Key`. Belt-and-suspenders for any team document that still has null-keyed members from before this fix. - -Confirmed during planning, no change needed: - -- `TeamRepository.SetInvitationResponseAsync` already swaps `Key = userKey` on accept (and on reject, with state=Rejected). - -Out of scope: - -- Reset-button polish for invited rows. (User declined.) -- Migration of existing data with `Key=null`. (UI gate prevents the symptom; existing nulls heal naturally as invitations flow through accept/reject.) -- Server-side `.Single` resilience inside `TeamRepository.SetMemberNameAsync`. (Once `Member.Key` is unique by construction the existing `.Single` is correct.) -- Per-team alias for self (i.e., a user picking different names per team). The chosen design treats self-edit as global identity. -- Separate "Edit profile" page outside `TeamComponent`. The team grid is the only edit surface delivered here. - -## Approach - -### A. Auto-generate Member.Key (low-risk) - -In `Tharga.Team.MongoDB/TeamServiceRepositoryBase.cs`, inside `AddTeamMemberAsync`, after `CreateTeamMember` and adjacent to the existing `Invitation` / `State` auto-generation: - -```csharp -if (string.IsNullOrEmpty(memberModel.Key)) -{ - memberModel = memberModel with { Key = Guid.NewGuid().ToString() }; -} -``` - -### B. Name promotion on accept (cross-repo bridge) - -The plumbing must reach across the team-repo and user-repo boundaries. Chosen path: route via `IUserService` so `TeamServiceRepositoryBase`'s constructor signature does not change. - -1. **`Tharga.Team.MongoDB/IUserRepository.cs`** — add two methods: - - `Task GetByKeyAsync(string userKey)` — lookup-by-Key (today only `GetAsync(identity)` exists). - - `Task UpdateAsync(TUserEntity user)` — replace the whole user document. - -2. **`Tharga.Team.MongoDB/UserRepository.cs`** — implement both via `_collection.GetOneAsync(x => x.Key == userKey)` and `_collection.ReplaceOneAsync` respectively. - -3. **`Tharga.Team/IUserService.cs`** — add two methods (different semantics): - - `Task SeedUserNameAsync(string userKey, string name)` — only-if-empty. Used by `TeamServiceBase` on invitation-accept. - - `Task SetUserNameAsync(string userKey, string name)` — always overwrites. Used by user self-edit. - -4. **`Tharga.Team/UserServiceBase.cs`** — provide virtual `Task.CompletedTask` no-op defaults for both so consumers extending the base without overriding aren't broken. - -5. **`Tharga.Team.MongoDB/UserServiceRepositoryBase.cs`** — override both: - - Shared helper: `var user = await _userRepository.GetByKeyAsync(userKey);` if null → log warning + return. - - `SeedUserNameAsync`: if `!string.IsNullOrEmpty(user.Name)` → return (only-if-empty). Else write + cache invalidate. - - `SetUserNameAsync`: always write (`user with { Name = name }` → `UpdateAsync`) + cache invalidate. - -6. **`Tharga.Team/TeamServiceBase.cs`** — orchestrate in `SetInvitationResponseAsync`: - - On `accept == true`: pre-fetch the team, find the member with `Invitation.InviteKey == inviteKey`, capture `member.Name` as `seedName`. - - Call existing `SetTeamMemberInvitationResponseAsync(teamKey, userKey, inviteKey, true)` — already swaps `Key = userKey` and clears `Invitation`. **Update** the underlying `TeamRepository.SetInvitationResponseAsync` to also set `Name = null` on accept (1-line addition to the existing `with` block). - - If `seedName` is non-empty, `await _userService.SeedUserNameAsync(userKey, seedName)`. - -### C. User self-edit own row (UI + orchestration) - -In `Tharga.Team.Blazor/Features/Team/TeamComponent.razor`: - -1. **Pencil visibility** — change the gate from `EnableMemberNameEdit && _canManage` to `EnableMemberNameEdit && (_canManage || context.Key == _user.Key)` so a non-admin user sees the pencil on their own row only. -2. **Save behaviour** — `SaveMemberName` branches: - - If `member.Key == _user.Key` (self-edit): `await UserService.SetUserNameAsync(_user.Key, newName); await TeamManagementService.SetMemberNameAsync(team.Key, _user.Key, null);` Then reload + fire `OnMemberNameChanged` with the (cleared) override and the new identity. - - Else (admin-edit on another row): unchanged — `await TeamManagementService.SetMemberNameAsync(...)` writes the per-team override. -3. **Reset button** — hide for self-edit (`context.Key == _user.Key && !_canManage`); for self there is no override-vs-default distinction. - -### D. UI defensive gate - -In `Tharga.Team.Blazor/Features/Team/TeamComponent.razor`, line ~65: - -```razor -@if (EnableMemberNameEdit && (_canManage || context.Key == _user.Key) && !string.IsNullOrEmpty(_editingMemberKey) && _editingMemberKey == context.Key) -``` - -(Note: the visibility changes from C.1 and the null guard are merged into a single condition.) - -## Acceptance criteria - -- New invited members in MongoDB carry a non-null `Member.Key` (manual verify on sample app). -- Inline name-edit on an invited member returns the row to read-only display after save (manual verify). -- Multiple invited members do not all enter edit mode together when one is opened (manual verify). -- On invitation accept: `User.Name` is seeded from `Member.Name` if previously empty; never overwritten if present. `Member.Name` is cleared on accept regardless. (Manual verify with a fresh user; unit tested for the orchestration logic.) -- A non-admin user can edit their own row in `` and the change updates `User.Name` globally + clears `Member.Name` in the current team. Other teams' admin-set overrides for that user are unaffected. (Manual verify across two teams; unit tested.) -- `dotnet build -c Release` clean. -- `dotnet test -c Release` — all existing tests still pass; new orchestration tests added in `Tharga.Team.Service.Tests` for: invitation-accept seed flow (called when prior name was set, NOT called when empty, NOT called on reject), user self-edit branch (calls `SetUserNameAsync` + clears Member.Name) vs admin-edit branch (calls only `SetMemberNameAsync`). -- Bundled with the close-out commit from #61. - -## Done condition - -User confirms manually on the sample app: -- A new invitation creates a member with non-null `Key` and non-cleared `Name` while invited. -- After accept (with a fresh user whose IdP returns no name), `User.Name` carries the admin-entered invitation name and `Member.Name` is null. -- After accept (with a user whose IdP already returns a name), `User.Name` is unchanged and `Member.Name` is null. diff --git a/plan/plan.md b/plan/plan.md deleted file mode 100644 index a66691e..0000000 --- a/plan/plan.md +++ /dev/null @@ -1,44 +0,0 @@ -# Plan: Stable Member.Key + name promotion + user self-edit - -Branch: `feature/invited-member-edit-fix` (already carries the close-out commit `69c8226 feat: teammember-duplicate-fix complete` — bundled per user preference to avoid an extra PR). - -## Steps - -- [x] **1. Confirm plan with user.** Done. -- [x] **2. Auto-generate `Member.Key` in `AddTeamMemberAsync`.** `TeamServiceRepositoryBase.cs` — symmetric block alongside existing Invitation/State defaults. -- [x] **3. Extend `IUserRepository`.** Added `GetByKeyAsync(string userKey)` and `SetNameAsync(string userKey, string name)`. Note: shipped a focused `SetNameAsync` (partial MongoDB update via `UpdateDefinitionBuilder.Set`) instead of a generic `UpdateAsync(TUserEntity)` because `IUser.Name` is read-only on the interface and `with { Name = … }` doesn't compile against the constraint `where TUserEntity : EntityBase, IUser`. The focused-method form is also more efficient (no full-document round-trip). -- [x] **4. Extend `IUserService`.** Added `SeedUserNameAsync` (only-if-empty) and `SetUserNameAsync` (always-overwrites) with `Task.CompletedTask` virtual defaults on `UserServiceBase`, real implementations in `UserServiceRepositoryBase` (GetByKey → check rule → SetNameAsync → InvalidateUserCache). Added `protected void InvalidateUserCache(string identity)` on `UserServiceBase` for the cache invalidation. -- [x] **5. Clear `Member.Name` on accept.** Added `Name = null,` to the `member with { ... }` accept block in `TeamRepository.SetInvitationResponseAsync`. -- [x] **6. Orchestrate seeding in `TeamServiceBase.SetInvitationResponseAsync`.** Pre-capture via new virtual hook `GetInvitedMemberNameAsync(teamKey, inviteKey)` (default returns null on `TeamServiceBase`; overridden in `TeamServiceRepositoryBase` using its typed access to `team.Members`). After response call, if seedName non-empty, `await _userService.SeedUserNameAsync(userKey, seedName)`. -- [x] **7. Self-edit branch in `TeamComponent.razor`.** `SaveMemberName` branches on `member.Key == _user.Key`. Self: `UserService.SetUserNameAsync` + `SetMemberNameAsync(..., null)`. Other: existing per-team override path. Empty self-edit input is treated as a no-op cancel. -- [x] **8. UI gates in `TeamComponent.razor`.** New `canEditThisRow` / `isSelfEdit` locals at the top of the Name template. Edit-mode visibility now `canEditThisRow && !string.IsNullOrEmpty(_editingMemberKey) && _editingMemberKey == context.Key`. Pencil visible whenever `canEditThisRow`. Reset button hidden when `isSelfEdit`. -- [x] **9. Tests.** Added in `Tharga.Team.Service.Tests`: - - `SetInvitationResponseSeedTests` — 4 tests: accept with name calls Seed once, accept with empty/whitespace name doesn't call Seed (×2), reject doesn't call Seed. - - `UserServiceBaseDefaultsTests` — 2 tests: both new virtual defaults are no-ops. - - UI self-edit branch not unit-tested (no Razor harness). Manual verification will cover it. - No tests for `IUserRepository.SetNameAsync` / `GetByKeyAsync` — no MongoDB integration test project in repo. -- [x] **10. Build and run all tests.** Build clean (0 warnings, 0 errors). 274 / 274 tests passing (was 268, +6 new). 145 Service + 37 Mcp + 92 Blazor. -- [~] **11. Manual verification on sample.** Run the sample app: - - Invite a new user with a name. Inspect MongoDB → confirm `Members[].Key` is a non-null GUID and `Members[].Name` is the admin-entered value. - - Edit the invited member's name in the UI, save. Confirm row returns to read-only display showing the new name. - - Invite a second user; open one's edit pencil — confirm the other invited row stays read-only. - - Accept the invitation as the invitee (using a fresh user whose IdP-returned name is empty or present). Confirm: - - `Member.Key` was swapped to `User.Key`. - - `Member.Name` is now null. - - `User.Name` was seeded from `Member.Name` only if it was empty before. - - **Self-edit:** as a non-admin member of a team, click the pencil on your own row, change the name, save. Confirm: - - `User.Name` updated globally. - - `Member.Name` is null in this team. - - In a *different* team where the same user has an admin-set override, that override is unchanged. -- [ ] **12. Commit.** Conventional commit `fix: stable Member.Key for invited members + Member.Name promotion (accept + self-edit)`. Includes `plan/`. -- [ ] **13. Hand back to user for testing.** -- [ ] **14. (After user OK) Open PR.** Push and open PR against `master`. Description doubles as release notes (per `mission.md`); cover the close-out + all three areas (null-Key fix, accept-time promotion, self-edit). -- [ ] **15. (After PR merged) Close out.** Archive `plan/feature.md` to `$DOC_ROOT/Tharga/plans/Toolkit/Platform/done/invited-member-edit-fix.md`, delete `plan/`, final commit `feat: invited-member-edit-fix complete`. - -## Notes - -- No NuGet upgrades needed (`dotnet outdated` reports nothing). -- The branch already carries the close-out commit for the previous feature (`69c8226`). The single PR will include: `plan/` deletion (close-out of #61) + new `plan/` + this fix's code changes. Net result on master: clean. -- **API surface change** (consumer-visible): `IUserRepository` gains `GetByKeyAsync` + `UpdateAsync`. `IUserService` gains `SeedUserNameAsync` + `SetUserNameAsync`. All four are non-breaking for consumers extending `UserServiceRepositoryBase` / using `IUserRepository` as DI-only (typical pattern). External implementers of those interfaces directly would see breakage — none known in this codebase. Suggest a minor version bump on next release. -- **Decided against** adding `TUserEntity` as a third type-parameter to `TeamServiceRepositoryBase`. Would have been a breaking change for every consumer extending it (`Tharga.Platform.Sample/Framework/Team/TeamService.cs`, plus external). Routing through `IUserService` keeps `TeamServiceRepositoryBase`'s ctor stable. \ No newline at end of file From ed7346fd7da8e206e162d2fcc6761385886240aa Mon Sep 17 00:00:00 2001 From: Daniel Bohlin Date: Sun, 10 May 2026 19:58:53 +0200 Subject: [PATCH 2/3] fix: guard TeamServiceBase against null current user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Loading a page before authentication (or after session expiry) crashed the Blazor circuit with NullReferenceException because TeamServiceBase.GetTeamsAsync took the result of GetCurrentUserAsync() and passed it straight to the MongoDB GetTeamsAsync(IUser) override, which dereferenced user.Key. The same unguarded GetCurrentUserAsync pattern existed at 7 call sites in TeamServiceBase. Each call site now handles null per its semantic: - Read paths (GetTeamsAsync, GetTeamsAsync) yield break silently. Empty result is the correct semantic for "no current user, no teams to show". - Touch path (SetMemberLastSeenAsync) returns silently. It's a passive heartbeat; throwing would surprise no-op callers. - Side-effect paths (CreateTeamAsync, RemoveMemberAsync, TransferOwnershipAsync, AssureAccessLevel) call a new private RequireCurrentUserAsync() helper that throws UnauthorizedAccessException with "Authentication required." Surfaces as 401 in pipelines that map UnauthorizedAccessException, otherwise propagates. Also adds a defensive null guard at the data-layer boundary in TeamServiceRepositoryBase.GetTeamsAsync(IUser user) — returns AsyncEnumerable.Empty() for null input. Belt-and-suspenders so the same bug class can't be reintroduced from a future caller that forgets the upstream guard. 6 new tests in UnauthenticatedTeamServiceTests cover each public API path. 280 / 280 tests pass; build clean. --- .../TeamServiceRepositoryBase.cs | 1 + .../UnauthenticatedTeamServiceTests.cs | 69 +++++++++++++ Tharga.Team/TeamServiceBase.cs | 18 +++- plan/feature.md | 96 +++++++++++++++++++ plan/plan.md | 26 +++++ 5 files changed, 206 insertions(+), 4 deletions(-) create mode 100644 Tharga.Team.Service.Tests/UnauthenticatedTeamServiceTests.cs create mode 100644 plan/feature.md create mode 100644 plan/plan.md diff --git a/Tharga.Team.MongoDB/TeamServiceRepositoryBase.cs b/Tharga.Team.MongoDB/TeamServiceRepositoryBase.cs index 4940636..5713d90 100644 --- a/Tharga.Team.MongoDB/TeamServiceRepositoryBase.cs +++ b/Tharga.Team.MongoDB/TeamServiceRepositoryBase.cs @@ -132,6 +132,7 @@ protected override Task SetTeamMemberNameAsync(string teamKey, string userKey, s protected override IAsyncEnumerable GetTeamsAsync(IUser user) { + if (user == null) return AsyncEnumerable.Empty(); return _teamRepository.GetTeamsByUserAsync(user.Key); } diff --git a/Tharga.Team.Service.Tests/UnauthenticatedTeamServiceTests.cs b/Tharga.Team.Service.Tests/UnauthenticatedTeamServiceTests.cs new file mode 100644 index 0000000..7e0f5f2 --- /dev/null +++ b/Tharga.Team.Service.Tests/UnauthenticatedTeamServiceTests.cs @@ -0,0 +1,69 @@ +namespace Tharga.Team.Service.Tests; + +/// +/// Verifies that handles a null current user gracefully: +/// read paths return empty, side-effect paths throw . +/// +public class UnauthenticatedTeamServiceTests +{ + private readonly TestTeamService _sut; + + public UnauthenticatedTeamServiceTests() + { + var userService = Substitute.For(); + userService.GetCurrentUserAsync().Returns((IUser)null); + + _sut = new TestTeamService(userService); + _sut.AddTeam("team-1", "Test Team", + new TestMember { Key = "user-1", AccessLevel = AccessLevel.Owner, State = MembershipState.Member }, + new TestMember { Key = "user-2", AccessLevel = AccessLevel.User, State = MembershipState.Member }); + } + + [Fact] + public async Task GetTeamsAsync_Unauthenticated_ReturnsEmpty() + { + var teams = new List(); + await foreach (var t in _sut.GetTeamsAsync()) teams.Add(t); + + Assert.Empty(teams); + } + + [Fact] + public async Task GetTeamsAsync_Generic_Unauthenticated_ReturnsEmpty() + { + var teams = new List>(); + await foreach (var t in _sut.GetTeamsAsync()) teams.Add(t); + + Assert.Empty(teams); + } + + [Fact] + public async Task CreateTeamAsync_Unauthenticated_Throws() + { + await Assert.ThrowsAsync( + () => _sut.CreateTeamAsync("New Team")); + } + + [Fact] + public async Task RemoveMemberAsync_Unauthenticated_Throws() + { + // user-2 is a non-owner; attempting to remove them as an unauthenticated caller + // should hit the RequireCurrentUserAsync guard before any DB write. + await Assert.ThrowsAsync( + () => _sut.RemoveMemberAsync("team-1", "user-2")); + } + + [Fact] + public async Task SetMemberLastSeenAsync_Unauthenticated_DoesNotThrow() + { + var ex = await Record.ExceptionAsync(() => _sut.SetMemberLastSeenAsync("team-1")); + Assert.Null(ex); + } + + [Fact] + public async Task TransferOwnershipAsync_Unauthenticated_Throws() + { + await Assert.ThrowsAsync( + () => _sut.TransferOwnershipAsync("team-1", "user-1")); + } +} diff --git a/Tharga.Team/TeamServiceBase.cs b/Tharga.Team/TeamServiceBase.cs index d5d390c..b3b86a5 100644 --- a/Tharga.Team/TeamServiceBase.cs +++ b/Tharga.Team/TeamServiceBase.cs @@ -36,6 +36,7 @@ protected TeamServiceBase(IUserService userService) public async IAsyncEnumerable GetTeamsAsync() { var user = await GetCurrentUserAsync(); + if (user == null) yield break; await foreach (var team in GetTeamsAsync(user)) { @@ -46,6 +47,7 @@ public async IAsyncEnumerable GetTeamsAsync() public async IAsyncEnumerable> GetTeamsAsync() where TMember : ITeamMember { var user = await GetCurrentUserAsync(); + if (user == null) yield break; await foreach (var team in GetTeamsAsync(user)) { @@ -61,7 +63,7 @@ public async Task> GetTeamAsync(string teamKey) where TM public async Task CreateTeamAsync(string name) { - var user = await GetCurrentUserAsync(); + var user = await RequireCurrentUserAsync(); var displayName = ResolveDisplayName(user); name ??= $"{displayName}'s team"; @@ -124,7 +126,7 @@ public async Task RemoveMemberAsync(string teamKey, string userKey) if (member.AccessLevel == AccessLevel.Owner) throw new InvalidOperationException("The owner cannot leave the team. Transfer ownership first."); - var user = await GetCurrentUserAsync(); + var user = await RequireCurrentUserAsync(); if (member.Key == user.Key && member.AccessLevel == AccessLevel.Administrator) { var otherAdminsOrOwners = members.Count(x => @@ -211,13 +213,14 @@ protected virtual Task GetInvitedMemberNameAsync(string teamKey, string public async Task SetMemberLastSeenAsync(string teamKey) { var user = await GetCurrentUserAsync(); + if (user == null) return; await SetTeamMemberLastSeenAsync(teamKey, user.Key); _teamMemberCache.TryRemove($"{teamKey}.{user.Key}", out _); } public async Task TransferOwnershipAsync(string teamKey, string newOwnerUserKey) where TMember : ITeamMember { - var user = await GetCurrentUserAsync(); + var user = await RequireCurrentUserAsync(); var team = await GetTeamAsync(teamKey); var currentOwner = team.Members.SingleOrDefault(x => x.Key == user.Key); if (currentOwner == null || currentOwner.AccessLevel != AccessLevel.Owner) @@ -262,7 +265,7 @@ private async Task GetRandomUnsusedTeamKey() private async Task AssureAccessLevel(string teamKey, AccessLevel accessLevel) where TMember : ITeamMember { - var user = await GetCurrentUserAsync(); + var user = await RequireCurrentUserAsync(); var team = await GetTeamAsync(teamKey); var member = team.Members.Single(x => x.Key == user.Key); if (member.State != MembershipState.Member) throw new InvalidOperationException("User is not a member."); @@ -275,6 +278,13 @@ private async Task GetCurrentUserAsync() return user; } + private async Task RequireCurrentUserAsync() + { + var user = await GetCurrentUserAsync(); + if (user == null) throw new UnauthorizedAccessException("Authentication required."); + return user; + } + public static string ResolveDisplayName(IUser user) { if (user == null) return "Unknown"; diff --git a/plan/feature.md b/plan/feature.md new file mode 100644 index 0000000..d95d919 --- /dev/null +++ b/plan/feature.md @@ -0,0 +1,96 @@ +# Feature: Unauthenticated TeamServiceBase guards + +Filed in `Requests.md` 2026-05-10 ("NullReferenceException in `TeamServiceBase` when current user is unauthenticated"). + +## Problem + +`TeamServiceBase.GetTeamsAsync()` does `var user = await GetCurrentUserAsync();` and immediately passes the (possibly null) result to `GetTeamsAsync(IUser user)`. The MongoDB override dereferences `user.Key`, throwing `NullReferenceException`. The same null-user pattern exists at **7 call sites** in `Tharga.Team/TeamServiceBase.cs`. Loading any page hosting `` before authentication (or after session expiry) triggers an unhandled exception, breaks the circuit, and bricks the page. + +## Goal + +Make `TeamServiceBase` resilient to a null current user. Each call site handles the unauthenticated case according to its semantic — read paths return empty; side-effect paths throw a meaningful exception. + +## Scope + +In-scope: + +1. **Two centralized helpers on `TeamServiceBase`:** + - `private async Task GetCurrentUserAsync()` — kept as today (returns null silently). Used by read paths that handle null themselves. + - `private async Task RequireCurrentUserAsync()` (new) — throws `UnauthorizedAccessException("Authentication required.")` when the current user is null. Used by side-effect operations. +2. **Per-call-site refactor** — each of the 7 call sites uses the appropriate helper: + | Line | Method | Treatment | + |---|---|---| + | 38 | `GetTeamsAsync()` | `if (user == null) yield break;` | + | 48 | `GetTeamsAsync()` | same | + | 64 | `CreateTeamAsync(string name)` | `RequireCurrentUserAsync()` | + | 127 | `RemoveMemberAsync` | `RequireCurrentUserAsync()` | + | 213 | `SetMemberLastSeenAsync` | `if (user == null) return;` (touch op; benign no-op) | + | 220 | `TransferOwnershipAsync` | `RequireCurrentUserAsync()` | + | 265 | `AssureAccessLevel` (private gate) | `RequireCurrentUserAsync()` | +3. **Defensive null guard at the MongoDB boundary** — `TeamServiceRepositoryBase.GetTeamsAsync(IUser user)` returns `AsyncEnumerable.Empty()` for null input. Belt-and-suspenders: even if a future caller forgets the upstream guard, the data layer doesn't NRE. +4. **Tests** in `Tharga.Team.Service.Tests` — one per public API to cover the unauthenticated path. + +Out of scope: + +- UI changes in `TeamComponent.razor`. The component already renders no rows when `_teams` is empty, so the immediate-load path is correct after the server fix. Friendlier messaging for unauthenticated users (e.g. hiding the "Create new Team" button when no current user) is a separate UX concern. +- `TeamComponent`'s own `_user = await UserService.GetCurrentUserAsync()` may still be null after init. This was already the case; `_user.Key` accesses inside `@foreach (var team in _teams)` are unreachable when `_teams` is empty. +- Auditing decorators that wrap `TeamServiceBase` — they operate above this layer and are unaffected. +- README/docs change. + +## Approach + +### Server change + +In `Tharga.Team/TeamServiceBase.cs`: + +1. Add private `RequireCurrentUserAsync()`: + ```csharp + private async Task RequireCurrentUserAsync() + { + var user = await GetCurrentUserAsync(); + if (user == null) throw new UnauthorizedAccessException("Authentication required."); + return user; + } + ``` + +2. Refactor each public method per the table above. + +### MongoDB boundary guard + +In `Tharga.Team.MongoDB/TeamServiceRepositoryBase.cs`: + +```csharp +protected override IAsyncEnumerable GetTeamsAsync(IUser user) +{ + if (user == null) return AsyncEnumerable.Empty(); + return _teamRepository.GetTeamsByUserAsync(user.Key); +} +``` + +### Tests + +In `Tharga.Team.Service.Tests` — new file `UnauthenticatedTeamServiceTests.cs`: + +- `GetTeamsAsync_Unauthenticated_ReturnsEmpty` +- `GetTeamsAsync_Generic_Unauthenticated_ReturnsEmpty` +- `CreateTeamAsync_Unauthenticated_Throws` +- `RemoveMemberAsync_Unauthenticated_Throws` +- `SetMemberLastSeenAsync_Unauthenticated_DoesNotThrow` +- `TransferOwnershipAsync_Unauthenticated_Throws` + +All use `TestTeamService` with the mocked `IUserService.GetCurrentUserAsync()` returning null. + +## Acceptance criteria + +- All 7 `GetCurrentUserAsync()` call sites in `TeamServiceBase` handle null appropriately (empty yield, no-op, or `UnauthorizedAccessException`). +- `TeamServiceRepositoryBase.GetTeamsAsync(IUser user)` returns empty for null input. +- 6 new tests cover the unauthenticated path; existing 274 tests still pass. +- `dotnet build -c Release` clean. +- A consumer's sample app loaded by an unauthenticated principal no longer crashes the circuit on `` initial render. +- Bundled with the close-out commit from the previous feature (`fa36d13`) per user preference. + +## Done condition + +User confirms manually on the sample app: +- Loading the team page without authentication shows an empty team list (or a friendly empty state) instead of crashing the circuit. +- Authenticated flow is unchanged. diff --git a/plan/plan.md b/plan/plan.md new file mode 100644 index 0000000..9233e5e --- /dev/null +++ b/plan/plan.md @@ -0,0 +1,26 @@ +# Plan: Unauthenticated TeamServiceBase guards + +Branch: `feature/unauthenticated-team-service-guards` (already carries the close-out commit `fa36d13 feat: invited-member-edit-fix complete` from the previous feature — bundled per user preference). + +## Steps + +- [x] **1. Confirm plan with user.** Done. +- [x] **2. Add `RequireCurrentUserAsync` helper.** Added below the existing `GetCurrentUserAsync()` private helper. +- [x] **3. Refactor 7 call sites** in `TeamServiceBase`. All 7 done per the table. +- [x] **4. Defensive null guard at MongoDB boundary.** `TeamServiceRepositoryBase.GetTeamsAsync(IUser user)` now returns `AsyncEnumerable.Empty()` for null input. +- [x] **5. Tests.** `Tharga.Team.Service.Tests/UnauthenticatedTeamServiceTests.cs` with 6 tests, all passing. Note: `RemoveMemberAsync_Unauthenticated_Throws` had to target a non-owner member (user-2) because the owner-protection check (`The owner cannot leave the team. Transfer ownership first.`) fires before the `RequireCurrentUserAsync()` guard for the owner row. +- [x] **6. Build and run all tests.** Build clean (0 warnings, 0 errors). 280 / 280 tests passing (was 274, +6 new). 151 Service + 37 Mcp + 92 Blazor. +- [~] **7. Manual verification on sample.** Load a `` page without signing in (incognito or new browser session). Confirm: + - No `NullReferenceException` in the host console. + - The page renders the empty state ("You are not member of a team."). + - Sign in and confirm the authenticated flow is unchanged. +- [ ] **8. Commit.** Conventional commit `fix: guard TeamServiceBase against null current user`. Includes `plan/`. +- [ ] **9. Hand back to user for testing.** +- [ ] **10. (After user OK) Open PR.** Push and open PR against `master`. Description doubles as release notes. +- [ ] **11. (After PR merged) Close out.** Archive `plan/feature.md` to `$DOC_ROOT/Tharga/plans/Toolkit/Platform/done/unauthenticated-team-service-guards.md`, delete `plan/`, final commit `feat: unauthenticated-team-service-guards complete`. + +## Notes + +- No NuGet upgrades needed (`dotnet outdated` reports nothing). +- The branch already carries the close-out commit for the previous feature (`fa36d13`). Single PR will include the previous `plan/` deletion + new `plan/` + this fix's code changes. Net result on master: previous `plan/` removed and new `plan/` added (this feature's, to be removed by its own close-out next). +- Marks the `UnauthorizedAccessException` choice: `System.UnauthorizedAccessException` (not a custom Tharga exception) keeps it standard for ASP.NET pipeline handling and aligns with the request's suggestion. Surfacing-as-401 is up to the consumer's pipeline. \ No newline at end of file From 4e9aeb8062890ddc9a2e7502642e494507ec071e Mon Sep 17 00:00:00 2001 From: Daniel Bohlin Date: Sun, 10 May 2026 21:09:03 +0200 Subject: [PATCH 3/3] feat: unauthenticated-team-service-guards complete --- plan/feature.md | 96 ------------------------------------------------- plan/plan.md | 26 -------------- 2 files changed, 122 deletions(-) delete mode 100644 plan/feature.md delete mode 100644 plan/plan.md diff --git a/plan/feature.md b/plan/feature.md deleted file mode 100644 index d95d919..0000000 --- a/plan/feature.md +++ /dev/null @@ -1,96 +0,0 @@ -# Feature: Unauthenticated TeamServiceBase guards - -Filed in `Requests.md` 2026-05-10 ("NullReferenceException in `TeamServiceBase` when current user is unauthenticated"). - -## Problem - -`TeamServiceBase.GetTeamsAsync()` does `var user = await GetCurrentUserAsync();` and immediately passes the (possibly null) result to `GetTeamsAsync(IUser user)`. The MongoDB override dereferences `user.Key`, throwing `NullReferenceException`. The same null-user pattern exists at **7 call sites** in `Tharga.Team/TeamServiceBase.cs`. Loading any page hosting `` before authentication (or after session expiry) triggers an unhandled exception, breaks the circuit, and bricks the page. - -## Goal - -Make `TeamServiceBase` resilient to a null current user. Each call site handles the unauthenticated case according to its semantic — read paths return empty; side-effect paths throw a meaningful exception. - -## Scope - -In-scope: - -1. **Two centralized helpers on `TeamServiceBase`:** - - `private async Task GetCurrentUserAsync()` — kept as today (returns null silently). Used by read paths that handle null themselves. - - `private async Task RequireCurrentUserAsync()` (new) — throws `UnauthorizedAccessException("Authentication required.")` when the current user is null. Used by side-effect operations. -2. **Per-call-site refactor** — each of the 7 call sites uses the appropriate helper: - | Line | Method | Treatment | - |---|---|---| - | 38 | `GetTeamsAsync()` | `if (user == null) yield break;` | - | 48 | `GetTeamsAsync()` | same | - | 64 | `CreateTeamAsync(string name)` | `RequireCurrentUserAsync()` | - | 127 | `RemoveMemberAsync` | `RequireCurrentUserAsync()` | - | 213 | `SetMemberLastSeenAsync` | `if (user == null) return;` (touch op; benign no-op) | - | 220 | `TransferOwnershipAsync` | `RequireCurrentUserAsync()` | - | 265 | `AssureAccessLevel` (private gate) | `RequireCurrentUserAsync()` | -3. **Defensive null guard at the MongoDB boundary** — `TeamServiceRepositoryBase.GetTeamsAsync(IUser user)` returns `AsyncEnumerable.Empty()` for null input. Belt-and-suspenders: even if a future caller forgets the upstream guard, the data layer doesn't NRE. -4. **Tests** in `Tharga.Team.Service.Tests` — one per public API to cover the unauthenticated path. - -Out of scope: - -- UI changes in `TeamComponent.razor`. The component already renders no rows when `_teams` is empty, so the immediate-load path is correct after the server fix. Friendlier messaging for unauthenticated users (e.g. hiding the "Create new Team" button when no current user) is a separate UX concern. -- `TeamComponent`'s own `_user = await UserService.GetCurrentUserAsync()` may still be null after init. This was already the case; `_user.Key` accesses inside `@foreach (var team in _teams)` are unreachable when `_teams` is empty. -- Auditing decorators that wrap `TeamServiceBase` — they operate above this layer and are unaffected. -- README/docs change. - -## Approach - -### Server change - -In `Tharga.Team/TeamServiceBase.cs`: - -1. Add private `RequireCurrentUserAsync()`: - ```csharp - private async Task RequireCurrentUserAsync() - { - var user = await GetCurrentUserAsync(); - if (user == null) throw new UnauthorizedAccessException("Authentication required."); - return user; - } - ``` - -2. Refactor each public method per the table above. - -### MongoDB boundary guard - -In `Tharga.Team.MongoDB/TeamServiceRepositoryBase.cs`: - -```csharp -protected override IAsyncEnumerable GetTeamsAsync(IUser user) -{ - if (user == null) return AsyncEnumerable.Empty(); - return _teamRepository.GetTeamsByUserAsync(user.Key); -} -``` - -### Tests - -In `Tharga.Team.Service.Tests` — new file `UnauthenticatedTeamServiceTests.cs`: - -- `GetTeamsAsync_Unauthenticated_ReturnsEmpty` -- `GetTeamsAsync_Generic_Unauthenticated_ReturnsEmpty` -- `CreateTeamAsync_Unauthenticated_Throws` -- `RemoveMemberAsync_Unauthenticated_Throws` -- `SetMemberLastSeenAsync_Unauthenticated_DoesNotThrow` -- `TransferOwnershipAsync_Unauthenticated_Throws` - -All use `TestTeamService` with the mocked `IUserService.GetCurrentUserAsync()` returning null. - -## Acceptance criteria - -- All 7 `GetCurrentUserAsync()` call sites in `TeamServiceBase` handle null appropriately (empty yield, no-op, or `UnauthorizedAccessException`). -- `TeamServiceRepositoryBase.GetTeamsAsync(IUser user)` returns empty for null input. -- 6 new tests cover the unauthenticated path; existing 274 tests still pass. -- `dotnet build -c Release` clean. -- A consumer's sample app loaded by an unauthenticated principal no longer crashes the circuit on `` initial render. -- Bundled with the close-out commit from the previous feature (`fa36d13`) per user preference. - -## Done condition - -User confirms manually on the sample app: -- Loading the team page without authentication shows an empty team list (or a friendly empty state) instead of crashing the circuit. -- Authenticated flow is unchanged. diff --git a/plan/plan.md b/plan/plan.md deleted file mode 100644 index 9233e5e..0000000 --- a/plan/plan.md +++ /dev/null @@ -1,26 +0,0 @@ -# Plan: Unauthenticated TeamServiceBase guards - -Branch: `feature/unauthenticated-team-service-guards` (already carries the close-out commit `fa36d13 feat: invited-member-edit-fix complete` from the previous feature — bundled per user preference). - -## Steps - -- [x] **1. Confirm plan with user.** Done. -- [x] **2. Add `RequireCurrentUserAsync` helper.** Added below the existing `GetCurrentUserAsync()` private helper. -- [x] **3. Refactor 7 call sites** in `TeamServiceBase`. All 7 done per the table. -- [x] **4. Defensive null guard at MongoDB boundary.** `TeamServiceRepositoryBase.GetTeamsAsync(IUser user)` now returns `AsyncEnumerable.Empty()` for null input. -- [x] **5. Tests.** `Tharga.Team.Service.Tests/UnauthenticatedTeamServiceTests.cs` with 6 tests, all passing. Note: `RemoveMemberAsync_Unauthenticated_Throws` had to target a non-owner member (user-2) because the owner-protection check (`The owner cannot leave the team. Transfer ownership first.`) fires before the `RequireCurrentUserAsync()` guard for the owner row. -- [x] **6. Build and run all tests.** Build clean (0 warnings, 0 errors). 280 / 280 tests passing (was 274, +6 new). 151 Service + 37 Mcp + 92 Blazor. -- [~] **7. Manual verification on sample.** Load a `` page without signing in (incognito or new browser session). Confirm: - - No `NullReferenceException` in the host console. - - The page renders the empty state ("You are not member of a team."). - - Sign in and confirm the authenticated flow is unchanged. -- [ ] **8. Commit.** Conventional commit `fix: guard TeamServiceBase against null current user`. Includes `plan/`. -- [ ] **9. Hand back to user for testing.** -- [ ] **10. (After user OK) Open PR.** Push and open PR against `master`. Description doubles as release notes. -- [ ] **11. (After PR merged) Close out.** Archive `plan/feature.md` to `$DOC_ROOT/Tharga/plans/Toolkit/Platform/done/unauthenticated-team-service-guards.md`, delete `plan/`, final commit `feat: unauthenticated-team-service-guards complete`. - -## Notes - -- No NuGet upgrades needed (`dotnet outdated` reports nothing). -- The branch already carries the close-out commit for the previous feature (`fa36d13`). Single PR will include the previous `plan/` deletion + new `plan/` + this fix's code changes. Net result on master: previous `plan/` removed and new `plan/` added (this feature's, to be removed by its own close-out next). -- Marks the `UnauthorizedAccessException` choice: `System.UnauthorizedAccessException` (not a custom Tharga exception) keeps it standard for ASP.NET pipeline handling and aligns with the request's suggestion. Surfacing-as-401 is up to the consumer's pipeline. \ No newline at end of file