Skip to content

[ADR-183] OAuth2 client credentials token provider for cloud assets#179

Open
jodavis-claude wants to merge 4 commits intofeature/ADR-162-client-side-layout-updatesfrom
dev/claude/ADR-183-oauth-token-provider
Open

[ADR-183] OAuth2 client credentials token provider for cloud assets#179
jodavis-claude wants to merge 4 commits intofeature/ADR-162-client-side-layout-updatesfrom
dev/claude/ADR-183-oauth-token-provider

Conversation

@jodavis-claude
Copy link
Copy Markdown
Collaborator

Jira: https://jodasoft.atlassian.net/browse/ADR-183

What changed

  • Added ICloudAuthTokenProvider interface with GetTokenAsync(CancellationToken) in Services/CloudAssets/
  • Added CognitoTokenProvider singleton: POSTs to Cognito token endpoint with client credentials, caches token in-memory, proactively refreshes 60s before expiry, uses SemaphoreSlim for thread safety
  • Added ClientId, ClientSecret, CognitoTokenEndpointUrl to CloudSettings
  • Added log messages 1010–1012 to MessageLogger.cs for token acquisition events
  • Added 5 unit tests covering first-call fetch, cached reuse, near-expiry refresh, cancellation, and HTTP failure propagation
  • Updated _doc_CloudAssets.md with OAuth2 token provider section

Test plan

  • 396 app unit tests pass (includes new CognitoTokenProviderTests)
  • 27 backend service tests pass
  • 21 headless E2E tests pass

🤖 Generated with Claude Code


Generated by Claude Code

claude added 2 commits May 5, 2026 00:23
Introduce OAuth2 client credentials authentication to the cloud asset
infrastructure. CognitoTokenProvider acquires tokens from AWS Cognito,
caches them in memory, and transparently refreshes them within 60 seconds
of expiry. A SemaphoreSlim prevents concurrent refreshes.

CloudSettings gains ClientId, ClientSecret, and CognitoTokenEndpointUrl
(all default to empty string — supply via user secrets or env vars).

MessageLogger gains CognitoTokenProvider_* log methods (Event IDs 1010-1012).

Unit tests cover: first-call fetch, cached-token reuse, near-expiry refresh,
cancellation, and HTTP failure propagation.

https://claude.ai/code/session_016PwpXFt999Q5Xk52qFukfu
Update CognitoTokenProvider_GetTokenAsync_HttpFailure_PropagatesExceptionAsync
to assert the exact exception message that EnsureSuccessStatusCode() throws
(includes the 401 status code) rather than a default HttpRequestException.

https://claude.ai/code/session_016PwpXFt999Q5Xk52qFukfu
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 5, 2026

Test Results

545 tests  +5   545 ✅ +5   5m 49s ⏱️ +29s
  8 suites ±0     0 💤 ±0 
  8 files   ±0     0 ❌ ±0 

Results for commit 914979d. ± Comparison against base commit 2a1c6e7.

♻️ This comment has been updated with latest results.

Copy link
Copy Markdown
Collaborator Author

@jodavis-claude jodavis-claude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review — see inline comments.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator Author

Re: Near-duplicate of CognitoTokenService

The spec (ADR-183) explicitly calls for a new ICloudAuthTokenProvider interface and CognitoTokenProvider implementation in Services/CloudAssets/. CognitoTokenService lives in Services/Backend/ and serves backend infrastructure with a different responsibility boundary. Consolidating them is a separate refactoring concern outside this task's scope.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator Author

Re: new HttpClient() anti-pattern

This is consistent with the existing CognitoTokenService pattern already in the codebase. Migrating to IHttpClientFactory is a separate concern that would apply to both classes together as a future improvement, not something scoped to this task.


Generated by Claude Code

- Move CognitoTokenProvider log event IDs from conflicting 1010-1012 range to 1800-1899
- Add ICloudAuthTokenProvider/CognitoTokenProvider DI registration to AddCloudAssetServices
- Add MockLogger.VerifyMessages assertion to SecondCallBeforeExpiry test
- Replace Queue<T> with ConcurrentQueue<T> in FakeHttpMessageHandler for thread safety

https://claude.ai/code/session_016PwpXFt999Q5Xk52qFukfu
@jodavis-claude jodavis-claude requested a review from jodavis May 5, 2026 04:41
…letion

Add three behaviours to the team-task skill:
- Phase 4 creates the PR as a draft (draft: true) using the MCP tool
- Completion marks the draft ready for review (draft: false)
- Completion requests a review from @jodavis
// ─────────────────────────────────────────────────────────────────

[TestMethod]
public async Task CognitoTokenProvider_GetTokenAsync_FirstCall_FetchesAndReturnsTokenAsync()
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't create async unit tests. Use TaskCompletionSource to simulate asynchronous operations, and have tests to cover all edge cases and error conditions (e.g. any call to an async method could block, throw, be cancelled, or succeed. All async calls that accept CancellationToken should respond to the CancellationToken passed in to GetTokenAsync).

// FakeHttpMessageHandler — returns pre-configured responses in order
// ─────────────────────────────────────────────────────────────────

private sealed class FakeHttpMessageHandler : HttpMessageHandler
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this to the TestUtilities namespace. It will probably be useful for other tests that need to make HTTP calls, e.g. the REST API calls that the real CloudAssetDownloader will eventually make.


// Assert
token.Should().Be("first-token");
handler.CallCount.Should().Be(1);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also verify that the correct URL/content was passed, not just that a call was made.

Comment on lines +81 to +82
new("client_id", _settings.ClientId),
new("client_secret", _settings.ClientSecret),
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If ClientId and ClientSecret are not set, this should fail gracefully. I'm not sure what that means for the API. Maybe it returns null, and the caller will know not to attach an Authorization header if the token is null?

{
internal static IServiceCollection AddCloudAssetServices(this IServiceCollection services, IConfiguration configuration)
=> services
.AddSingleton<ICloudAuthTokenProvider, CognitoTokenProvider>()
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing is calling this yet, so we have no idea whether the code actually works.

Let's add a call to ICloudAuthTokenProvider.GetTokenAsync in the stub implementation of ICloudAssetDownloader. It doesn't need a token, but it can try to acquire one anyway.

There should be E2E tests with ClientId/ClientSecret set and not set, so we validate that the application can start without credentials (no cloud downloads) and with credentials (cloud downloads attempted). I think we created a mock OAuth server to test authentication in ApiTests. These scenarios should be added to the CloudAsset E2E scenarios.

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.

3 participants