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

Add AzureNamedKeyCredential #17548

Merged
merged 19 commits into from
Apr 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion sdk/core/azure-core/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
# Release History

## 1.13.1 (Unreleased)
## 1.14.0 (Unreleased)

### New Features

- Added `azure.core.credentials.AzureNamedKeyCredential` credential #17548.
rakshith91 marked this conversation as resolved.
Show resolved Hide resolved

## 1.13.0 (2021-04-02)

Expand Down
2 changes: 1 addition & 1 deletion sdk/core/azure-core/azure/core/_version.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,4 @@
# regenerated.
# --------------------------------------------------------------------------

VERSION = "1.13.1"
VERSION = "1.14.0"
47 changes: 43 additions & 4 deletions sdk/core/azure-core/azure/core/credentials.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@
# Licensed under the MIT License. See LICENSE.txt in the project root for
# license information.
# -------------------------------------------------------------------------
from collections import namedtuple
from typing import TYPE_CHECKING
import six


if TYPE_CHECKING:
from typing import Any, NamedTuple
from typing_extensions import Protocol
Expand All @@ -26,11 +26,12 @@ def get_token(self, *scopes, **kwargs):


else:
from collections import namedtuple

AccessToken = namedtuple("AccessToken", ["token", "expires_on"])

__all__ = ["AzureKeyCredential", "AzureSasCredential", "AccessToken"]
AzureNamedKey = namedtuple("AzureNamedKey", ["name", "key"])


__all__ = ["AzureKeyCredential", "AzureSasCredential", "AccessToken", "AzureNamedKeyCredential"]


class AzureKeyCredential(object):
Expand Down Expand Up @@ -111,3 +112,41 @@ def update(self, signature):
if not isinstance(signature, six.string_types):
raise TypeError("The signature used for updating must be a string.")
self._signature = signature


class AzureNamedKeyCredential(object):
"""Credential type used for working with any service needing a named key that follows patterns
established by the other credential types.

:param str name: The name of the credential used to authenticate to an Azure service.
:param str key: The key used to authenticate to an Azure service.
:raises: TypeError
"""
def __init__(self, name, key):
# type: (str, str) -> None
if not isinstance(name, six.string_types) or not isinstance(key, six.string_types):
raise TypeError("Both name and key must be strings.")
self._credential = AzureNamedKey(name, key)

@property
def named_key(self):
# type () -> AzureNamedKey
"""The value of the configured name.

:rtype: AzureNamedKey
"""
return self._credential

def update(self, name, key):
Copy link
Member

Choose a reason for hiding this comment

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

Do we have scenarios where you want to update/change the name of the key?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think one scenario could be that users want to use a new share access policy which has new name and key.

Copy link
Member

Choose a reason for hiding this comment

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

Can user update key only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

technically - they are allowed to provide the same name - but they must provide a name

Copy link
Member

Choose a reason for hiding this comment

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

I think the most common scenario is the name is fixed after creation, but user may modify the key value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yunhaoling any thoughts on this?

If we know that users may normally not modify the name, I'll incline towards making name kwarg only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually on second thought, i think we should let this be - as much as i like kwargs, it doesn't seem like they'll grow later in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also, if the user thinks they wont need to change name for their use case, AzureKeyCredential serves their purpose

Copy link
Member

Choose a reason for hiding this comment

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

...unless they need to initially specify the name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, right - still doesn't change tht kwargs would not grow in this case - so will stick to it

# type: (str, str) -> None
"""Update the named key credential.

Both name and key must be provided in order to update the named key credential.
Individual attributes cannot be updated.

:param str name: The name of the credential used to authenticate to an Azure service.
:param str key: The key used to authenticate to an Azure service.
"""
if not isinstance(name, six.string_types) or not isinstance(key, six.string_types):
raise TypeError("Both name and key must be strings.")
self._credential = AzureNamedKey(name, key)
30 changes: 28 additions & 2 deletions sdk/core/azure-core/tests/test_authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,13 @@
import time

import azure.core
from azure.core.credentials import AccessToken, AzureKeyCredential, AzureSasCredential
from azure.core.credentials import AccessToken, AzureKeyCredential, AzureSasCredential, AzureNamedKeyCredential
from azure.core.exceptions import ServiceRequestError
from azure.core.pipeline import Pipeline
from azure.core.pipeline.policies import BearerTokenCredentialPolicy, SansIOHTTPPolicy, AzureKeyCredentialPolicy, AzureSasCredentialPolicy
from azure.core.pipeline.policies import (
BearerTokenCredentialPolicy, SansIOHTTPPolicy, AzureKeyCredentialPolicy,
AzureSasCredentialPolicy
)
from azure.core.pipeline.transport import HttpRequest

import pytest
Expand Down Expand Up @@ -230,3 +233,26 @@ def test_azure_sas_credential_policy_raises():
sas = 1234
with pytest.raises(TypeError):
credential = AzureSasCredential(sas)

def test_azure_named_key_credential():
cred = AzureNamedKeyCredential("sample_name", "samplekey")

assert cred.named_key.name == "sample_name"
assert cred.named_key.key == "samplekey"
assert isinstance(cred.named_key, tuple)

cred.update("newname", "newkey")
assert cred.named_key.name == "newname"
assert cred.named_key.key == "newkey"
Copy link
Member

Choose a reason for hiding this comment

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

We should have a test to make sure that we are returning a tuple as well...

Copy link
Contributor Author

@rakshith91 rakshith91 Apr 25, 2021

Choose a reason for hiding this comment

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

added - for clarification, we don't really return a tuple, we update the cred with a new tuple

assert isinstance(cred.named_key, tuple)

def test_azure_named_key_credential_raises():
with pytest.raises(TypeError, match="Both name and key must be strings."):
cred = AzureNamedKeyCredential("sample_name", 123345)

cred = AzureNamedKeyCredential("sample_name", "samplekey")
assert cred.named_key.name == "sample_name"
assert cred.named_key.key == "samplekey"

with pytest.raises(TypeError, match="Both name and key must be strings."):
cred.update(1234, "newkey")