-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[core-auth] Add AzureNamedKeyCredential class and NamedKeyCredential interface #14371
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT license. | ||
|
|
||
| /** | ||
| * Represents a credential defined by a static API name and key. | ||
| */ | ||
| export interface NamedKeyCredential { | ||
| /** | ||
| * The value of the API key represented as a string | ||
| */ | ||
| readonly key: string; | ||
| /** | ||
| * The value of the API name represented as a string. | ||
| */ | ||
| readonly name: string; | ||
| } | ||
|
|
||
| /** | ||
| * A static name/key-based credential that supports updating | ||
| * the underlying name and key values. | ||
| */ | ||
| export class AzureNamedKeyCredential implements NamedKeyCredential { | ||
| private _key: string; | ||
| private _name: string; | ||
|
|
||
| /** | ||
| * The value of the key to be used in authentication. | ||
| */ | ||
| public get key(): string { | ||
| return this._key; | ||
| } | ||
|
|
||
| /** | ||
| * The value of the name to be used in authentication. | ||
| */ | ||
| public get name(): string { | ||
| return this._name; | ||
| } | ||
|
|
||
| /** | ||
| * Create an instance of an AzureNamedKeyCredential for use | ||
| * with a service client. | ||
| * | ||
| * @param name - The initial value of the name to use in authentication. | ||
| * @param key - The initial value of the key to use in authentication. | ||
| */ | ||
| constructor(name: string, key: string) { | ||
| if (!name || !key) { | ||
| throw new TypeError("name and key must be non-empty strings"); | ||
| } | ||
|
|
||
| this._name = name; | ||
| this._key = key; | ||
| } | ||
|
|
||
| /** | ||
| * Change the value of the key. | ||
| * | ||
| * Updates will take effect upon the next request after | ||
| * updating the key value. | ||
| * | ||
| * @param newName - The new name value to be used. | ||
| * @param newKey - The new key value to be used. | ||
| */ | ||
| public update(newName: string, newKey: string): void { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what if I just want to update one? Leaving the other empty seems like a not unreasonable contract compared to having to set it to its existing value
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case I went just based on the requirements for the linked issue:
I think it may make the API a bit confusing if we allow setting them separately. I'd expect the I could obviously flip the position of name/key...but this would be opposite of what the other languages are doing, and mentally I think of the key being associated to the policy name, not the other way around. I could expose
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm just a little confused, since I would expect updating the key to be way more common than the name, but perhaps I'm not understanding the name field's purpose very well. I agree that it's better to not ship too much surface area at once, but are other languages also fine with this kinda clunky contract?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jsquire
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes; the update/read is intended to be atomic to avoid race conditions with concurrent use. For those languages with threads, allowing them to be set individually offers the chance for inconsistent data.
This was the design that the architects approved, with the trade-offs made to favor safe concurrent use. The goal was to ensure that we didn't provide overloads that callers may be tempted to use in a manner that could produce undesired results. You may want to have a conversation with Brian since threading isn't a consideration here.
The credential is intended for use when a service requires both the key name and key value to generate the value needed for authorization. The name isn't meant to be informational, but part of the auth flow. Not sure if that helps at all... 😄
The intention is that updates will always provide both values; essentially, you're fully reinitializing the state. If the name is not provided, I would expect that the current value be updated to
I would agree. However, it is a valid use case to generate a new access policy and need to replace the existing one, which requires the name and key.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm once again relieved to not have to think about threading concerns in JS. :) |
||
| if (!newName || !newKey) { | ||
| throw new TypeError("newName and newKey must be non-empty strings"); | ||
| } | ||
|
|
||
| this._name = newName; | ||
| this._key = newKey; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any particular reason these are getters instead of normal props?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were wanting the user to have to call
update()to update the key, and otherwise treat these as readonly properties. This is the pattern the other credentials in this package (e.g.AzureKeyCredential) follows as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that makes sense