[Key Vault] Add support for custom role definitions #16063

mccoyp · 2021-01-09T01:56:33Z

Resolves #14938.

This adds set_role_definition, get_role_definition, and delete_role_definition methods to Administration's KeyVaultAccessControlClient. set_role_definition makes permissions a required parameter in an attempt to make it more clear that a definition without defined permissions is undesirable.

...yvault/azure-keyvault-administration/azure/keyvault/administration/_access_control_client.py

sdk/keyvault/azure-keyvault-administration/azure/keyvault/administration/_models.py

...lt/azure-keyvault-administration/azure/keyvault/administration/aio/_access_control_client.py

sdk/keyvault/azure-keyvault-administration/tests/test_access_control.py

sdk/keyvault/azure-keyvault-administration/azure/keyvault/administration/_models.py

chlowell · 2021-01-20T20:26:55Z

sdk/keyvault/azure-keyvault-administration/azure/keyvault/administration/_enums.py

+    keys_value = "/keys"  #: use this if you want role assignments to apply to all keys
+
+
+class DataActionPermission(str, Enum):


I expect we'll want a "KeyVault" prefix because the values apply only to Key Vault, and other services will have their own actions. Also, are these values better characterized as operations or actions than permissions?

/cc @christothes that's a good idea for all languages as well.

Hm... since these would be used to populate a KeyVaultPermission object and they do describe actions, I agree that something like KeyVaultDataAction might make more sense.

sdk/keyvault/azure-keyvault-administration/tests/test_access_control.py

sdk/keyvault/azure-keyvault-administration/tests/test_access_control_async.py

heaths

Public APIs look good.

heaths · 2021-01-20T21:36:53Z

sdk/keyvault/azure-keyvault-administration/azure/keyvault/administration/_enums.py

+class KeyVaultRoleScope(str, Enum):
+    """Collection of well known role scopes. This list is not exhaustive"""
+
+    global_value = "/"  #: use this if you want role assignments to apply to everything on the resource


Is the "_value" suffix common in Python? In .NET we just use "Global" and "Keys". If so, cool.

"global" is a keyword. I forget how we ended up with _value here; looking at it again I like "global_scope" and "keys". Another option I kind of like is CAPS which I think other libraries have used (sadly other Key Vault libraries use lowercase). What do you think @mccoyp?

If we decide to go with a _scope suffix, I'd suggest we do it for both.

Thinking about how a user would use the enum and because "Scope" is already in the class name, I think "global" and "keys" would make sense, without a suffix. I also wondered about whether these should be in caps -- I kept them in snake case for consistency with other libraries, but I was under the impression that enum values should be in caps

I'm happy with caps; consistency is less important to me than making the better choice here.

heaths · 2021-01-20T21:37:32Z

sdk/keyvault/azure-keyvault-administration/azure/keyvault/administration/_enums.py

+    keys_value = "/keys"  #: use this if you want role assignments to apply to all keys
+
+
+class DataActionPermission(str, Enum):


/cc @christothes that's a good idea for all languages as well.

heaths · 2021-01-20T21:38:41Z

sdk/keyvault/azure-keyvault-administration/azure/keyvault/administration/_enums.py

+    """Supported permissions for data actions.
+    """
+
+    #: Read HSM key metadata.


Make sure these names align with what @christothes defined in the swagger PR (not merged just yet but should be soon).

mccoyp · 2021-01-20T23:26:27Z

sdk/keyvault/azure-keyvault-administration/azure/keyvault/administration/_enums.py

+    KEYS = "/keys"  #: use this if you want role assignments to apply to all keys
+
+
+class KeyVaultDataAction(str, Enum):


How do we feel with this enum name?

chlowell · 2021-01-22T00:12:13Z

...yvault/azure-keyvault-administration/azure/keyvault/administration/_access_control_client.py

+        :param role_scope: scope of the role definition. :class:`KeyVaultRoleScope` defines common broad scopes.
+            Specify a narrower scope as a string. Managed HSM only supports '/', or KeyVaultRoleScope.global_value.
+        :type role_scope: str or KeyVaultRoleScope
+        :param role_definition_name: the role definition's name. Must be a UUID.


Suggested change

:param role_definition_name: the role definition's name. Must be a UUID.

:param role_definition_name: the role definition's name.

Thinking the form of the name isn't interesting because this role definition already exists and so must have a compliant name.

mccoyp added KeyVault Client This issue points to a problem in the data-plane of the library. labels Jan 9, 2021

mccoyp added this to the [2021] February milestone Jan 9, 2021

chlowell reviewed Jan 11, 2021

View reviewed changes

mccoyp force-pushed the feature/roledefinition branch from 927eb2a to 6863c81 Compare January 13, 2021 01:58

mccoyp added 2 commits January 12, 2021 18:51

Implement sync API

d24ecaa

Implement async API

8b3acbd

mccoyp force-pushed the feature/roledefinition branch from 6863c81 to 8b3acbd Compare January 13, 2021 02:52

mccoyp added 4 commits January 13, 2021 18:06

Add sync tests, update public model

cc4717f

Add async tests, update Definitions model

baf412e

Add sync test recordings

b632199

Add async test recordings

93461c2

chlowell reviewed Jan 19, 2021

View reviewed changes

Re-generate with approved swagger changes

b38a0d2

mccoyp force-pushed the feature/roledefinition branch from 6605d77 to 386da11 Compare January 20, 2021 05:53

Add enum, address feedback

00481f3

mccoyp force-pushed the feature/roledefinition branch from 386da11 to 00481f3 Compare January 20, 2021 06:03

mccoyp marked this pull request as ready for review January 20, 2021 06:06

mccoyp requested a review from schaabs as a code owner January 20, 2021 06:06

mccoyp requested review from heaths, christothes and chlowell January 20, 2021 06:07

Update KeyVaultRoleScope location

4738f4a

chlowell reviewed Jan 20, 2021

View reviewed changes

heaths approved these changes Jan 20, 2021

View reviewed changes

Address feedback

25be3af

mccoyp commented Jan 20, 2021

View reviewed changes

mccoyp added 2 commits January 21, 2021 10:25

Add test recordings

c3ab528

Apply naming changes

f6dd2a9

chlowell approved these changes Jan 22, 2021

View reviewed changes

Thanks, Charles!

f0c5d83

mccoyp merged commit 42397a9 into Azure:master Jan 22, 2021

mccoyp deleted the feature/roledefinition branch January 22, 2021 02:04

mccoyp mentioned this pull request Feb 5, 2021

[Key Vault] Improved custom role definition support in administration #16556

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Key Vault] Add support for custom role definitions #16063

[Key Vault] Add support for custom role definitions #16063

mccoyp commented Jan 9, 2021 •

edited

Loading

chlowell Jan 20, 2021

heaths Jan 20, 2021

mccoyp Jan 20, 2021

heaths left a comment

heaths Jan 20, 2021

chlowell Jan 20, 2021 •

edited

Loading

christothes Jan 20, 2021

mccoyp Jan 20, 2021

chlowell Jan 20, 2021

heaths Jan 20, 2021

heaths Jan 20, 2021

mccoyp Jan 20, 2021

chlowell Jan 22, 2021

		keys_value = "/keys" #: use this if you want role assignments to apply to all keys


		class DataActionPermission(str, Enum):

		KEYS = "/keys" #: use this if you want role assignments to apply to all keys


		class KeyVaultDataAction(str, Enum):

	:param role_definition_name: the role definition's name. Must be a UUID.
	:param role_definition_name: the role definition's name.

[Key Vault] Add support for custom role definitions #16063

[Key Vault] Add support for custom role definitions #16063

Conversation

mccoyp commented Jan 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heaths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chlowell Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccoyp commented Jan 9, 2021 •

edited

Loading

chlowell Jan 20, 2021 •

edited

Loading