Add KeyVaultAccessControlClient for data plane RBAC by chlowell · Pull Request #13372 · Azure/azure-sdk-for-python

chlowell · 2020-08-27T17:14:28Z

Closes #9755.

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

iscai-msft · 2020-08-27T17:20:12Z

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

+        :param str role_definition_id: ID of the role's definition
+        :param str principal_id: ID of the principal which will be assigned the role. This maps to the ID inside the
+            Active Directory. It can point to a user, service principal, or security group.
+        :rtype: RoleAssignment


should be ~azure.keyvault.administration.RoleAssignment

Sphinx links this correctly, the class is imported in this module.

oh interesting, do you know for sure if microsoft docs will?

I don't. Trial balloon! 🎈

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

iscai-msft · 2020-08-27T17:24:16Z

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

+    """Role definition permissions.
+
+    :ivar list[str] actions: allowed actions
+    :ivar list[str] not_actions: denied actions


side note: not sure if this has already been a design discussion, but I feel like denied_actions sounds better than not_actions

Sure, that's reasonable. I don't know whether there's been a discussion either, I just copied this from @christothes 😊

I'm using denied actions as well.

iscai-msft · 2020-08-27T17:26:06Z

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

+            assert definition.description is not None
+            assert definition.id is not None
+            assert definition.name is not None
+            assert len(definition.permissions)


is it possible for things like permissions, name etc to assert on the value?

Not without prior knowledge of the values, or another API that enables checking our deserialization (I don't see a candidate). Nothing's coming to my mind short of hardcoding what appear to be the current defaults; I'm reluctant to do that. Any ideas?

you're right, i think we can leave it for now (i can't think of anything either)

iscai-msft

christothes

LGTM, just left some naming suggestions for consistency

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

christothes · 2020-08-28T20:53:11Z

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

+        # type: (str, Union[str, UUID], str, str, **Any) -> RoleAssignment
+        """Create a role assignment.
+
+        :param str role_scope: scope the role assignment will apply over


For the other languages we are using a custom type to provide some context for the valid values that can be passed here ("/", "/keys", "/keys/some specific Key resource Id"). Is there a pythonic way to accomplish the same thing here?

There's nothing I know of in the standard library quite like .NET's Uri. Typical Python just passes strings around. I think validating the argument with a regex is the best we could do here. I think it's unlikely but not inconceivable we'd do that; for today I'll punt 🏈

Makes sense - perhaps just some hints in the comment text would be enough.

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

sadasant · 2020-08-28T22:07:33Z

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

+from ._models import KeyVaultPermission, RoleAssignment, RoleDefinition
+
+
+__all__ = ["ApiVersion", "KeyVaultAccessControlClient", "KeyVaultPermission", "RoleAssignment", "RoleDefinition"]


Regrading ApiVersion, is this the standard in Python? In JavaScript we are standardizing on serviceVersion.

Yes, it's consistent with the Python SDK.

sadasant · 2020-08-28T22:11:05Z

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

+    # pylint:disable=protected-access
+
+    @distributed_trace
+    def create_role_assignment(self, role_scope, role_assignment_name, role_definition_id, principal_id, **kwargs):


We're using just name in JavaScript. Should I change it to something like roleAssignmentName? Or perhaps we could change it to name here? I'm tempted to lean towards shorter parameters if possible.

iscai-msft

🚢

…into link_om_sample * 'master' of https://github.com/Azure/azure-sdk-for-python: (23 commits) Int32 serialization (Azure#13452) add output to opinion mining sample (Azure#13494) Add Document w/ Eng Sys Checks (Azure#13492) update version (Azure#13495) Remove resources post test (Azure#13379) bing_id -> bing_entity_search_api_id (Azure#13491) [EventGrid] Read me + improve docstrings (Azure#13484) Build AuthenticationRecords from ADFS identity tokens (Azure#13341) Support Subject Name/Issuer authentication (Azure#13350) Add KeyVaultAccessControlClient for data plane RBAC (Azure#13372) [text analytics] Add redacted_text (Azure#13449) add python sdk sample (Azure#13338) [text analytics] add versionadded sphinx documentation (Azure#13450) [text analytics] add bing_id property to LinkedEntity class (Azure#13446) fix typing for paging methods (Azure#13410) [text analytics] add domain_filter param (Azure#13451) fix issue Azure#11658 for is_valid_resource_id (Azure#11709) added create_table_if_not_exists method to table service client (Azure#13385) [ServiceBus] Test and failure improvements (Azure#13345) Proper encoding and decoding of source URLs - Fixes special characters in source URL issue (Azure#13275) ...

chlowell added 3 commits August 27, 2020 09:31

models

cbac8aa

access control clients

47902d2

tests

88c5ba6

chlowell added KeyVault Client This issue points to a problem in the data-plane of the library. labels Aug 27, 2020

chlowell requested review from christothes, g2vinay, heaths, mccoyp and sadasant August 27, 2020 17:14

chlowell requested review from iscai-msft and schaabs as code owners August 27, 2020 17:14

iscai-msft reviewed Aug 27, 2020

View reviewed changes

correct type annotations

90f4ccc

iscai-msft previously approved these changes Aug 27, 2020

View reviewed changes

christothes reviewed Aug 28, 2020

View reviewed changes

sadasant mentioned this pull request Aug 28, 2020

[Key Vault Admin] Convenience layer - KeyVaultAccessControlClient Azure/azure-sdk-for-js#10815

Merged

sadasant reviewed Aug 28, 2020

View reviewed changes

prefix models with "KeyVault"

9765bfe

sadasant reviewed Aug 28, 2020

View reviewed changes

chlowell added 2 commits August 28, 2020 15:25

clearer documentation of principal_id

f675ce7

document the form of role_scope

79ee5de

chlowell dismissed iscai-msft’s stale review via 79ee5de August 28, 2020 22:50

chlowell requested a review from iscai-msft August 31, 2020 19:06

iscai-msft approved these changes Aug 31, 2020

View reviewed changes

chlowell merged commit 6930938 into Azure:master Sep 1, 2020

chlowell deleted the rbac branch September 1, 2020 16:09

		from ._models import KeyVaultPermission, RoleAssignment, RoleDefinition


		__all__ = ["ApiVersion", "KeyVaultAccessControlClient", "KeyVaultPermission", "RoleAssignment", "RoleDefinition"]

Conversation

chlowell commented Aug 27, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iscai-msft left a comment

Choose a reason for hiding this comment

Uh oh!

christothes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iscai-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants