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

Add force refresh support for acquire_token_silent broker flow #737

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

Ugonnaak1
Copy link

@Ugonnaak1 Ugonnaak1 commented Aug 28, 2024

AB#2806005

Changes proposed in this request
Currently Broker flows don't allow bypassing broker cache, however force_refresh enables bypassing of msals token cache. My change involves adding the implementation that will call into pymsalruntime to renew the access token given force_refresh is set to true

Testing
Testing done using manual validation

@Ugonnaak1 Ugonnaak1 requested a review from a team as a code owner August 28, 2024 22:51
msal/application.py Outdated Show resolved Hide resolved
@bgavrilMS
Copy link
Member

@rayluo - do you have any more comments on this PR? This is blocking a PIM scenario (see https://identitydivision.visualstudio.com/Engineering/_boards/board/t/Auth%20Client%20-%20CPP/Backlog%20items/?workitem=2806005)

@rayluo
Copy link
Collaborator

rayluo commented Oct 2, 2024

@Ugonnaak1 , the PR itself looks good. I refactored just a little bit.

One more thing needs to be done.

Testing
Testing done using manual validation

You only tested it on Windows at that time. But the dev branch already received new change with Mac broker support. Please merge in the dev branch, re-test it on Windows and on Mac, and then report back. Thanks!

sample/confidential_client_sample.py Dismissed Show dismissed Hide dismissed
sample/confidential_client_sample.py Dismissed Show dismissed Hide dismissed
sample/confidential_client_sample.py Dismissed Show dismissed Hide dismissed
sample/device_flow_sample.py Dismissed Show dismissed Hide dismissed
sample/device_flow_sample.py Dismissed Show dismissed Hide dismissed
sample/interactive_sample.py Dismissed Show dismissed Hide dismissed
sample/interactive_sample.py Dismissed Show dismissed Hide dismissed
sample/username_password_sample.py Dismissed Show dismissed Hide dismissed
sample/username_password_sample.py Dismissed Show dismissed Hide dismissed
tests/test_e2e.py Dismissed Show dismissed Hide dismissed
@Ugonnaak1
Copy link
Author

@Ugonnaak1 , the PR itself looks good. I refactored just a little bit.

One more thing needs to be done.

Testing
Testing done using manual validation

You only tested it on Windows at that time. But the dev branch already received new change with Mac broker support. Please merge in the dev branch, re-test it on Windows and on Mac, and then report back. Thanks!

@rayluo I've retested on Windows and the test are passing. However, I'm unable to test on Mac. Would you be able to confirm it passes on Mac? Thanks

@rayluo
Copy link
Collaborator

rayluo commented Oct 8, 2024

I've retested on Windows and the test are passing. However, I'm unable to test on Mac. Would you be able to confirm it passes on Mac? Thanks

@fengga in your team worked on MSAL Python in the Mac broker project recently. Would you please work with @fengga to complete the Mac test and leave a comment in the PR attesting that the Mac test has been performed? After that, please ping me again for a final signoff.

@Ugonnaak1
Copy link
Author

@rayluo It appears the logic of comparing the at_to_renew with broker returned AT is not implemented on Mac and a feature request needs to be put in to have this implemented. Since this feature was initially requested just for windows, could we merge it so the card this is tracking in ADO may be closed? Thanks!

@rayluo
Copy link
Collaborator

rayluo commented Oct 25, 2024

@rayluo It appears the logic of comparing the at_to_renew with broker returned AT is not implemented on Mac and a feature request needs to be put in to have this implemented. Since this feature was initially requested just for windows, could we merge it so the card this is tracking in ADO may be closed? Thanks!

Hmm, if I remember correctly, you once mentioned (in here?) that the test on Mac did not pass. Was that message deleted? Regardless, what is the investigation outcome? How difficult to solve that? Or at least, do we know which component causes the issue?

MSAL Python is a cross-platform library so all its behaviors shall have cross-platform parity. Each merge shall not introduce disparity. If your concern is that ADO card, you can close the old card and create a new card for "at_to_renew on mac platform".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants