Make sure there's a logger present for each Credentials Request call #2223
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nkolev92
requested review from
rohit21agrawal,
PatoBeltran,
zhili1208,
mishra14,
dtivel and
jainaashish
| @@ -91,6 +91,7 @@ public SecurePluginCredentialProvider(IPluginManager pluginManager, PluginDiscov | |||
|
|
|||
| if (_isAnAuthenticationPlugin) | |||
| { | |||
| AddOrUpdateLogger(plugin.Plugin, _logger); | |||
There was a problem hiding this comment.
why is this being passed as a function param to a private function if it's a class member?
There was a problem hiding this comment.
good point, updated it.
rohit21agrawal
approved these changes
May 21, 2018
nkolev92
added a commit
that referenced
this pull request
May 22, 2018
nkolev92
added a commit
that referenced
this pull request
May 30, 2018
nkolev92
added a commit
that referenced
this pull request
May 31, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug
Fixes: NuGet/Home#6762
Regression: No
If Regression then when did it last work:
If Regression then how are we preventing it in future:
Fix
Details: The fix here is rather simple but the impact is considerable.
We don't have a logger available every time a plugin is created.
We also have different types of loggers etc.
It's near impossible to solve the logging problem on a generic level, so I'll just address this use case here.
Basically, simply by making sure that a plugin has a logger each time before it calls GetRequestCredentials.
That way the plugin is allowed to log between the time it receives the request and it sends the response.
This is how device flow auth will work for dotnet.exe
I will work on adding this extra behavior in the spec.
Testing/Validation
Tests Added: No
Reason for not adding tests: Difficult to add automated tests for this. Tested extensively manually, by simulating a device flow auth with the test plugin:
nkolev92/NuGet.Tools@0b9cbdb
Validation done: ^^
//cc @jeffkl