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

[DO NOT MERGE] Adding claims parameter to GetTokenOptions #13595

Closed
wants to merge 2 commits into from

Conversation

schaabs
Copy link
Member

@schaabs schaabs commented Feb 4, 2021

Adding the optional claims parameter to getToken to support CAE claims challenges and additional claims.

@ghost ghost added the Azure.Core label Feb 4, 2021
@schaabs schaabs requested a review from sadasant February 4, 2021 20:19
/**
* Additional claims to be included in the token. See {@link https://openid.net/specs/openid-connect-core-1_0-final.html#ClaimsParameter} for more information on format and content.
*/
claims?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since CAE has been pushed out of the current milestone, am guessing this is not needed for the Feb release? @sadasant Let's make sure @azure/core-auth is released before merging this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if this needs to be released or not. It seemed like it needed to be released. @schaabs please confirm.

Copy link
Member Author

Choose a reason for hiding this comment

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

It needs to be released before we can release any CAE support in core-http or identity. So it doesn't have to be released now, but if not we'll need to include this with our out of band releases for CAE support. All that said, I don't see any harm in releasing it now.

@sadasant sadasant requested a review from chradek as a code owner February 4, 2021 23:46
@sadasant
Copy link
Contributor

sadasant commented Feb 4, 2021

Added @ellismg since he's waiting for core-auth to be released to use it on eventgrid.

@sadasant sadasant requested a review from ellismg February 4, 2021 23:59
Copy link
Member

@ellismg ellismg left a comment

Choose a reason for hiding this comment

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

Assuming that @ramya-rao-a is happy with the plan of record of including this, the code change itself LGTM.

For the record, EventGrid is indifferent on if this included in the release or not, but we do need the SAS stuff that landed earlier, so I'm 100% fine if you also hold off on this and then do some out of band release with the CAE stuff later.

Copy link
Contributor

@sadasant sadasant left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

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

CAE support would be in beta, and we are yet to discuss whether core-auth would ship a beta or a patch/minor update to support the CAE features. So, my recommendation would be to release the core-auth from the master branch so that Event Grid is unblocked, and then have an out of band release for core-auth as per CAE requirements

Adding a "request changes" to this PR to hold off on the PR merge until core-auth is released

@sadasant sadasant changed the title Adding claims parameter to GetTokenOptions [DO NOT MERGE] Adding claims parameter to GetTokenOptions Feb 10, 2021
@sadasant sadasant self-assigned this Feb 10, 2021
@sadasant
Copy link
Contributor

sadasant commented Mar 1, 2021

Closed on favor of: #13888

@sadasant sadasant closed this Mar 1, 2021
@sadasant sadasant deleted the add-gettoken-claims branch March 1, 2021 22:53
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Mar 24, 2021
Revert "[Management Plane][Azure Search] Adding new API version 2021-01-04. (Azure#13355)" (Azure#13595)

This reverts commit 5c2185adda8aa915a391ae182c1f02cc587f4f79.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants