Skip to content

Commit 1078705

Browse files
myieyeclaude
andcommitted
fix(auth): publish AuthenticationChangedEvent after releasing the auth lock
RemoveAccountAsync published AuthenticationChangedEvent while GetAuth still held _authSemaphore. Subject.OnNext dispatches synchronously, so a subscriber re-entering GetAuth/Logout would self-deadlock on the non-reentrant semaphore. Mirror Logout: GetAuth now captures the result under the lock, releases, then publishes outside it only when an account was removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 651ed99 commit 1078705

2 files changed

Lines changed: 33 additions & 2 deletions

File tree

backend/FwLite/FwLiteShared.Tests/Auth/OAuthClientSilentRefreshTests.cs

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,27 @@ public async Task InvalidGrant_RemovesAccountAndRaisesEvent()
5858
events.Should().ContainSingle().Which.Server.Should().Be(Server);
5959
}
6060

61+
// The AuthenticationChangedEvent must be published after _authSemaphore is released: Subject.OnNext
62+
// dispatches synchronously, so a subscriber that re-enters a semaphore-taking auth method (here
63+
// GetCurrentToken -> GetAuth) would self-deadlock on the non-reentrant semaphore if we published
64+
// while still holding it. Re-entering synchronously and completing proves the lock is free.
65+
[Fact]
66+
public async Task RemoveAccount_PublishesEventAfterLockReleased()
67+
{
68+
var (client, _, eventBus, events) = BuildClient((_, _) =>
69+
throw new MsalUiRequiredException("invalid_grant", "refresh token expired"));
70+
71+
bool? reentrantTokenWasNull = null;
72+
eventBus.OnAuthenticationChanged.Subscribe(_ =>
73+
reentrantTokenWasNull = client.GetCurrentToken().AsTask().GetAwaiter().GetResult() is null);
74+
75+
await client.GetAuth();
76+
77+
events.Should().ContainSingle("the event still fires exactly once");
78+
reentrantTokenWasNull.Should().Be(true,
79+
"the subscriber re-entered GetAuth synchronously without deadlocking, proving the publish happens after the lock is released");
80+
}
81+
6182
[Fact]
6283
public async Task Cancellation_KeepsCachedAccount()
6384
{

backend/FwLite/FwLiteShared/Auth/OAuthClient.cs

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,8 @@ public async Task Logout()
179179
{
180180
_authSemaphore.Release();
181181
}
182+
//publish outside the lock: Subject.OnNext dispatches synchronously, so a subscriber that
183+
//re-enters a GetAuth/Logout path would self-deadlock on the non-reentrant _authSemaphore.
182184
_globalEventBus.PublishEvent(new AuthenticationChangedEvent(_lexboxServer));
183185
}
184186

@@ -202,6 +204,8 @@ public async ValueTask<bool> IsSignedIn()
202204
return _authResult;
203205
}
204206

207+
var accountRemoved = false;
208+
AuthenticationResult? result;
205209
await _authSemaphore.WaitAsync();
206210
try
207211
{
@@ -226,6 +230,7 @@ public async ValueTask<bool> IsSignedIn()
226230
case SilentAuthFailureOutcome.RemoveAccount:
227231
_logger.LogWarning(e, "Silent token acquisition failed with a non-recoverable error, logging out");
228232
await RemoveAccountAsync(account);
233+
accountRemoved = true;
229234
break;
230235
case SilentAuthFailureOutcome.KeepCachedCredentials:
231236
_logger.LogWarning(e, "Silent token acquisition failed with a transient or unknown error; keeping cached credentials");
@@ -241,12 +246,17 @@ public async ValueTask<bool> IsSignedIn()
241246
}
242247
}
243248

244-
return _authResult;
249+
//capture under the lock: after release a concurrent GetAuth could repopulate _authResult.
250+
result = _authResult;
245251
}
246252
finally
247253
{
248254
_authSemaphore.Release();
249255
}
256+
257+
//publish outside the lock, as Logout does (see the rationale there).
258+
if (accountRemoved) _globalEventBus.PublishEvent(new AuthenticationChangedEvent(_lexboxServer));
259+
return result;
250260
}
251261

252262
//test seam — overridable so tests can stub the result without faking MSAL's builder chain.
@@ -273,11 +283,11 @@ internal enum SilentAuthFailureOutcome
273283
_ => SilentAuthFailureOutcome.KeepCachedCredentials,
274284
};
275285

286+
//caller is responsible for publishing AuthenticationChangedEvent after releasing _authSemaphore (see Logout).
276287
private async Task RemoveAccountAsync(IAccount account)
277288
{
278289
await _application.RemoveAsync(account);
279290
_authResult = null;
280-
_globalEventBus.PublishEvent(new AuthenticationChangedEvent(_lexboxServer));
281291
}
282292

283293
public async Task<string?> GetCurrentName()

0 commit comments

Comments
 (0)