search

orhun / rustypaste

A minimal file upload/pastebin service.
https://blog.orhun.dev/blazingly-fast-file-sharing
MIT License
760 stars 47 forks source link

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

Closed nydragon closed 3 weeks ago

nydragon commented 3 weeks ago

Description

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

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

Types of Changes

Checklist:

tessus commented 3 weeks ago

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

nydragon commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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.

https://github.com/orhun/rustypaste/blob/1d5a9c60044ab0ece53868eccbe7cf011d0c5633/src/server.rs#L417

https://github.com/orhun/rustypaste/blob/1d5a9c60044ab0ece53868eccbe7cf011d0c5633/src/server.rs#L153

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 commented 3 weeks ago

Alright, thank you for the explanation, should I then leave out the improvements mentioned by tessus (https://github.com/orhun/rustypaste/pull/339#issuecomment-2304652210)?

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

nydragon commented 3 weeks ago

Should be all good now:

nydragon commented 3 weeks ago

Applied all requests

nydragon commented 3 weeks ago

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

tessus commented 3 weeks ago

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