Ops new endpoints #32375
Open
Ops new endpoints #32375
+160
−69
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sofiar-msft
requested review from
AikoBB,
maximrytych-ms and
mjafferi-msft
as code owners
AikoBB
reviewed
Dec 31, 2024
|
|
||
| const entraTokenCredentialOptions: EntraCommunicationTokenCredentialOptions = { | ||
| resourceEndpoint: "https://<your-resource>.communication.azure.com", | ||
| tokenCredential: entraTokenCredential, |
There was a problem hiding this comment.
let's inform clients using js to install the @azure/identity package
AikoBB
reviewed
Dec 31, 2024
| @@ -106,6 +106,25 @@ const entraTokenCredentialOptions: EntraCommunicationTokenCredentialOptions = { | |||
| const credential = new AzureCommunicationTokenCredential(entraTokenCredentialOptions); | |||
| ``` | |||
|
|
|||
| The same approach can be used for authorizing an Entra user with a Teams license to use Teams Phone Extensibility features through your Azure Communication Services resource. | |||
There was a problem hiding this comment.
can we also update changelog.md file with the upcoming changes
| @@ -16,6 +16,13 @@ import { | |||
| createPipelineRequest, | |||
| } from "@azure/core-rest-pipeline"; | |||
|
|
|||
| const TeamsExtensionScopePrefix = "https://auth.msft.communication.azure.com/"; | |||
| const ComunicationClientsScopePrefix = "https://communication.azure.com/clients/"; | |||
| const TeamsExtensionEndpoint = "/access/teamsPhone/:exchangeTeamsAccessToken"; | |||
There was a problem hiding this comment.
Suggested change
| const TeamsExtensionEndpoint = "/access/teamsPhone/:exchangeTeamsAccessToken"; | |
| const TeamsExtensionEndpoint = "/access/teamsPhone/:exchangeAccessToken"; |
wrong endpoint, it should be this one
|
|
||
| constructor(private options: EntraCommunicationTokenCredentialOptions) { | ||
| constructor(options: EntraCommunicationTokenCredentialOptions) { |
There was a problem hiding this comment.
why the private keyword was removed?
| this.client = getClient(options.resourceEndpoint); | ||
| this.httpClient = createDefaultHttpClient(); | ||
| this.options = options; | ||
| this.options.scopes = this.options.scopes |
There was a problem hiding this comment.
Suggested change
| this.options.scopes = this.options.scopes | |
| this.options.scopes = this.options.scopes || ["https://communication.azure.com/clients/.default"]; | |
can we simplify as follow?
| return requestUri; | ||
| } | ||
|
|
||
| private DetermineEndpointAndApiVersion() : [string, string] { |
There was a problem hiding this comment.
Suggested change
| private DetermineEndpointAndApiVersion() : [string, string] { | |
| private determineEndpointAndApiVersion() : [string, string] { |
| const scopes = ["https://communication.azure.com/clients/VoIP"]; | ||
| const comunicationClientsEndpoint = "/access/entra/:exchangeAccessToken?api-version=2024-04-01-preview"; | ||
| const communicationClientsScope = "https://communication.azure.com/clients/VoIP"; | ||
| const teamsExtensionEndpoint = "/access/teamsPhone/:exchangeTeamsAccessToken?api-version=2025-03-02-preview"; |
There was a problem hiding this comment.
Suggested change
| const teamsExtensionEndpoint = "/access/teamsPhone/:exchangeTeamsAccessToken?api-version=2025-03-02-preview"; | |
| const teamsExtensionEndpoint = "/access/teamsPhone/:exchangeAccessToken?api-version=2025-03-02-preview"; |
|
|
||
| it("Token exchange is not called again when Entra token stays the same", async function () { | ||
| let scope = successApiMock(); | ||
| it("Token exchange is not called again when Entra token stays the same " + apiVersion, async function () { |
There was a problem hiding this comment.
instead of the apiVersion can we use the endpoint, so we can differentiate between two endpoints
| const credential = new AzureCommunicationTokenCredential(entraTokenCredentialOptions); | ||
|
|
||
| const tokenResult = (await credential.getToken()).token; | ||
| assert.strictEqual(tokenResult, acsToken); |
There was a problem hiding this comment.
can we verify that the Entra endpoint is being called?
| assert.isTrue(getTokenSpy.alwaysCalledWithExactly([scopes], undefined)); | ||
| assert.isTrue(getTokenSpy.callCount > 0); | ||
| assert.isTrue(apiMock.isDone()); | ||
| getTokenSpy.restore(); |
There was a problem hiding this comment.
Can we verify if the correct endpoint is being called based on the provided scopes?
| } | ||
|
|
||
| private DetermineEndpointAndApiVersion() : [string, string] { | ||
| if (!this.options.scopes || this.options.scopes.length === 0) { |
There was a problem hiding this comment.
this if condition and the last else condition are not covered by unit tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Packages impacted by this PR
Issues associated with this PR
Describe the problem that is addressed by this PR
What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
Are there test cases added in this PR? (If not, why?)
Provide a list of related PRs (if any)
Command used to generate this PR:**(Applicable only to SDK release request PRs)
Checklists