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

Attempts account removal from broker first #651

Merged
merged 1 commit into from Jan 19, 2024
Merged

Conversation

rayluo
Copy link
Collaborator

@rayluo rayluo commented Jan 18, 2024

This change is inspired by this conversation.

CC: @jiasli

Copy link

@ashok672 ashok672 left a comment

Choose a reason for hiding this comment

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

what if broker sign out also throws an unexpected exception? Is this possible. I don't understand why self.forget_me would throw an error skipping execution below.

@rayluo
Copy link
Collaborator Author

rayluo commented Jan 19, 2024

what if broker sign out also throws an unexpected exception? Is this possible.

It is always possible for any code path to potentially throw an exception. And we can let those really unexpected exceptions bubble up.

I don't understand why self.forget_me would throw an error skipping execution below.

In that referenced Azure CLI's case, they persist the MSAL Python's token cache on disk, and the forget_me() operation would attempt removing some tokens thus modifying that token cache file. Again, any code path could throw exception, in this case, it can be disk I/O error.

@jiasli
Copy link
Contributor

jiasli commented Jan 19, 2024

Thanks for the attempt to solve Azure/azure-cli#20231 (comment).

However, I don't think this change makes any difference as OSError: [WinError -2146893813] is thrown when get_accounts() is called. We won't even be able to retrieve the account that can be passed to remove_account(), because the account information is also in msal_token_cache.bin.

In other word, if msal_token_cache.bin can't be accessed, we totally lose track of what accounts are in WAM.

Copy link
Contributor

@jiasli jiasli left a comment

Choose a reason for hiding this comment

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

Let's rethink about it.

@rayluo rayluo force-pushed the remove-broker-accounts-first branch from 495340f to 4a7d36a Compare January 19, 2024 20:47
@rayluo rayluo merged commit 49a9198 into dev Jan 19, 2024
12 checks passed
@rayluo rayluo deleted the remove-broker-accounts-first branch January 19, 2024 20:55
@rayluo rayluo mentioned this pull request Feb 22, 2024
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.

None yet

3 participants