feat(server): add support for multiple auth tokens via env vars

[nydragon]

Description

Add support for reading a list of newline separated tokens from a file
which path is being supplied through the following environment
variables:

• AUTH_FILE
• DELETE_FILE

Also add support for setting environment variables inside of fixtures through the optional custom_env function.

Also describe this new functionality in the readme.

Motivation and Context

Need a way to supply multiple auth token akin to AUTH_TOKEN due to agenix behaviour.

closes #338

How Has This Been Tested?

Added UTs for file content parsing. File reading has not received any UT, but has been manually tested.

Changelog Entry

Added

• Multiple authentication and deletion token parsing via a file to which AUTH_FILE and DELETION_FILE point. This file needs to be a newline separated list of strings. Whitespace is discarded.

Types of Changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (no code change)
• Refactor (refactoring production code)
• Other

Checklist:

• My code follows the code style of this project.
• I have updated the documentation accordingly.
• I have formatted the code with rustfmt.
• I checked the lints with clippy.
• I have added tests to cover my changes.
• All new and existing tests passed.

[tessus]

Thanks for this PR. LGTM (unless I missed some Rust specific optimization I am not aware of, since I am not a Rust expert).

[tessus]

Oh, could you add a test to the fixtures as well?

[nydragon]

Thanks for this PR. LGTM (unless I missed some Rust specific optimization I am not aware of, since I am not a Rust expert).

I was wondering if it would be better to .trim() the entire file or the individual lines after splitting (current behaviour).

entire file -> calls trim once instead of n of lines time
on every line -> allows the end user to be "sloppy", ie trailing whitepace, empty lines etc

[nydragon]

Oh, could you add a test to the fixtures as well?

Sure! I didn't see an option to set env vars tho... Did i miss something?

[tessus]

I was wondering if it would be better to .trim() the entire file or the individual lines after splitting (current behaviour).

One would have to run perf tests for the difference, but I doubt it is huge for a small to medium sized file with tokens.
Personally I like the current behavior better, since you took care of a few edge cases as well.

The only workflow optimization I can think of is that the file is not read and parsed every time a request is made, but only when the config is read or reloaded via refresh_rate.

If you want to try that, please go ahead.

[tessus]

Sure! I didn't see an option to set env vars tho... Did i miss something?

No, you are correct.

@orhun are you ok with adding another hook to add env vars to the fixtures?

e.g. in test.sh:

rp_env {
  export ENV=bla
}

and then we only need in the test_fixtutes.sh:

run_fixture() {
  cd "$FIXTURE_DIR/$1" || exit 1
  source "test.sh"
  rp_env
  NO_COLOR=1 CONFIG=config.toml "$PROJECT_DIR/target/debug/rustypaste" &
  ..
  ..

P.S.: Hmm, since the test.sh scripts are not run but sourced, we might have to unset the exported var in teardown, although that is run in a subshell, which means we would also have to use another hook e.g. rp_env_teardown.

[nydragon]

I was wondering if it would be better to .trim() the entire file or the individual lines after splitting (current behaviour).

One would have to run perf tests for the difference, but I doubt it is huge for a small to medium sized file with tokens. Personally I like the current behavior better, since you took care of a few edge cases as well.

The only workflow optimization I can think of is that the file is not read and parsed every time a request is made, but only when the config is read or reloaded via refresh_rate.

If you want to try that, please go ahead.

That'd make definitely sense! I was a bit surprised to see that get_tokens is called on every request. Why is that? Wouldn't it be enough to just call the function periodically according to refresh_rate?

[tessus]

That'd make definitely sense! I was a bit surprised to see that get_tokens is called on every request. Why is that? Wouldn't it be enough to just call the function periodically according to refresh_rate?

This is a good question. I think it developed this way. At the beginning there was only one token in the config file and one env var. I added the multiple tokens and delete tokens, and then it was refactored by somebody else to be used as a middleware with web grants.
In all these stages only the config was referenced (not the file read) and the ENV vars read, thus no real overhead.

[orhun]

Looks good! We can simplify it a bit :)

[orhun]

are you ok with adding another hook to add env vars to the fixtures?

Hmm, yeah, if it is not much of a hassle. Bash is annoying.

I was a bit surprised to see that get_tokens is called on every request. Why is that?

We use a library called actix_web_grants which works by adding a middleware and then you can simply protect your endpoints with an annotation.

rustypaste/src/server.rs

Line 417 in 1d5a9c6

.wrap(GrantsMiddleware::with_extractor(extract_tokens))

rustypaste/src/server.rs

Line 153 in 1d5a9c6

#[actix_web_grants::protect("TokenType::Delete", ty = TokenType, error = unauthorized_error)]

Implemented in #199

That is why extract_tokens is called on every request since there is a guard. We can maybe cache some parts to make it more performant.

Wouldn't it be enough to just call the function periodically according to refresh_rate?

That means we need to store the tokens somewhere (probably in memory) and that's not how the grants library works.

[nydragon]

Alright, thank you for the explanation, should I then leave out the improvements mentioned by tessus (#339 (comment))?

I'll also add the env handling to the fixture script.

[nydragon]

Should be all good now:

• added fixture for the file based authentication process
• added fixture for the file based deletion process
• added env var setting through the rp_env function, teardown is handled by putting run_fixture into a subshell
• applied code review requests

[nydragon]

Applied all requests

[tessus]

Great, thanks.

One last thing. Could you please update the README in the fixtures dir?

Can you please add the new function to the file format?

[nydragon]

I think I already did that: https://github.com/orhun/rustypaste/pull/339/files#diff-3374186a79a0a6316047cb823494f238bdc644489b1b709bf65c192b5c05183a

[tessus]

Thanks, I missed it. All good then! I think it is ready to be merged.

[orhun]

Perfect, thanks!