Skip to content

Normalize authentication through a shared AuthService - #2600

Closed
niemyjski wants to merge 1 commit into
mainfrom
issue/oauth-auth-hardening
Closed

niemyjski wants to merge 1 commit into
mainfrom
issue/oauth-auth-hardening

Conversation

@niemyjski

Copy link
Copy Markdown
Member

Centralize password-login handling in a shared AuthService, align account-state checks across authentication paths, and normalize credential parsing, token exchange, outbound address validation, and webhook request handling. Add regression coverage for login, recovery, concurrency, and existing request formats.

Validation: 110 focused auth tests passed on this commit; earlier runs passed 988 broader API/contract checks and 79 auth endpoint tests with local Redis caching. Changed C# files were formatted.

Review blocker: failure tracking still uses bounded markers. Simplifying it to counters depends on resolving the reproduced Foundatio 13.0.4 in-memory counter behavior described in the review comment. Window-specific cache keys also restart transient throttle history during rollout; mixed-version nodes use separate histories.

Verification and implementation details
  • Password-login failures share per-user and per-IP limits across interactive and Basic authentication. Successful concurrent requests consume no failure quota; the normal path uses no distributed locks or cache writes.
  • Cleanup preserves failures recorded after an attempt began. Quarter-hour expiry remains fixed, and password recovery preserves account active state and IP failures.
  • Existing OAuth code-cache keys, Basic token aliases, and colon-containing passwords are covered. Frontend code, shared integration fixtures, and HTTP samples are unchanged.
  • External-provider responses were stubbed. Production and multi-process load behavior were not tested.

Focused verification:

EX_StripeApiKey= EX_ExceptionlessServerUrl=http://localhost:7110 EX_ExceptionlessApiKey= dotnet test --project tests/Exceptionless.Tests --no-restore -- --filter-class Exceptionless.Tests.Services.AuthServiceTests --filter-class Exceptionless.Tests.Api.Endpoints.AuthEndpointTests

Centralize password-login checks and normalize credential parsing, token exchange, and outbound request validation. Add regression coverage for login, password recovery, concurrent requests, and integration handling.
@niemyjski

Copy link
Copy Markdown
Member Author

@ejsmith could you review this specific cache-counter behavior before we simplify AuthService? I would prefer ordinary counters over retaining the bounded-marker workaround.

With Foundatio 13.0.4, .NET 10 on macOS arm64, five isolated runs of 10,000 InMemoryCacheClient.IncrementAsync(key, 1) calls at concurrency 16 produced:

Run Expected Actual
1 10,000 9,405
2 10,000 9,329
3 10,000 9,663
4 10,000 9,594
5 10,000 9,539

This reproduction uses only an in-memory cache, without application services or network access. The decompiled implementation reads and mutates the same existing CacheEntry inside the ConcurrentDictionary.AddOrUpdate callback. Concurrent callbacks can read the same value and overwrite each other's increments; that appears to explain the observed results. Redis counter behavior is not implicated by this reproduction.

Exact reproduction

Create a .NET 10 console project with <PackageReference Include="Foundatio" Version="13.0.4" /> and implicit usings enabled, then run:

using Foundatio.Caching;

using var cache = new InMemoryCacheClient();
const int incrementCount = 10000;
for (int run = 0; run < 5; run++)
{
    string key = $"counter-{run}";
    await cache.SetAsync(key, 0L);
    await Parallel.ForEachAsync(Enumerable.Range(0, incrementCount),
        new ParallelOptions { MaxDegreeOfParallelism = 16 },
        async (attempt, cancellationToken) => await cache.IncrementAsync(key, 1));
    long actual = (await cache.GetAsync<long>(key)).Value;
    Console.WriteLine($"Run {run + 1}: expected {incrementCount}, actual {actual}");
    if (actual != incrementCount)
        Environment.ExitCode = 1;
}

This PR remains draft because simply replacing the markers with the current in-memory counter implementation would lose concurrent failures. The dependency should provide the atomic counter behavior so authentication can stay simple.

@niemyjski niemyjski closed this Sep 29, 2026
@niemyjski
niemyjski deleted the issue/oauth-auth-hardening branch September 29, 2026 03:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant