Skip to content
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

[Batch] Rollback the support for TokenCredential #16870

Merged
merged 4 commits into from
Aug 12, 2021

Conversation

deyaaeldeen
Copy link
Member

Batch has been advertising using shared keys as their primary way of authentication in their docs. #16679 added support for TokenCredential authentication but it turned out that batch is not compatible with the support for token credentials in @azure/ms-rest-js because the latter assumes a scope that only works with management-plane libs.

If we want to fix the support for AAD, we will have to add support for custom scopes in @azure/ms-rest-js so we can pass the batch scope https://batch.core.windows.net//.default to it and also update azureTokenCredential as follows:

azureTokenCredential.msalFlow.identityClient.authorityHost = "https://login.microsoftonline.com/<tenant id>";
azureTokenCredential.msalFlow.identityClient.baseUri = "https://login.microsoftonline.com/<tenant id>";
azureTokenCredential.msalFlow.tenantId = "<tenant id>";

Because of the hassle of doing the fix and the fact that AAD support was not advertisied in batch to begin with, we want to go ahead and revert the support for TokenCredential instead.

sdk/batch/batch/package.json Outdated Show resolved Hide resolved
sdk/batch/batch/README.md Outdated Show resolved Hide resolved

### How to use
1. Use `AzureCliCredentials` exported from `@azure/ms-rest-nodeauth`.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the benefit of showing the Azure CLI option over the API key one? Can we skip this and showcase the API key scenario as that is what is shown in the official samples as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was the readme used earlier, so I kept them to show case how to use nodeauth. To address your feedback, I deleted the CLI one and kept the shared key and the other nodeauth one.

sdk/batch/batch/README.md Show resolved Hide resolved
sdk/batch/batch/src/batchServiceClient.ts Show resolved Hide resolved
@deyaaeldeen
Copy link
Member Author

@ramya-rao-a please take a look now.

Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>
@deyaaeldeen deyaaeldeen merged commit 44eb526 into Azure:main Aug 12, 2021
@deyaaeldeen deyaaeldeen deleted the batch/remove-aad branch August 12, 2021 18:37
ckairen pushed a commit to ckairen/azure-sdk-for-js that referenced this pull request Aug 24, 2021
* [Batch] Rollback the support for TokenCredential

* address feedback

* edit docstring

* Update sdk/batch/batch/src/batchServiceClientContext.ts

Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>

Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Nov 25, 2021
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Nov 25, 2021
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Nov 25, 2021
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Nov 25, 2021
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Nov 25, 2021
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Nov 25, 2021
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jan 5, 2022
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.

2 participants