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

Auth Token feature #1952

Merged
merged 2 commits into from
Oct 1, 2024
Merged

Auth Token feature #1952

merged 2 commits into from
Oct 1, 2024

Conversation

barreiro
Copy link
Collaborator

as discussed in #1922

@barreiro barreiro force-pushed the auth-tokens branch 6 times, most recently from dd6925e to 636d6ac Compare August 30, 2024 04:54
@barreiro barreiro marked this pull request as ready for review September 10, 2024 03:19
@barreiro
Copy link
Collaborator Author

This feature is now ready for review

@lampajr
Copy link
Member

lampajr commented Sep 19, 2024

Hey Luis, sorry for delay! I am planning to review this one by today/tomorrow.. I'll come back to you asap 🚀

Copy link
Member

@lampajr lampajr left a comment

Choose a reason for hiding this comment

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

Hey again,

Here a first round of review, overall looks good and looks working. I tried the use case where I try to upload a new run and it looks working using the generated API key.

curl -s --url-query "test=optaplanner-jmh-benchmarks-8x" -X POST -H "content-type: application/json" -H 'X-Horreum-API-Key: <MY_KEY>' 'http://localhost:8080/api/run/data' -d @/tmp/op-run.json

There are still a couple of comments:

  1. @barreiro could you please rebase this on top of master? those changes shouldn't affect this feature but just to be sure.
  2. Return error msg when the API Key validation failed, I tried to run the same upload command with either a fake key or a revoked one and in both cases no error is returned to the user even if the request failed.

Here an example:
If don't send any auth token, the rest will return a generic error msg:

$ curl -s --url-query "test=optaplanner-jmh-benchmarks-8x" --url-query "owner=optaplanner-team" --url-query "start=2024-07-09T09:45:38" --url-query "stop=2024-07-09T09:45:38" -X POST -H "content-type: application/json"  'http://localhost:8080/api/run/data' -d @/tmp/op-run.json

Cannot upload to test optaplanner-jmh-benchmarks-8x% 

and in the logs I see (in DEBUG)

DEBUG [io.hyp.too.hor.svc.TestServiceImpl] (executor-thread-1) Failed to retrieve test optaplanner-jmh-benchmarks-8x as this user ( = []) is not uploader for optaplanner-team and token null does not match

If I send, instead, a revoked token:

$ curl -s --url-query "test=optaplanner-jmh-benchmarks-8x" --url-query "owner=optaplanner-team" --url-query "start=2024-07-09T09:45:38" --url-query "stop=2024-07-09T09:45:38" -X POST -H "content-type: application/json" -H 'X-Horreum-API-Key: HUSR_0A1CE383_38D9_4AFB_B094_99B0D6D6BE73' 'http://localhost:8080/api/run/data' -d @/tmp/op-run.json

there is no response at all and no logs as well, I think it could be very useful to return at least an error msg, even a generic "cannot upload to test ..."

  1. In the UI, when there is no API keys set the webview still shows the table columns:
    Screenshot from 2024-09-20 11-33-54

I would suggest to show just the button when there is no API keys set similarly to what we are doing in other places - or showing a message like "No API keys set" and below the button (that's actually just a minor).

Copy link
Member

@johnaohara johnaohara left a comment

Choose a reason for hiding this comment

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

@barreiro this looks great!

For this PR, we need user docs adding

For future PR's i think we need to open issues to;

  • add global management of keys at admin level
  • remove "tokens" and "machine accounts" features

@barreiro
Copy link
Collaborator Author

@lampajr

  1. rebase this on top of master

👍

  1. Return error msg when the API Key validation failed,

This is not possible, unfortunately. The error msg in your example comes from the exception that is thown in the service logic. For authentication failure you get a simple 401 Unauthorized response that comes from quarkus. The IdentityProvider, where validation of API key happens, can authenticate the request but it can't rule out authentication through other mechanisms.

For reference, a better way to deal with this would be to curl -i or curl -w "%{http_code}" to see the response.

  1. The UI

👍

@barreiro barreiro force-pushed the auth-tokens branch 3 times, most recently from 66ec5da to bb8d174 Compare September 27, 2024 20:11
@johnaohara
Copy link
Member

@barreiro we need docs, is there a separate PR for docs?

@johnaohara johnaohara merged commit 5608049 into Hyperfoil:master Oct 1, 2024
2 checks passed
This was referenced Oct 14, 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.

3 participants