feat: allow providing session token in config

[satyamkondle]

Type:Feature
Scope:aws-s3-storage

The following has been addressed in the PR:

Ability to provide session token when connecting to private s3 bucket.

• There is a related issue?
  No.
• Unit or Functional tests are not included in the PR
  No, looked at the existing unit tests. This is only adding a value to the config and I don't see any tests checking each config value. Please advice if I need to add tests.

Description:
Sometimes session tokens are required to be able to connect. Adding the ability to add session token to config.

[juanpicado]

LGTM thanks @satyamkondle

[juanpicado]

could you attach some AWS doc info regarding the session you are adding?

[armingjazi]

@satyamkondle regarding the tests, I think you should be fine, since S3 is mocked in the actual test. If you really want to add something though, you could do a bit of mocking check with something like this:

// s3PackageManagerMockedS3.test.ts // this already exists in test folder
import S3 from 'aws-sdk';

it('passes config to S3', () => {
    const expectedConfig = { sessionToken: 'sometoken' };
    expect(S3).toHaveBeenCalledWith(expectedConfig);
});

where config would be what you expect ( e.g with session token or without)

[armingjazi]

quick question here: aren't sessionTokens temporary? are there any Docs on their TTL? if they are expired say after one day, does not make sense to put them in config no?

[satyamkondle]

Right, sessionTokens are temporary. I think it is okay to have them in the config and users can provide an environment variable instead of hard coding and changing that again, right?

[armingjazi]

well depends on the TTL, would be nice to know in what kind of circumstance they would expire. If it happens too frequently, it would not be a good choice, it would mean, the devOps people have to keep manually update it

[satyamkondle]

In our use case, we are not using verdaccio as a long running service. We just start it, connect to it and download packages during build processes and exit. During the build process we use aws roles so this is not a problem.

However, when we want to push packages from local machines, we only have temporary credentials (token) that expire every day. So, for us to publish packages to S3 using verdaccio from local machines, we need this (unless I am missing something). I believe there will be people who would run into the same. This is not for DevOps, this is for developers essentially.

[armingjazi]

oh interesting use case. We went a different way, having verdaccio as a running service on an aws instance. In any case, it is maybe worth mentioning that in the Docs? @juanpicado I wonder if there is specific Docs, on best practices when it comes to verdaccio (as long running service, or one-off Dev as mentioned).

[juanpicado]

First time I heard that his plugin is being used for merely development purposes, which is cool :) .

As @tetrohed suggests, I'd update the readme, unfortunately, we do not have docs in our website regarding specific plugins, the main reason is (at least me) don't have much domain of AWS itself so I guess users will come to read the README (it's what I'd do).

Perhaps the future website might have more info about each plugin (https://github.com/verdaccio/website_2.0) like GatsbyJS does for instance, but not the case at this moment.

@stackdumper feel free to update the README (add a new section) and tell more about your use case and the new property you are adding, I guess others will find your experience really interesting.

[satyamkondle]

@satyamkondle regarding the tests, I think you should be fine, since S3 is mocked in the actual test. If you really want to add something though, you could do a bit of mocking check with something like this:

// s3PackageManagerMockedS3.test.ts // this already exists in test folder
import S3 from 'aws-sdk';

it('passes config to S3', () => {
    const expectedConfig = { sessionToken: 'sometoken' };
    expect(S3).toHaveBeenCalledWith(expectedConfig);
});

where config would be what you expect ( e.g with session token or without)

Sounds good. I do want to check the environment variable following @tetrohed comment. Will update. Thank you!

[satyamkondle]

@tetrohed Added a test. Can you please take a relook?

[satyamkondle]

@juanpicado : Thank you and here is the aws doc about temporary credentials: https://docs.aws.amazon.com/IAM/latest/UserGuide/id_credentials_temp_use-resources.html

[codecov]

Codecov Report

Merging #362 into 9.x will increase coverage by 9.81%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##              9.x     #362      +/-   ##
==========================================
+ Coverage   68.16%   77.97%   +9.81%     
==========================================
  Files          29        6      -23     
  Lines        1401      386    -1015     
  Branches      203       61     -142     
==========================================
- Hits          955      301     -654     
+ Misses        406       47     -359     
+ Partials       40       38       -2     

Flag	Coverage Δ
#core	86.36% <ø> (ø)
#plugins	76.90% <ø> (+10.44%)	⬆️

Impacted Files	Coverage Δ
plugins/local-storage/src/local-database.ts	71.25% <0.00%> (-14.38%)	⬇️
plugins/local-storage/src/local-fs.ts	80.98% <0.00%> (-6.75%)	⬇️
core/file-locking/src/readFile.ts	82.35% <0.00%> (-5.89%)	⬇️
plugins/google-cloud/src/storage-helper.ts
plugins/memory/src/local-memory.ts
core/streams/src/index.ts
plugins/memory/src/memory-handler.ts
tools/testing-utilities/src/package-generator.ts
core/file-locking/src/utils.ts
plugins/audit/src/audit.ts
... and 14 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d289a2...55f1b29. Read the comment docs.

[juanpicado]

LGTM thanks ! Please, would be great a new PR updating the README adding a use case for this feature.