Add KeyVaultAccessControlClient for data plane RBAC #13372
Conversation
chlowell
added
KeyVault
Client
chlowell
requested review from
sadasant,
christothes,
heaths,
g2vinay and
mccoyp
...yvault/azure-keyvault-administration/azure/keyvault/administration/_access_control_client.py
Show resolved
Hide resolved
| :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 |
There was a problem hiding this comment.
should be ~azure.keyvault.administration.RoleAssignment
There was a problem hiding this comment.
Sphinx links this correctly, the class is imported in this module.
There was a problem hiding this comment.
oh interesting, do you know for sure if microsoft docs will?
There was a problem hiding this comment.
I don't. Trial balloon! 🎈
...yvault/azure-keyvault-administration/azure/keyvault/administration/_access_control_client.py
Outdated
Show resolved
Hide resolved
| """Role definition permissions. | ||
|
|
||
| :ivar list[str] actions: allowed actions | ||
| :ivar list[str] not_actions: denied actions |
There was a problem hiding this comment.
side note: not sure if this has already been a design discussion, but I feel like denied_actions sounds better than not_actions
There was a problem hiding this comment.
Sure, that's reasonable. I don't know whether there's been a discussion either, I just copied this from @christothes 😊
There was a problem hiding this comment.
I'm using denied actions as well.
| assert definition.description is not None | ||
| assert definition.id is not None | ||
| assert definition.name is not None | ||
| assert len(definition.permissions) |
There was a problem hiding this comment.
is it possible for things like permissions, name etc to assert on the value?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
you're right, i think we can leave it for now (i can't think of anything either)
sdk/keyvault/azure-keyvault-administration/azure/keyvault/administration/_models.py
Outdated
Show resolved
Hide resolved
sdk/keyvault/azure-keyvault-administration/azure/keyvault/administration/_models.py
Outdated
Show resolved
Hide resolved
sdk/keyvault/azure-keyvault-administration/azure/keyvault/administration/_models.py
Outdated
Show resolved
Hide resolved
| # type: (str, Union[str, UUID], str, str, **Any) -> RoleAssignment | ||
| """Create a role assignment. | ||
|
|
||
| :param str role_scope: scope the role assignment will apply over |
There was a problem hiding this comment.
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 was a problem hiding this comment.
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 🏈
There was a problem hiding this comment.
Makes sense - perhaps just some hints in the comment text would be enough.
...yvault/azure-keyvault-administration/azure/keyvault/administration/_access_control_client.py
Outdated
Show resolved
Hide resolved
| from ._models import KeyVaultPermission, RoleAssignment, RoleDefinition | ||
|
|
||
|
|
||
| __all__ = ["ApiVersion", "KeyVaultAccessControlClient", "KeyVaultPermission", "RoleAssignment", "RoleDefinition"] |
There was a problem hiding this comment.
Regrading ApiVersion, is this the standard in Python? In JavaScript we are standardizing on serviceVersion.
There was a problem hiding this comment.
Yes, it's consistent with the Python SDK.
| # pylint:disable=protected-access | ||
|
|
||
| @distributed_trace | ||
| def create_role_assignment(self, role_scope, role_assignment_name, role_definition_id, principal_id, **kwargs): |
There was a problem hiding this comment.
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.
…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) ...
Closes #9755.