Skip to content

feat(tables): Add audience option to TableServiceClientOptions #33802

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jeremymeng
Copy link
Member

(note from Jeremy: all except this note are created by GitHub CoPilot)

This PR adds an audience option to the TableServiceClientOptions interface that allows customers to pass a custom scope to override the default scope when using token authentication.

Feature

  • Added audience option to the TableServiceClientOptions interface
  • Updated TableServiceClient to use the custom audience if provided
  • Updated TableClient to use the custom audience if provided
  • Added comprehensive unit tests for both client classes

Testing

Unit tests have been added to verify both the default scope selection logic and the custom audience option work as expected.

Documentation

Added JSDoc to explain the new audience option. The option is optional and defaults to the existing behavior.

via `audience` client option

(by copilot)
…ace and support in both clients

This change adds an audience option to the TableServiceClientOptions interface that allows
customers to pass a custom scope to override the default scope when using token authentication.

- Updated TableServiceClient to use the custom audience if provided
- Updated TableClient to use the custom audience if provided
- Added unit tests for both client classes to verify the audience option works correctly
- Used nullish coalescing operator (??) for more precise handling of audience option
@Copilot Copilot AI review requested due to automatic review settings April 11, 2025 18:46
@jeremymeng jeremymeng requested review from bterlson, a team and joheredi as code owners April 11, 2025 18:46
Copy link
Contributor

@Copilot 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.

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

sdk/tables/data-tables/test/public/endpoint.spec.ts:19

  • [nitpick] The outer describe block 'Endpoint tests' is somewhat generic given that the inner tests target both Cosmos and Storage endpoints. Consider using a more descriptive title that encompasses both endpoint types for clarity.
describe(`Endpoint tests`, () => {

@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

@azure/data-tables

@jeremymeng
Copy link
Member Author

For #33719

Copy link
Member

@christothes christothes left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -31,6 +31,12 @@ export type TransactionAction = CreateDeleteEntityAction | UpdateEntityAction;
export type TableServiceClientOptions = CommonClientOptions & {
endpoint?: string;
version?: string;
/**
Copy link
Member

@jsquire jsquire Apr 11, 2025

Choose a reason for hiding this comment

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

It looks like you're deviating from existing patterns, as there's no KnownTablesAudience enum with the scopes. Was that intentional?

Prior art:

Copy link
Member Author

Choose a reason for hiding this comment

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

Storage doesn't have a Known* enum. We could add that support if we have a list of known values.

Copy link
Member Author

Choose a reason for hiding this comment

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

When we added the support for container registry, we really wished that the service could support a static scope and figured out the proper ones to use by themselves. I bet the work item is still in their backlog somewhere

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. It would have been a much better experience if there was an approach available to allow any service to infer the proper scope. Unfortunately, the folks who were working on guidance could not come to agreement on a design and the burden remains on callers when using sovereign clouds.

Copy link
Member Author

Choose a reason for hiding this comment

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

I asked copilot to ensure "/.default" is appended. Known*Audience can be additive to provide convenience.

This change ensures that when a custom audience is provided for token authentication, it always ends with '/.default'. The fix modifies both TableClient and TableServiceClient to check if the user-provided audience already ends with '/.default', and if not, append it automatically.

This improves authentication reliability when working with custom audiences in Azure Tables SDK.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants