-
Notifications
You must be signed in to change notification settings - Fork 4
Add Authorization for Measurement Creation #652
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
base: main
Are you sure you want to change the base?
Conversation
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.
NOICE!
- Let's fix the TODO item, please - see my comment below.
- I think we need to first ship a new spark-0k-checker version that sets the access token, before we can land & ship this. Correct?
- Can we revert chore: Remove domain redirect #647 and https://github.com/CheckerNetwork/spark-0k-checker/pull/5 now?
api/bin/spark.js
Outdated
// TODO: | ||
assert(CHECKER_TOKEN, 'CHECKER_TOKEN is required') |
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.
What is this TODO for?
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.
Good catch, I've left TODO to add adittional context why is the CHECKER_TOKEN
required.
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.
I've added a small comment 03b5110. Let me know if you think I should add more context.
Yes, that would be correct.
Indeed we could revert both. I propose that we revert only chore: Remove domain redirect #647 as https://github.com/CheckerNetwork/spark-0k-checker/pull/12 contains changes that will make sure that |
SGTM 👍🏻 |
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 👏🏻
Co-authored-by: Julian Gruber <[email protected]>
…easurement-submission-authorizaiton
…:CheckerNetwork/spark-api into add/measurement-submission-authorizaiton
This pull request introduces a new authorization mechanism for the
createMeasurement
API endpoint by requiring acheckerToken
. Additionally, it updates the test suite to validate this new functionality thoroughly. The changes enhance security and ensure unauthorized or invalid requests are properly handled.API Updates:
checkerToken
parameter to thehandler
function and updated thecreateMeasurement
function to check for valid authorization headers before measurement creation. Unauthorized requests now return a403 Unauthorized
response. (api/index.js
, [1] [2] [3]createHandler
function to include thecheckerToken
parameter when initializing thehandler
. (api/index.js
, api/index.jsL459-R472)Test Suite Enhancements:
VALID_CHECKER_TOKEN
to test constants and updated all relevant test cases to include theauthorization
header with the valid token when making POST requests to/measurements
. (api/test/test.js
, [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]403 Unauthorized
response, and no measurements are created in the database. (api/test/test.js
, api/test/test.jsR427-R466)checkerToken
are processed successfully, returning the expected responses. (api/test/test.js
, api/test/test.jsL642-R680)Related https://github.com/CheckerNetwork/spark-0k-checker/pull/12
Closes #653