ManagedIdentityClient(..., client_capabilities=["cp1"]).acquire_token_for_client(..., claims_challenge="...") #791
Conversation
rayluo
force-pushed
the
at-to-refresh
branch
from
eca0556 to
9b50e14
Compare
rayluo
force-pushed
the
at-to-refresh
branch
3 times, most recently
from
2e6dc04 to
f05ae3c
Compare
rayluo
force-pushed
the
at-to-refresh
branch
2 times, most recently
from
92ae1e3 to
05b1424
Compare
rayluo
changed the title
| @@ -402,6 +432,8 @@ def _obtain_token(http_client, managed_identity, resource): | |||
| os.environ["IDENTITY_HEADER"], | |||
| os.environ["IDENTITY_SERVER_THUMBPRINT"], | |||
| resource, | |||
| access_token_sha256_to_refresh=access_token_sha256_to_refresh, | |||
There was a problem hiding this comment.
TL;DR: This is not really a problem here, but still thanks for pointing this out.
Long explanation. There are (inevitably?) several internal layers of helpers. Currently, most of the helpers use a name "access_token_sha256_to_refresh", which "access" prefix was influenced by MSAL C++'s official parameter name "AccessTokenToRefresh". But we did use the right name "token_sha256_to_refresh" on the wire when sending out the request. You can confirm that by both the implementation inside _obtain_token_on_service_fabric() and by the test cases.
| @@ -563,7 +598,12 @@ def _obtain_token_on_service_fabric( | |||
| logger.debug("Obtaining token via managed identity on Azure Service Fabric") | |||
| resp = http_client.get( | |||
| endpoint, | |||
| params={"api-version": "2019-07-01-preview", "resource": resource}, | |||
| params={k: v for k, v in { | |||
| "api-version": "2019-07-01-preview", | |||
There was a problem hiding this comment.
| "api-version": "2019-07-01-preview", | |
| "api-version": "2019-07-01-preview", |
We just need a final confirmation on this one @rayluo. App Service is setting a new api-version, we want a sign off from SF
There was a problem hiding this comment.
What kind of sign-off do we need? A confirmation written in email? Or as a PR? (This is my genuine question, because I want to be more effective on this kind of things.)
In this case, I chatted with SF via Teams channel. I can add you into that group chat upon request.
There was a problem hiding this comment.
I do it this way, so we have tracking items closed and partner's agreement captured
There was a problem hiding this comment.
I do it this way, so we have tracking items closed and partner's agreement captured
That work item is now in Done status, but where can we find the partner's agreement captured? The api version topic seems not mentioned, neither in that work item, nor in the spec that work item referenced.
There was a problem hiding this comment.
@gladjohn , now that we have that final confirmation, can we resolve this and re-review this PR?
| 'api-version': '2019-07-01-preview', | ||
| 'resource': 'R', | ||
| 'token_sha256_to_refresh': hashlib.sha256(b"AT").hexdigest(), | ||
| }, |
There was a problem hiding this comment.
do we need a test of claims + forceRefresh?
There was a problem hiding this comment.
Not currently applicable, as MSAL Python does not support forceRefresh. We may revisit this when the circumstances changes in the future.
| 'token_sha256_to_refresh': hashlib.sha256(b"AT").hexdigest(), | ||
| }, | ||
| headers={'Secret': 'foo'}, | ||
| ) |
There was a problem hiding this comment.
can we also add a test case specifically injecting a mismatched hash to verify how the code handles that scenario?
There was a problem hiding this comment.
Not applicable, unless we would also want to implement CCA.AcquireTokenForClient(...).WithAccessTokenSha256ToRefresh(...) in non-dotnet MSALs.
|
|
||
| def test_happy_path(self): | ||
| self._test_happy_path(self.app) | ||
| def test_happy_path_with_client_capabilities_should_relay_capabilities(self): |
There was a problem hiding this comment.
can you also pass an empty list client_capabilities=[], and confirm that xms_cc is omitted? did not see one.
There was a problem hiding this comment.
Fair point. Added it into the next test case.
This pull request adds theaccess_token_sha256_to_refreshparameter to theacquire_token_for_clientmethod in themsal.ConfidentialClientApplicationclass.This PR meets feature requirements and acceptance criteria.This PR supports
ManagedIdentityClient(..., client_capabilities=["cp1"]).acquire_token_for_client(..., claims_challenge="...").Currently, the new behavior will be available for Service Fabric. Other Managed Identity v1 providers may pick up those new behaviors soon. After that happens, we will enable this behavior for more Managed Identity code paths.