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

[azure-mgmt-core] is_valid_resource_id returns True for invalid resource IDs #11658

Closed
jiasli opened this issue May 27, 2020 · 3 comments
Closed
Assignees
Labels
Azure.Mgmt.Core Azure management core bug This issue requires a change to an existing behavior in the product in order to be resolved. issue-addressed The Azure SDK team member assisting with this issue believes it to be addressed and ready to close. Mgmt This issue is related to a management-plane library.
Milestone

Comments

@jiasli
Copy link
Member

jiasli commented May 27, 2020

Is your feature request related to a problem? Please describe.
In Azure/azure-cli#10641 (comment), Azure CLI receives an incorrect --scope /subscriptions/{subscription_id}/resourceGroups/ which lacks the resource group name. It gets injected into the resource URL, thus corrupting the command.

In order to fix it, I try to use azure.mgmt.core.tools.is_valid_resource_id to detect whether a scope is valid:

def is_valid_resource_id(rid, exception_type=None):

Unfortunately, is_valid_resource_id can't return the correct value for some edge cases like this.

A code snippet:

from azure.mgmt.core.tools import parse_resource_id, is_valid_resource_id

res_ids = ['/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourceGroups/rg1',
           '/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourceGroups',
           '/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourceGroups/',
           '/subscriptions//resourceGroups/']

for res_id in res_ids:
    print(res_id)
    print(parse_resource_id(res_id))
    print(is_valid_resource_id(res_id))
    print()

Output:

/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourceGroups/rg1
{'subscription': '0b1f6471-1bf0-4dda-aec3-cb9272f09590', 'resource_group': 'rg1'}
True

/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourceGroups
{'subscription': '0b1f6471-1bf0-4dda-aec3-cb9272f09590'}
False

# The trailing slash makes is_valid_resource_id return True
/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourceGroups/
{'subscription': '0b1f6471-1bf0-4dda-aec3-cb9272f09590', 'resource_group': ''}
True

# Empty subscription ID shouldn't be allowed
/subscriptions//resourceGroups/
{'subscription': '', 'resource_group': ''}
True

Describe the solution you'd like

  1. No empty name '' should be allowed when parsing a resource ID
  2. is_valid_resource_id should return False for /subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourceGroups/
@ghost ghost added the needs-triage This is a new issue that needs to be triaged to the appropriate team. label May 27, 2020
@lmazuel lmazuel assigned changlong-liu and unassigned lmazuel May 27, 2020
@lmazuel lmazuel added Azure.Mgmt.Core Azure management core Mgmt This issue is related to a management-plane library. labels May 27, 2020
@ghost ghost removed the needs-triage This is a new issue that needs to be triaged to the appropriate team. label May 27, 2020
@lmazuel lmazuel added the bug This issue requires a change to an existing behavior in the product in order to be resolved. label May 27, 2020
@lmazuel lmazuel added this to the Backlog milestone May 27, 2020
@lmazuel
Copy link
Member

lmazuel commented May 27, 2020

Hi @jiasli
This part of the code has been written by the CLI team back in the days in msrestazure, and I ported it as-is in azure-mgmt-core. I have no knowledge of it, and also @RodgeFu told me that @changlong-liu will now take care of azure-mgmt-core.
Thanks!

changlong-liu added a commit to changlong-liu/azure-sdk-for-python that referenced this issue May 29, 2020
changlong-liu added a commit that referenced this issue Sep 1, 2020
* fix issue #11658

* use regex

* add tests

* revert print
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this issue Sep 1, 2020
…into add_redacted_text

* 'master' of https://github.com/Azure/azure-sdk-for-python:
  [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)
  Switch retry (Azure#13264)
  [ServiceBus] ServiceBusClient close spawned children (Azure#13077)
  fixing version issue by not overwriting the version with the semantic… (Azure#13411)
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this issue Sep 1, 2020
…into expose_parse_vault_id

* 'master' of https://github.com/Azure/azure-sdk-for-python: (37 commits)
  [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)
  Switch retry (Azure#13264)
  [ServiceBus] ServiceBusClient close spawned children (Azure#13077)
  fixing version issue by not overwriting the version with the semantic… (Azure#13411)
  clean up reference and tests (Azure#13412)
  Consistent returns (Azure#13245)
  [text analytics] return None for offset and length for v3.0 (Azure#13382)
  edit all authentication files and add a test (Azure#13355)
  Add managed_identity_client_id argument to DefaultAzureCredential (Azure#13218)
  [text analytics] add string-index-type support (Azure#13378)
  [text analytics] fix error response if pii entities is called from v3.0 client (Azure#13383)
  Send spec (Azure#13143)
  Anomaly Detector 3.0.0b2 release (Azure#13351)
  ...
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this issue Sep 2, 2020
…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)
  ...
rakshith91 pushed a commit to rakshith91/azure-sdk-for-python that referenced this issue Sep 4, 2020
* fix issue Azure#11658

* use regex

* add tests

* revert print
@lmazuel lmazuel assigned msyyc and unassigned changlong-liu Nov 12, 2021
@RAY-316 RAY-316 added the issue-addressed The Azure SDK team member assisting with this issue believes it to be addressed and ready to close. label Dec 15, 2021
@ghost
Copy link

ghost commented Dec 15, 2021

Hi @jiasli. Thank you for opening this issue and giving us the opportunity to assist. We believe that this has been addressed. If you feel that further discussion is needed, please add a comment with the text “/unresolve” to remove the “issue-addressed” label and continue the conversation.

@ghost
Copy link

ghost commented Dec 22, 2021

Hi @jiasli, since you haven’t asked that we “/unresolve” the issue, we’ll close this out. If you believe further discussion is needed, please add a comment “/unresolve” to reopen the issue.

@ghost ghost closed this as completed Dec 22, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2023
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Mgmt.Core Azure management core bug This issue requires a change to an existing behavior in the product in order to be resolved. issue-addressed The Azure SDK team member assisting with this issue believes it to be addressed and ready to close. Mgmt This issue is related to a management-plane library.
Projects
None yet
Development

No branches or pull requests

5 participants