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

udpate access token url on app service #20292

Merged
merged 5 commits into from
Apr 2, 2021

Conversation

yiliuTo
Copy link
Member

@yiliuTo yiliuTo commented Mar 31, 2021

Replace parameter from objectid to clientid of the access token url on App service.

@ghost ghost added KeyVault azure-spring All azure-spring related issues labels Mar 31, 2021
Copy link
Contributor

@backwind1233 backwind1233 left a comment

Choose a reason for hiding this comment

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

nice

if (identity != null) {
url.append("&clientid=").append(identity);
if (clientId != null) {
url.append("&clientid=").append(clientId);
Copy link
Contributor

Choose a reason for hiding this comment

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

what if move "LOGGER.log(INFO, "Using managed identity with client ID: {0}", clientId);" here

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi, what is your concern of it? I think it's helpful to inform users about the flow of acquiring tokens and also debug.

Choose a reason for hiding this comment

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

IMU, @backwind1233 's concern is that if (clientId != null) appeared more than one time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link

@chenrujun chenrujun left a comment

Choose a reason for hiding this comment

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

LGTM.

@yiliuTo
Copy link
Member Author

yiliuTo commented Apr 2, 2021

/check-enforcer override

@yiliuTo yiliuTo merged commit 601ba46 into Azure:master Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
azure-spring All azure-spring related issues KeyVault
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants