Feature/files api #540

dbampalikis · 2023-02-27T14:42:10Z

The PR creates an endpoint in the API service for retrieving information about the files a user has submitted. The user is authenticated using the JWToken and currently the way to parse the token is only done through a public key file.

Lacks integration tests

jbygdell

Maybe not perfectly correct but you get the gists

cmd/api/api.go

internal/config/config.go

codecov-commenter · 2023-02-27T15:42:39Z

Codecov Report

Merging #540 (58e99fb) into master (1bcd776) will decrease coverage by 0.38%.
The diff coverage is 34.88%.

❗ Current head 58e99fb differs from pull request most recent head 7325bae. Consider uploading reports for the commit 7325bae to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #540      +/-   ##
==========================================
- Coverage   39.38%   39.00%   -0.38%     
==========================================
  Files          13       13              
  Lines        3161     3325     +164     
==========================================
+ Hits         1245     1297      +52     
- Misses       1874     1981     +107     
- Partials       42       47       +5

Flag	Coverage Δ
unittests	`39.00% <34.88%> (-0.38%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
internal/database/db.go	`69.72% <0.00%> (-10.47%)`	⬇️
internal/config/config.go	`77.98% <4.54%> (-3.91%)`	⬇️
cmd/api/api.go	`56.02% <57.28%> (-2.32%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

cmd/api/api.go

internal/database/db.go

pontus · 2023-03-02T12:34:07Z

cmd/api/api_test.go

+	WrongTokenAlgClaims = map[string]interface{}{
+		"iss":       "Online JWT Builder",
+		"iat":       time.Now().Unix(),
+		"exp":       time.Now().Add(time.Hour * 2).Unix(),
+		"aud":       "4e9416a7-3515-447a-b848-d4ac7a57f",
+		"sub":       "[email protected]",
+		"auth_time": "1632207224",
+		"jti":       "cc847f9c-7608-4b4f-9c6f-6e734813355f",
+	}


pontus · 2023-03-02T13:19:22Z

Looks OK, I notice one can have at most one key per issuer, meaning key rotation is not possible, but guess we won't have actual problems with that for now.

pontus · 2023-03-03T15:11:22Z

I'm not sure exactly what use case this is meant to address, but I believe it would be nice to make it easier for sda-cli to support resuming uploads, in which case it would be nice to offer submission_file_size to the user as well if there is a checksum of type UPLOADED.

Or would that be out of scope?

blankdots

if i understand correctly the only way to match users to files is if we issue the token ... not sure how that will work from a services standpoint, for this reason I would prefer if having a local key for generating the token is not the default but an option as we can have a list of controlled issuers that we allow to generate the tokens and we can check the users against that.

blankdots · 2023-03-06T08:33:59Z

cmd/api/api.go


 	if Conf.API.MQ.Connection.IsClosed() {
-		statusCocde = http.StatusServiceUnavailable
+		statusCode = http.StatusServiceUnavailable


not sure i follow why we need MQ checks here,the API does not do anything with the broker.

blankdots · 2023-03-06T08:36:27Z

cmd/api/api.go

+	c.JSON(200, files)
+}
+
+// getUserFromToken parses the token, validates it against the key and returns the key


i think the token check should be split, so that it is easier to integrate with sda download: see: https://github.com/neicnordic/sda-download/blob/main/pkg/auth/auth.go#L54-L90

Also it would be good to have a list of trusted issuers: https://github.com/neicnordic/sda-download/blob/main/pkg/auth/auth.go#L327-L344

blankdots · 2023-03-06T08:39:49Z

internal/config/config.go

@@ -511,3 +519,28 @@ func CopyHeader() bool {

 	return false
 }
+
+// Function for reading the ega key in []byte
+func GetJwtKey(jwtpubkeypath string, jwtKeys map[string][]byte) error {


i think we can have /jwk endpoint and serve our key there so it creates less hassle when changing the function for checking jwt signature.

Yes, that makes sense actually. I might need to leave this function as a second option though, at least until we create an endpoint for serving the key

It's not in https://www.iana.org/assignments/well-known-uris/well-known-uris.xhtml, but .well-known/jwks.json would be a good path for that, see e.g. https://auth0.com/docs/secure/tokens/json-web-tokens/json-web-key-sets.

cmd/api/api.go

+
+	verifiedToken, err := jwt.Parse([]byte(tokenStr), jwt.WithKey(jwa.RS256, key))
+	if err != nil {
+		log.Debugf("failed to verify token as RS256 signature of token %s, %s", tokenStr, err)


cmd/api/api.go

dbampalikis requested review from jbygdell, norling and aaperis February 27, 2023 14:45

jbygdell reviewed Feb 27, 2023

View reviewed changes

dbampalikis force-pushed the feature/files-api branch from d8912fd to 6085c1d Compare February 27, 2023 15:38

pontus reviewed Feb 28, 2023

View reviewed changes

cmd/api/api.go Outdated Show resolved Hide resolved

pontus reviewed Feb 28, 2023

View reviewed changes

cmd/api/api.go Outdated Show resolved Hide resolved

blankdots reviewed Feb 28, 2023

View reviewed changes

cmd/api/api.go Outdated Show resolved Hide resolved

pontus reviewed Mar 1, 2023

View reviewed changes

internal/database/db.go Outdated Show resolved Hide resolved

dbampalikis force-pushed the feature/files-api branch from d301336 to d4661c4 Compare March 2, 2023 08:44

dbampalikis requested review from pontus, jbygdell and blankdots March 2, 2023 09:24

pontus reviewed Mar 2, 2023

View reviewed changes

pontus approved these changes Mar 2, 2023

View reviewed changes

dbampalikis marked this pull request as ready for review March 3, 2023 15:06

dbampalikis force-pushed the feature/files-api branch 3 times, most recently from cd5a83b to 5832318 Compare March 3, 2023 15:20

dbampalikis marked this pull request as draft March 3, 2023 15:22

dbampalikis marked this pull request as ready for review March 3, 2023 15:22

blankdots reviewed Mar 6, 2023

View reviewed changes

dbampalikis force-pushed the feature/files-api branch from c4d69c7 to da758e6 Compare March 6, 2023 08:53

github-advanced-security bot found potential problems Mar 6, 2023

View reviewed changes

dbampalikis force-pushed the feature/files-api branch from 32cbba3 to cce3390 Compare March 6, 2023 09:06

dbampalikis mentioned this pull request Apr 12, 2023

[api] Add authentication to API service neicnordic/sensitive-data-archive#444

Closed

jbygdell force-pushed the feature/files-api branch from 46d6a8b to 73254ab Compare April 17, 2023 10:44

dbampalikis and others added 8 commits April 17, 2023 12:49

Update config

0b7f61b

Update api in docker compose files

2ed1f83

Fix linter issues

de05fe3

Add script to create jwt in tests

9d0cacb

Add api to integration testing

858bd28

Add check for api response in integration testing

bac5c7c

Remove unused variable from api tests

691b031

Storage_test don't look for static files

33c3b46

jbygdell force-pushed the feature/files-api branch from 73254ab to df1e393 Compare April 17, 2023 10:51

jbygdell and others added 4 commits April 17, 2023 13:40

[Api integration test] start containers from within the test

e9bffbc

Update go mod and sum

8962071

Remove unused variable from getUserFromToken

1112f6c

Remove unused variable from CreateRSAToken

b3d17f4

jbygdell force-pushed the feature/files-api branch from df1e393 to b3d17f4 Compare April 17, 2023 11:40

Make script run at a proper time

e46bce5

jbygdell force-pushed the feature/files-api branch from fb5b12d to e46bce5 Compare April 17, 2023 12:14

Put public keys in a separate folder

5136c88

jbygdell force-pushed the feature/files-api branch 8 times, most recently from 5cdd6c5 to 7325bae Compare April 18, 2023 09:11

jbygdell added 2 commits April 18, 2023 11:32

Don't expose token in logs

a48bdec

Fail test if API returns bad value

58e99fb

kusalananda removed the request for review from norling August 24, 2023 07:48

jbygdell marked this pull request as draft September 21, 2023 06:40

MalinAhlberg mentioned this pull request Oct 17, 2023

Add files api neicnordic/sensitive-data-archive#360

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/files api #540

Feature/files api #540

dbampalikis commented Feb 27, 2023 •

edited

Loading

jbygdell left a comment

codecov-commenter commented Feb 27, 2023 •

edited

Loading

pontus Mar 2, 2023

pontus commented Mar 2, 2023

pontus commented Mar 3, 2023

blankdots left a comment

blankdots Mar 6, 2023

blankdots Mar 6, 2023

blankdots Mar 6, 2023

dbampalikis Mar 13, 2023

pontus Mar 17, 2023

Feature/files api #540

Are you sure you want to change the base?

Feature/files api #540

Conversation

dbampalikis commented Feb 27, 2023 • edited Loading

jbygdell left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 27, 2023 • edited Loading

Codecov Report

pontus Mar 2, 2023

Choose a reason for hiding this comment

pontus commented Mar 2, 2023

pontus commented Mar 3, 2023

blankdots left a comment

Choose a reason for hiding this comment

blankdots Mar 6, 2023

Choose a reason for hiding this comment

blankdots Mar 6, 2023

Choose a reason for hiding this comment

blankdots Mar 6, 2023

Choose a reason for hiding this comment

dbampalikis Mar 13, 2023

Choose a reason for hiding this comment

pontus Mar 17, 2023

Choose a reason for hiding this comment

dbampalikis commented Feb 27, 2023 •

edited

Loading

codecov-commenter commented Feb 27, 2023 •

edited

Loading