-
Notifications
You must be signed in to change notification settings - Fork 435
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
profiler: add API key check at initialization of Profiler object #718
Conversation
Validation only triggers when the user specifies a key using WithAPIKey. No other actions are taken, no error is returned, and the specified key will be used.
Moved the API key check to profiler/profiler.go. An API key with an invalid format will now cause newProfiler to throw an error.
My mistake, didn't know we had gofmt as part of our ci.
Refactored the error message as well, which was too specific; hopefully users onboarding into a configuration where an API key is necessary either don't exist or can refer to documentation.
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.
Thanks for the work. Left a few pointers.
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.
Since this is an open-source project, please provide a proper description of the work done, the motivations, etc. in the description of the PR. An internal ticket number has very little meaning to an external contributor.
Added a preliminary attempt at sanitizing API keys. This supposes that API keys are secret, and even invalid API keys may contain a valid API key and must be de-identified a little bit before display. None of this is relevant if we don't care... Address a few other concerns from #718 review
Instead, just don't print the API key.
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.
This is looking good now! Can we please also have tests for isAPIKeyValid
? Table-tests is what you want here, it will make it easy. See https://github.com/golang/go/wiki/TableDrivenTests
We should also test that the error is returned, as it should, when trying to create a profiler (newProfiler
) with a bad key.
profiler/options_test.go
Outdated
@@ -37,18 +37,20 @@ func TestOptions(t *testing.T) { | |||
}) | |||
|
|||
t.Run("WithAPIKey", func(t *testing.T) { | |||
var validKey = "12345678901234567890123456789012" |
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.
var validKey = "12345678901234567890123456789012" | |
validKey := "12345678901234567890123456789012" |
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.
Also, maybe we can have just one global constant testAPIKey
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.
Great suggestions.
Writing the tests revealed an issue with non-ASCII glyphs returning true by unicode.IsLower(), so this already paid off.
Also added a simple positive and negative check for Profiler instantiation.
Erroneous passes were possible with inputs from the extended ASCII space (and beyond), when those glyphs were characterized (ha) as lowercase by unicode.IsLower. This patch explicitly filters for the ASCII range.
Just a few tests to cover some obvious edge-cases. There may be many more.
Creating a profiler should fail if the API key validity check fails, and it should succeed if it succeeds (equivalently, if the API key isn't needed). Added simple tests for these conditions.
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.
Fantastic 👍
As per @gbbr's suggestion Co-authored-by: Gabriel Aszalos <gabriel.aszalos@gmail.com>
Thanks! |
Woohoo! Many thanks for the help and guidance on this one. |
This change adds a new check which verifies the sanity of the user provided API key and returns an error if the key is invalid.
ref: PROF-1571