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

[Identity] OnBehalfOfCredential #16995

Closed
wants to merge 15 commits into from

Conversation

sadasant
Copy link
Contributor

@sadasant sadasant commented Aug 20, 2021

This PR implements the OnBehalfOfCredential. This design to a while to figure out. You can read more about previous ideas through:

After careful consideration of the possible impacts of all the options, we arrived back at the design originally proposed by @schaabs : To pass the userAssertion as an object besides the tracing parameters on the ServiceClient methods.

This approach looks as follows:

const tokenCredential = new OnBehalfOfCredential(tenantId, clientId, clientSecret);
const client = new KeyClient("vault-url", tokenCredential);

const userAssertions = createUserAssertion("access-token");
await client.getKey("key-name", { authenticationOptions: { userAssertion } });

Related to #15804
Fixes #17133


Work on a separate PR:

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

I think this is shaping up nicely! I wish there was an easy way to avoid sprinkling AuthenticationOptions in so many places, but maybe this is the simplest solution and at least we'll have a place for other auth-related properties in the future

sdk/core/core-auth/review/core-auth.api.md Outdated Show resolved Hide resolved
sdk/core/core-auth/src/authenticationOptions.ts Outdated Show resolved Hide resolved
sdk/core/core-rest-pipeline/src/util/tokenCycler.ts Outdated Show resolved Hide resolved
sdk/core/core-rest-pipeline/src/util/tokenCycler.ts Outdated Show resolved Hide resolved
@sadasant
Copy link
Contributor Author

Tests are failing on Node12 on Windows. I’ll investigate

@ghost ghost added the no-recent-activity There has been no recent activity on this issue. label Feb 25, 2022
@ghost
Copy link

ghost commented Feb 25, 2022

Hi @sadasant. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

@sadasant
Copy link
Contributor Author

More complex designs of the OBO credential are not on the table for the foreseeable future. Closing this PR.

@sadasant sadasant closed this Feb 25, 2022
@sadasant sadasant deleted the identity/15804-obo-final branch February 25, 2022 16:41
@sadasant sadasant restored the identity/15804-obo-final branch February 25, 2022 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core Azure.Identity no-recent-activity There has been no recent activity on this issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Identity] Implement the advanced approach of the OnBehalfOfCredential
2 participants