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

Feature/api keys #1084

Merged
merged 17 commits into from
Oct 22, 2024
Merged

Feature/api keys #1084

merged 17 commits into from
Oct 22, 2024

Conversation

kostas-kou
Copy link
Contributor

@kostas-kou kostas-kou commented Oct 14, 2024

Related issue(s) and PR(s)
This PR closes #1036.

Description
With this PR an endpoint for adding key hash in added in the api service.
An extra step of adding an api role in the database was necessary in order to make it work.

  • payload is a public key file as base64 encoded string i.e base64 -w0 c4gh.pub.pem
  • Validation is done to ensure that:
    • payload is base64 encoded
    • payload is a cryp4gh public file
  • Integration tests have been expanded to:
    • add public key has to the database, twice so we also check the error.
    • validate that the hash in the database is identical to the manually computed hash.

How to test
From the root of the folder run make build-all
and then for linux run:

PR_NUMBER=$(/usr/bin/date '+%F')  docker compose -f .github/integration/sda-s3-integration.yml run integration_test

while for mac run:

PR_NUMBER=$(/bin/date '+%F')  docker compose -f .github/integration/sda-s3-integration.yml run integration_test

Then set the token as envvar: export ACCESS_TOKEN=$(curl -s -k http://localhost:8080/tokens | jq -r '.[0]')
and run:

curl -H "Authorization: Bearer $ACCESS_TOKEN" -H "Content-Type: application/json" -X POST -d '{"pubkey": "'"$(base64 -w0 PATH_TO_PUBKEY_FILE)"'", "description": "this is the key description"}' http://localhost:8090/c4gh-keys/add

Confirm that the key hash is in the database by running: docker exec -it postgres psql -U postgres -d sda and then the query select * from sda.encryption_keys;

postgresql/initdb.d/04_grants.sql Show resolved Hide resolved
postgresql/migratedb.d/13.sql Show resolved Hide resolved
sda/cmd/api/api.go Outdated Show resolved Hide resolved
sda/cmd/api/api.go Outdated Show resolved Hide resolved
sda/internal/schema/schema.go Outdated Show resolved Hide resolved
sda/cmd/api/api.md Outdated Show resolved Hide resolved
sda/cmd/api/api_test.go Outdated Show resolved Hide resolved
sda/cmd/api/api.go Show resolved Hide resolved
sda/cmd/api/api.go Outdated Show resolved Hide resolved
Copy link
Contributor

@MalinAhlberg MalinAhlberg left a comment

Choose a reason for hiding this comment

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

Good work! I agree with @jbygdell that it would be very nice, and much more user friendly, if the user could just send the public key and then the api would calculate the hex.
Apart from that it looks good and works with the mentioned tests coming in #1073 .

sda/cmd/api/api.go Outdated Show resolved Hide resolved
Copy link
Contributor

@nanjiangshu nanjiangshu left a comment

Choose a reason for hiding this comment

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

Great work! Works well when I tested and I just left a few minor comments.

Apart from that, I find that when trying to insert an already existing key, it takes more than 60 seconds to return the response. I guess this is caused by repeated retrying to insert the key? Would be great if the check of the existence a key can be done at an earlier stage so that the user don't need to wait 60 seconds to get the error message.

@MalinAhlberg
Copy link
Contributor

it takes more than 60 seconds to return the response.

Great catch @nanjiangshu! I was wondering why the integrations tests were even slower than usual 😅

@jbygdell jbygdell self-assigned this Oct 21, 2024
kostas-kou and others added 14 commits October 21, 2024 14:30
- Add the structure
- Add the database functions for inserting the key and its description
in the encryption_keys table
- Add the endpoint and the function for calling the db functions
Add unit test for adding key hash in the encryptio_keys table function
- The write time out of the http server was increased for proper
response in case of duplicate
- Spit the errors to confict and server error
- Add logging
- Update function comments
jbygdell
jbygdell previously approved these changes Oct 21, 2024
@jbygdell jbygdell force-pushed the feature/api-keys branch 5 times, most recently from 24f92b3 to 440b590 Compare October 22, 2024 06:12
Copy link
Contributor

@MalinAhlberg MalinAhlberg left a comment

Choose a reason for hiding this comment

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

Great!

Copy link
Contributor

@nanjiangshu nanjiangshu left a comment

Choose a reason for hiding this comment

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

Looks good!

@jbygdell jbygdell added this pull request to the merge queue Oct 22, 2024
Merged via the queue into main with commit 426230b Oct 22, 2024
27 checks passed
@jbygdell jbygdell deleted the feature/api-keys branch October 22, 2024 10:14
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.

Extend admin API for adding key hash
4 participants