Skip to content

Conversation

@Erwinvandervalk
Copy link
Contributor

What issue does this PR address?

  • allow empty client secret
  • fix issue with sample and clientid null

@Erwinvandervalk Erwinvandervalk added this to the atm-4.0 milestone Jul 11, 2025
@Erwinvandervalk Erwinvandervalk self-assigned this Jul 11, 2025
Copilot AI review requested due to automatic review settings July 11, 2025 09:01
@Erwinvandervalk Erwinvandervalk requested a review from a team as a code owner July 11, 2025 09:01
@Erwinvandervalk Erwinvandervalk added the area/foss/access-token-management Issues related to Access Token Management label Jul 11, 2025
@Erwinvandervalk Erwinvandervalk linked an issue Jul 11, 2025 that may be closed by this pull request
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enables using a null/empty client secret without throwing during token requests and adds a fallback when the client ID is missing in the sample.

  • Tests are updated to expect a successful token even when ClientSecret is null.
  • Validation and runtime checks for a null ClientSecret are removed.
  • The Blazor sample now handles a missing ClientId by falling back to configuration or a hard-coded value.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
access-token-management/test/AccessTokenManagement.Tests/ClientTokenManagementTests.cs Test now expects a token instead of an exception for empty secret
access-token-management/src/AccessTokenManagement/Internal/ClientCredentialsTokenClient.cs Removed null check for ClientSecret before sending form data
access-token-management/src/AccessTokenManagement/ClientCredentialsClient.cs Removed options validation error for null ClientSecret
access-token-management/samples/BlazorServer/Plumbing/OidcEvents.cs Added fallback for missing ClientId and updated comments
Comments suppressed due to low confidence (2)

access-token-management/test/AccessTokenManagement.Tests/ClientTokenManagementTests.cs:68

  • [nitpick] The test name indicates an exception is expected, but the behavior changed to return a token. Consider renaming this to something like Allow_empty_client_secret_returns_token for clarity.
    public async Task Missing_client_secret_throw_exception()

access-token-management/test/AccessTokenManagement.Tests/ClientTokenManagementTests.cs:82

  • [nitpick] The identifier The.ClientId is unclear—either use ClientId.Parse("test") inline or define a well-named constant to improve readability.
                client.ClientId = The.ClientId;

@Erwinvandervalk Erwinvandervalk requested a review from damianh July 11, 2025 09:40
@Erwinvandervalk Erwinvandervalk force-pushed the ev/bff/allow-empty-clientsecret branch from 4d007fd to 64dd5bd Compare July 15, 2025 07:52
fix issue with sample and clientid null

Apply suggestion from @Copilot

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

remove empty line
@Erwinvandervalk Erwinvandervalk force-pushed the ev/bff/allow-empty-clientsecret branch from 64dd5bd to ac24ab7 Compare July 15, 2025 08:30
@Erwinvandervalk Erwinvandervalk merged commit d8d1bff into main Jul 15, 2025
3 checks passed
@Erwinvandervalk Erwinvandervalk deleted the ev/bff/allow-empty-clientsecret branch July 15, 2025 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/foss/access-token-management Issues related to Access Token Management

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sample fails on ClientId null

3 participants