- Notifications
You must be signed in to change notification settings - Fork5.2k
Refresh cached credentials after PreAuthenticate fails#101053
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| NetEventSource.Info(pool.PreAuthCredentials,$"Pre-authentication credentials changed, removing Basic credential uri={preAuthCredentialUri}, username={preAuthCredential.UserName}"); | ||
| } | ||
| pool.PreAuthCredentials.Remove(preAuthCredentialUri!,BasicScheme); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
While we lock the PreAuthCredentials the logic leading to this can run in parallel, right? Do we have concurrency problem whenRemove can throw? Perhaps useTryRemove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The method callsDictionary<TKey, TValue>.Remove() internally, which does not throw if the key does not exist in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But considering parallel requests, since we drop the lock between removing the creds and adding new ones, there may be some weird behavior if a parallel request starts when in-between. So I will move the removal to the same lock scope.
src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.BasicAuth.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
wfurt left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
rzikm commentedApr 30, 2024
Test failures are known (HTTP3 tests which were addressed since the pipeline has run). |
* Support refreshing credentials in pre-auth cache* Fix minor bug in CredentialCache* Add unit test* Fix tests* Fix tests attempt 2* Merge two lock statements.* Fix build
* Support refreshing credentials in pre-auth cache* Fix minor bug in CredentialCache* Add unit test* Fix tests* Fix tests attempt 2* Merge two lock statements.* Fix build
Uh oh!
There was an error while loading.Please reload this page.
Fixes#93340.
This PR adds logic which, after Pre-Authenticated request fails with 401, checks whether the user supplied creentials are different from the cached ones, and if so, replaces the old (stale) creds with the new ones.
An implementational obstacle was removing the old creds from the preauth cache, because of insufficient API surface of CredentialCache. Therefore: