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.

Sign up for GitHub

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

Jump to bottom

Ingest adds keyhash to files db table #1073

Closed
wants to merge 7 commits into from
Closed

Ingest adds keyhash to files db table #1073

wants to merge 7 commits into from

Conversation

MalinAhlberg
Copy link
Contributor

@MalinAhlberg MalinAhlberg commented Oct 4, 2024 •
edited
Loading

Related issue(s) and PR(s)
This PR closes #893.
It will be undrafted when #1036 is closed.

Commit tmp: temporary fixes to api/hash will not be included in the final version of this PR.

Description

  • ingest hex encodes the user's public key and adds it to the files info in the sda.files table
  • if the keyhash is not in the sda.encryption_keys, an error is thrown and ingest stops
  • code test and integration test are added

How to test
Add the hashed public key to sda.encryption_keys and then try to ingest a file. See the integration test for details.

@MalinAhlberg MalinAhlberg force-pushed the feature/ingest-add-keyhash branch 5 times, most recently from ecec008 to ceecb05 Compare October 9, 2024 09:05
@kostas-kou kostas-kou mentioned this pull request Oct 14, 2024
Merged
@jbygdell
Copy link
Collaborator

This PR should not have to be dependent on #1036

@MalinAhlberg
Copy link
Contributor Author

This PR should not have to be dependent on #1036

It's the tests (eg here). It could have been implemented without this dependecy, but I thought it was nice to test the intended workflow.

@MalinAhlberg MalinAhlberg force-pushed the feature/ingest-add-keyhash branch 6 times, most recently from ca919ba to 910a2db Compare October 21, 2024 12:00
@MalinAhlberg MalinAhlberg force-pushed the feature/ingest-add-keyhash branch from 910a2db to b6b5fcd Compare October 21, 2024 13:06
@MalinAhlberg MalinAhlberg marked this pull request as ready for review October 21, 2024 13:18
@jbygdell jbygdell force-pushed the feature/api-keys branch 5 times, most recently from 440b590 to b488b73 Compare October 22, 2024 06:19
Base automatically changed from feature/api-keys to main October 22, 2024 10:14
@MalinAhlberg MalinAhlberg requested a review from a team October 22, 2024 11:27
jbygdell
jbygdell requested changes Oct 22, 2024
View reviewed changes
sda/cmd/ingest/ingest.go Show resolved Hide resolved
sda/internal/database/db_functions.go Outdated Show resolved Hide resolved
sda/internal/database/db_functions_test.go Outdated Show resolved Hide resolved
sda/internal/database/db_functions.go Outdated Show resolved Hide resolved
sda/internal/database/db_functions_test.go Show resolved Hide resolved
postgresql/initdb.d/04_grants.sql Outdated Show resolved Hide resolved
.github/integration/tests/sda/11_api-getfiles_test.sh Outdated Show resolved Hide resolved
.github/integration/tests/sda/20_ingest-verify_test.sh Outdated Show resolved Hide resolved
@MalinAhlberg MalinAhlberg self-assigned this Oct 23, 2024
@MalinAhlberg MalinAhlberg force-pushed the feature/ingest-add-keyhash branch from 85f4ef9 to 0f68dbe Compare October 24, 2024 06:55
@MalinAhlberg MalinAhlberg requested a review from jbygdell October 24, 2024 08:06
@MalinAhlberg MalinAhlberg requested a review from a team October 24, 2024 08:07
kostas-kou
kostas-kou previously approved these changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@kostas-kou kostas-kou left a comment

Choose a reason for hiding this comment

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

Very good job. Just an extremely minor thing, in the PR's description I would change the part saying that it prints a warning if the keyhash does not exist in the sda.encryption_keys table because in reality it will through an error. I know that maybe is too much but my eye caught it :)

@MalinAhlberg
Copy link
Contributor Author

MalinAhlberg commented Oct 25, 2024 •
edited
Loading

Very good job. Just an extremely minor thing, in the PR's description I would change the part saying that it prints a warning if the keyhash does not exist in the sda.encryption_keys table because in reality it will through an error. I know that maybe is too much but my eye caught it :)

Thanks @kostas-kou! The description was indeed outdated, very good that you noticed! It's updated now.

nanjiangshu
nanjiangshu reviewed Oct 27, 2024
View reviewed changes
.github/integration/tests/sda/10_upload_test.sh Outdated
exit 1
fi

apt-get -o DPkg::Lock::Timeout=60 update >/dev/null
Copy link
Contributor

Choose a reason for hiding this comment

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

It is inefficient and waste of time to (potentially) run apt-get update multiple times. It can be refactored to run apt-get update only once.

nanjiangshu
nanjiangshu reviewed Oct 27, 2024
View reviewed changes
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. I added a few comments in the integrations test shell scripts and some questions regarding error handling in the go code.

@MalinAhlberg MalinAhlberg force-pushed the feature/ingest-add-keyhash branch from 0f68dbe to 6f8ebfa Compare November 4, 2024 09:50
@jbygdell
Copy link
Collaborator

jbygdell commented Nov 4, 2024

Rebase on top of Main

@MalinAhlberg MalinAhlberg force-pushed the feature/ingest-add-keyhash branch 2 times, most recently from 8ef959e to 73194a3 Compare November 5, 2024 10:07
@MalinAhlberg MalinAhlberg force-pushed the feature/ingest-add-keyhash branch from 73194a3 to 1fec28c Compare November 11, 2024 08:47
@MalinAhlberg MalinAhlberg mentioned this pull request Nov 12, 2024
Merged
@MalinAhlberg
Copy link
Contributor Author

Closed in favor of #1126 .

pahatz
pahatz reviewed Nov 12, 2024
View reviewed changes
sda/cmd/ingest/ingest.go
@@ -388,6 +390,20 @@ func main() {
continue mainWorkLoop
}

// Set the file's hex encoded public key
log.Debugln("Compute and set key hash")
Copy link
Contributor

Choose a reason for hiding this comment

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

Will these get ignored when deployed? I don't remember the loglevel we set. Just wondering if they add any value or if they will flood our logs.

pahatz
pahatz approved these changes Nov 12, 2024
View reviewed changes
Copy link
Contributor

@pahatz pahatz left a comment

Choose a reason for hiding this comment

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

Looks great overall!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pahatz pahatz pahatz approved these changes

@nanjiangshu nanjiangshu Awaiting requested review from nanjiangshu

@jbygdell jbygdell Awaiting requested review from jbygdell

@kostas-kou kostas-kou Awaiting requested review from kostas-kou

Assignees

@MalinAhlberg MalinAhlberg

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ingest] Add key hash to database table
5 participants
@MalinAhlberg @jbygdell @nanjiangshu @pahatz @kostas-kou