Skip to content

[RBAC] az ad group show: fix --group value treated as regex problem #12426

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.

Sign up for GitHub

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

Merged
arrownj merged 7 commits into from
Mar 6, 2020
Merged

[RBAC] az ad group show: fix --group value treated as regex problem #12426

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ba43dc7
[RBAC] az ad group show: fix --group value treated as regex problem
arrownj Mar 3, 2020
e3ec50b
{RBAC} update syntax error of error message
arrownj Mar 4, 2020
04f4e5a
{RBAC} retrieve domain from az ad signed-in-user show instead of hard…
arrownj Mar 4, 2020
a7588bf
{RBAC} refactor validate_group function
arrownj Mar 5, 2020
11d1fbb
{RBAC} update recording file for test_graph_group_idempotent
arrownj Mar 5, 2020
91e4051
{RBAC} update test_graph
arrownj Mar 5, 2020
73d9004
{RBAC} update test_graph to use the right user info
arrownj Mar 5, 2020
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
30 changes: 18 additions & 12 deletions src/azure-cli/azure/cli/command_modules/role/_validators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,25 +13,31 @@
VARIANT_GROUP_ID_ARGS = ['object_id', 'group_id', 'group_object_id']


def _validate_group(namespace, attr, value, group_filter, has_next_filter):
Copy link
Member

@jiasli jiasli Mar 4, 2020

Choose a reason for hiding this comment

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

Personally, I think the usage of has_next_filter lowers the cohesion of this function. It makes _validate_group rely on the external logic and dependent between executions. I prefer not to exact the logic to _validate_group but put it in the original validate_group instead.

Copy link
Contributor Author

@arrownj arrownj Mar 4, 2020

Choose a reason for hiding this comment

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

I'm also hesitated at it. @qianwens , want to hear your opinions.

Copy link
Member

@qianwens qianwens Mar 5, 2020

Choose a reason for hiding this comment

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

I think it is correct to have _validate_group function or the handle exception logic in validate_group looks complicated. But for _validate_group, I agree that we should not raise different kind of error message based on the has_next_filter paramete. Currently _validate_group 's return logic is not predictable, sometime it return false, sometime it raise error, could you please refine the logic here, or you can replace this functio with _get_group_count to just return the count and let the validate_group to raise the exception

In reply to: 387564846 [](ancestors = 387564846)

Copy link
Contributor Author

@arrownj arrownj Mar 5, 2020

Choose a reason for hiding this comment

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

updated

client = _graph_client_factory(namespace.cmd.cli_ctx)
result = list(client.groups.list(filter=group_filter))
count = len(result)
ret = False
if count == 1:
setattr(namespace, attr, result[0].object_id)
ret = True
elif count == 0:
if not has_next_filter:
raise CLIError("No group matches the name of '{}'".format(value))
else:
raise CLIError("More than one groups match the name of '{}'".format(value))
return ret


def validate_group(namespace):
# For AD auto-commands, here we resolve logic names to object ids needed by SDK methods
attr, value = next(((x, getattr(namespace, x)) for x in VARIANT_GROUP_ID_ARGS
if hasattr(namespace, x)))
try:
uuid.UUID(value)
except ValueError:
client = _graph_client_factory(namespace.cmd.cli_ctx)
sub_filters = []
sub_filters.append("startswith(displayName,'{}')".format(value))
sub_filters.append("displayName eq '{}'".format(value))
result = list(client.groups.list(filter=' or '.join(sub_filters)))
count = len(result)
if count == 1:
setattr(namespace, attr, result[0].object_id)
elif count == 0:
raise CLIError("No group matches the name of '{}'".format(value))
else:
raise CLIError("More than one groups match the name of '{}'".format(value))
if not _validate_group(namespace, attr, value, "displayName eq '{}'".format(value), True):
_validate_group(namespace, attr, value, "startswith(displayName,'{}')".format(value), False)


def validate_member_id(namespace):
Expand Down
Loading