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

[AD] Support special characters in object names #22739

Merged
merged 5 commits into from
Jun 14, 2022
Merged

Conversation

jiasli
Copy link
Member

@jiasli jiasli commented Jun 6, 2022

Related command
az ad group show

Close #22700

Graph service

Even though Graph service recommends encoding query parameters (https://docs.microsoft.com/en-us/graph/query-parameters#encoding-query-parameters), it is not explicit on which characters should be encoded:

The values of query parameters should be percent-encoded.

# unencoded
GET https://graph.microsoft.com/v1.0/users?$filter=startswith(givenName, 'J')
# encoded
GET https://graph.microsoft.com/v1.0/users?$filter=startswith(givenName%2C+'J')

In the above example, only a subset of reserved characters defined by rfc3986 are encoded, like ,<SPACE>, but ()' are not (considered "safe"). Instead, startswith(givenName, 'J') should be encoded as

from urllib.parse import quote
print(quote("startswith(givenName, 'J')"))
# startswith%28givenName%2C%20%27J%27%29
# startswith(  givenName,     '  J'  )  <- original

Graph service SHOULD explicitly specify which characters should be encoded (https://github.com/microsoftgraph/microsoft-graph-docs-contrib/issues/2085).

CLI

GraphClient uses requests to make HTTP requests. requests uses urllib3's encoding mechanism which treats ,+ as "safe" and doesn't encode them.

https://github.com/urllib3/urllib3/blob/342aff50ff300d96a58e9be22f27fcee771ce98d/src/urllib3/util/url.py#L73-L79

UNRESERVED_CHARS = set(
    "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789._-~"
)
SUB_DELIM_CHARS = set("!$&'()*+,;=")
USERINFO_CHARS = UNRESERVED_CHARS | SUB_DELIM_CHARS | {":"}
PATH_CHARS = USERINFO_CHARS | {"@", "/"}
QUERY_CHARS = FRAGMENT_CHARS = PATH_CHARS | {"?"}

Even though Azure CLI 2.37.0 doesn't encode ,, the API still works:

> az ad group list --display-name azure-cli-test1 --debug
cli.azure.cli.core.util: Request URL: "https://graph.microsoft.com/v1.0/groups?$filter=startswith(displayName,'azure-cli-test1')"

SDK

Previously, graphrbac SDK encodes all special characters, including (),'.

https://github.com/Azure/msrest-for-python/blob/9c719385a1939ae840124838131a09002c123298/msrest/serialization.py#L699

                output = quote(str(output), safe='')
> az ad group show -g azure-cli-test+ --debug
msrest.http_logger: Request URL: 'https://graph.windows.net/54826b22-38d6-4fb2-bad9-b7b93a3e9c5a/groups?$filter=displayName%20eq%20%27azure-cli-test%2B%27&api-version=1.6'

> az ad group list --display-name azure-cli-test+ --debug
msrest.http_logger: Request URL: 'https://graph.windows.net/54826b22-38d6-4fb2-bad9-b7b93a3e9c5a/groups?$filter=startswith%28displayName%2C%27azure-cli-test%2B%27%29&api-version=1.6'

This is consistent with the Graph document, but inconsistent with Graph document's example.

Change

This PR encodes all special characters, like the old SDK does, in order to support special characters such as +/ in object names.

@ghost ghost requested a review from yonzhan June 6, 2022 11:36
@ghost ghost added the Auto-Assign Auto assign by bot label Jun 6, 2022
@ghost ghost requested a review from wangzelin007 June 6, 2022 11:36
@ghost ghost assigned jiasli Jun 6, 2022
@ghost ghost added this to the Jun 2022 (2022-07-05) milestone Jun 6, 2022
@ghost ghost added the Graph az ad label Jun 6, 2022
@ghost ghost requested review from evelyn-ys and calvinhzy June 6, 2022 11:36
@yonzhan
Copy link
Collaborator

yonzhan commented Jun 6, 2022

Graph

Comment on lines +190 to +191
Theoretically, GeneralNameReplacer should also follow this pattern, but since we haven't seen any ARM name that
is percent-encoded, we only replace percent-encoded names for MS Graph for better test performance.
Copy link
Member Author

@jiasli jiasli Jun 7, 2022

Choose a reason for hiding this comment

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

The test framework of Azure CLI and SDK never supports special characters in resource names, as GeneralNameReplacer doesn't take special characters into consideration.

I tried to change GeneralNameReplacer's logic to also replace encoded names, but it turns out to be an overdo since ARM resource names usually don't need to be encoded, so we only do it specially for MS Graph.

Copy link
Member

@jsntcy jsntcy Jun 13, 2022

Choose a reason for hiding this comment

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

Do we have any test to cover the changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

test_special_characters is exactly for that purpose.

@jiasli
Copy link
Member Author

jiasli commented Jun 8, 2022

Luckily, VCRPY can also match URLs that are percent-encoded, so we don't need to re-record all tests.

@jiasli jiasli marked this pull request as ready for review June 9, 2022 09:44
@jiasli jiasli requested review from jsntcy and kairu-ms as code owners June 9, 2022 09:44
@jiasli jiasli merged commit 2591276 into Azure:dev Jun 14, 2022
@jiasli jiasli deleted the url-encode branch June 14, 2022 03:58
@jiasli jiasli changed the title [Graph] Support special characters in object names [Role] Support special characters in object names Jun 14, 2022
@jiasli jiasli changed the title [Role] Support special characters in object names [AD] Support special characters in object names Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Assign Auto assign by bot Graph az ad
Projects
None yet
Development

Successfully merging this pull request may close these issues.

az ad group show --group breaks with group names that use + in v2.37
5 participants