[Communication] - Authentication - PhoneNumberAdministrationClient auth using token credential #13271
Conversation
JoshuaLai
requested review from
0rland0Wats0n,
ankitarorabit,
bterlson,
DominikMe,
RezaJooyandeh,
xirzec and
a team
as code owners
ghost
added
the
...unication/communication-administration/test/phoneNumberAdministrationClientWithToken.spec.ts
Outdated
Show resolved
Hide resolved
|
/azp run js - communication - tests |
|
No pipelines are associated with this pull request. |
|
/azp run js - communication-administration - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - communication-administration - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
sdk/communication/communication-administration/test/utils/recordedClient.ts
Outdated
Show resolved
Hide resolved
…ber to use Token Credential
|
/azp run js - communication-administration - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - communication-administration - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - communication-administration - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - communication-administration - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - communication-administration - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| const client = new PhoneNumberAdministrationClient(CONNECTION_STRING); | ||
| ``` | ||
|
|
||
| #### Creating an instance of PhoneNumberAdministrationClient with TokenCredential |
There was a problem hiding this comment.
Is saying TokenCredential enough for users to know that they can authenticate using managed identity (Azure Active Directory Authentication)?
There was a problem hiding this comment.
Yes that is fair I think we can make this change to be more consistent with the other platforms. I will update the change log as well.
| recorder | ||
| }; | ||
| } | ||
|
|
||
| try { | ||
| credential = new DefaultAzureCredential(); | ||
| credential = new ClientSecretCredential( |
There was a problem hiding this comment.
Did DefaultAzureCredential not work as in Java and Python?
There was a problem hiding this comment.
This is an interesting one. Under the hood there isn't a huge difference between the two. But when debugging we found that using DefaultAzureCredential threw this complicated error that made it very hard to debug the issue, but when using ClientSecretCredential it gave a more specific error (telling us there was an issue with the env vars). That was why I wanted to stick to ClientSecretCredential. Thoughts @DominikMe ? I know you wanted to originally use DefaultAzureCredential as well.
There was a problem hiding this comment.
@JoshuaLai Looking at your git history, you've switched to ClientSecretCredential in one commit which failed, then you added another commit that adds the missing env vars which passed. I don't think the switch to ClientSecretCredential was necessary, just the missing env vars were the culprit.
With the env vars present, DefaultAzureCredential should use EnvironmentCreedential which then uses ClientSecretCredential. So could you switch back to DefaultAzureCredential and try again? :-)
|
/azp run js - communication-administration - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
No description provided.