Scope to resource #785
Scope to resource #785
Conversation
| trailer = ( # https://learn.microsoft.com/en-us/entra/identity-platform/scopes-oidc#trailing-slash-and-default | ||
| "/" if u.path.startswith("//") else "") | ||
| return "{}://{}{}".format(u.scheme, u.netloc, trailer) |
There was a problem hiding this comment.
If resource is provided to Azure CLI, Azure CLI just adds a trailing /.default to the resource:
scopes = resource_to_scopes(resource) scope = resource + '/.default'So this scope-to-resource conversion is still not exactly the reverse of Azure CLI's process.
There was a problem hiding this comment.
There is an edge case where the resource contains more than 2 trailing slashes.
Imagine resource is https://myresource.com//, CLI will convert it to scope as https://myresource.com///.default. The converted-back resource will be https://myresource.com/, losing the last slash.
There was a problem hiding this comment.
This conversion logic will remove the path part.
As mentioned in Azure/azure-cli#18312, valid identifier URI can also be
https://{verifiedCustomerDomain}/{string}https://{string}.{verifiedCustomerDomain}/{string}
The trailing /{string} will be removed.
There was a problem hiding this comment.
...
So this scope-to-resource conversion is still not exactly the reverse of Azure CLI's process.
Not that the current msal.cloudshell._scope_to_resource() conversion algorithm is perfect, but logically speaking, the goal is not reversing Azure CLI's process, but to match Cloud Shell's quirk.
Imagine resource is
https://myresource.com//...
Theoretically, yes, but we never saw that before. And, most importantly, the "Cloud Shell's quirk" above contains several resources with one trailing slash, but never a double (or more) trailing slash.
https://myresource.com/mypathwill also be converted tohttps://myresource.com.
That is arguably an expected behavior. Again, we have never encountered a Cloud Shell resource containing path, except the two known outliners which have already been handled properly.
I will suggest we focus on this test case and verify its solving the "GUID/scope" use case.
There was a problem hiding this comment.
There seem to be a lot of special cases here. Will there be other customers for this code path?
There was a problem hiding this comment.
No other customer uses this code path. It is for Cloud Shell only, and not for any other managed identity.
rayluo
force-pushed
the
scope-to-resource
branch
from
83533ad to
f183da5
Compare
rayluo
force-pushed
the
scope-to-resource
branch
from
f183da5 to
8891464
Compare
rayluo
force-pushed
the
scope-to-resource
branch
from
8891464 to
9a320f2
Compare
rayluo
force-pushed
the
scope-to-resource
branch
from
9a320f2 to
388b86d
Compare
| "https://pas.windows.net/CheckMyAccess/Linux/.default", # Special case | ||
| "https://double-slash.com//scope": "https://double-slash.com/", | ||
| "https://single-slash.com/scope": "https://single-slash.com", | ||
| "guid/some/scope": "guid", |
There was a problem hiding this comment.
Is guid really the answer here? I think you should only strip away .\default
There was a problem hiding this comment.
Semantically, since the Cloud Shell's token endpoint is expecting a resource, so we consider the resource of a potential "GUID/path/subpath" input to be only the leading part i.e. the "GUID".
Ideally, Cloud Shell will support scope directly in the future and then we will be able to remove all these workarounds.
| @@ -32,8 +32,12 @@ def _scope_to_resource(scope): # This is an experimental reasonable-effort appr | |||
| if scope.startswith(a): | |||
| return a | |||
| u = urlparse(scope) | |||
| if not u.scheme and not u.netloc: # Typically the "GUID/scope" case | |||
There was a problem hiding this comment.
I don't agree with this logic. It can potentially strip important scope information and be seen as a security issue.
Since this is managed identity, we only deal with app scopes and there is 1:1 mapping between app scope and resource. i.e. scope = resource + "./default"
As such this logic should take it into account - i.e. strip "./default" if it exists. It should not attempt to remove anything else.
There was a problem hiding this comment.
There was a problem hiding this comment.
We discussed offline. The code path is for Azure CLI running inside Cloud Shell to get a token for the signed-in user. It is never about a managed machine. (That is also why it is misleading to expose Cloud Shell token acquisition as part of the managed identity.)
Currently, Cloud Shell's token endpoint accepts a resource parameter which shall not contain any scope; Cloud Shell also has special case that accepts "https://pas.windows.net/CheckMyAccess/Linux/.default" as-is. That .Net logic will not work here.
|
Per the agreement in a meeting today, this PR is now shipped as MSAL Python 1.31.2b1 to meet a release schedule. Another round of discussion with broader audience will be scheduled in the near future; further changes can be made accordingly. |
This PR fixes the #784
As an implementation detail, this PR does not find and remove a magic string
/.default. Instead, it still attempts to semantically strip the "path(s)" and keep the "domain" or "guid". Regardless of such detail, please focus on reviewing the test cases.CC @jiasli : Can you test from this PR? Please give me a reply and then I will merge it and ship it.