Add ENV VAR to allow disabling Rocket authentication #36

glyph · 2021-11-24T09:24:40Z

glyph commented

2021-11-24 09:24:40 +00:00

Checks for the DISABLE_ROCKET_AUTH env var in the request guard
- If false or unset, enable auth as usual
- If true, pass along the request without auth logic
Adds a disable_auth() helper function to set the env var for testing purposes
Documents the new env var
Bumps the crate version

Would love to hear you input on this approach. I think you have a lot more experience with web auth than I do. Are we missing anything here, or is there room for improvement?

As mentioned on cabal, having to call disable_auth() in each individual test function is quite clunky. At least it gives us a simple way to also test routes with authentication enabled.

- Checks for the `DISABLE_ROCKET_AUTH` env var in the request guard - If `false` or unset, enable auth as usual - If `true`, pass along the request without auth logic - Adds a `disable_auth()` helper function to set the env var for testing purposes - Documents the new env var - Bumps the crate version @notplants Would love to hear you input on this approach. I think you have a lot more experience with web auth than I do. Are we missing anything here, or is there room for improvement? As mentioned on cabal, having to call `disable_auth()` in each individual test function is quite clunky. At least it gives us a simple way to also test routes with authentication enabled.

glyph added 4 commits 2021-11-24 09:24:41 +00:00

bb34bdd653 add auth flag check

bc28a84ad4 disable auth for tests

3325706dcb document disable auth flag

b59e62f920 bump crate version

glyph requested review from notplants 2021-11-24 09:24:48 +00:00

glyph added the

enhancement

label 2021-11-24 09:24:59 +00:00

notplants commented

2021-11-24 10:59:25 +00:00

@glyph nice, the code in RequestGuard looks exactly how I imagined it would with the if/else.

A couple possible changes to consider:

use Rocket configs instead of Env variables directly. And where you are currently reading the Env variable in the RequestGuard, read the Rocket config value of DISABLE_ROCKET_AUTH instead. Rocket config can be set using Env variables, .toml file or programitically, which is the main avantage over using a env variable directly: https://rocket.rs/v0.5-rc/guide/configuration/#configuration
then instead of disable_auth function, write a utility function called init_test_rocket which is a wrapper around init_rocket, which takes an argument disable_auth and sets DISABLE_ROCKET_AUTH based on this argument. Then in all the tests, use init_test_rocket instead of using init_rocket directly, and pass the argument as needed. In the future if there are particular configurations every test needs, we could also put them in here.

Both of these are stylistic and not major changes. (1) might be more flexible. (2) might be more intuitive/streamlined to me (you say whether rocket should use authentication or not at the same time as you are instantiating it, instead of as a separate function call before).

I guess you could also do something organized like (2) without (1) is an option too.

Maybe there are other approaches as well, but this is what came to mind.

@glyph nice, the code in RequestGuard looks exactly how I imagined it would with the if/else. A couple possible changes to consider: 1. use Rocket configs instead of Env variables directly. And where you are currently reading the Env variable in the RequestGuard, read the Rocket config value of DISABLE_ROCKET_AUTH instead. Rocket config can be set using Env variables, .toml file or programitically, which is the main avantage over using a env variable directly: https://rocket.rs/v0.5-rc/guide/configuration/#configuration 2. then instead of disable_auth function, write a utility function called init_test_rocket which is a wrapper around init_rocket, which takes an argument disable_auth and sets DISABLE_ROCKET_AUTH based on this argument. Then in all the tests, use init_test_rocket instead of using init_rocket directly, and pass the argument as needed. In the future if there are particular configurations every test needs, we could also put them in here. Both of these are stylistic and not major changes. (1) might be more flexible. (2) might be more intuitive/streamlined to me (you say whether rocket should use authentication or not at the same time as you are instantiating it, instead of as a separate function call before). I guess you could also do something organized like (2) without (1) is an option too. Maybe there are other approaches as well, but this is what came to mind.

👍 1

notplants commented

2021-11-24 11:09:41 +00:00

hmm I'm now also wondering how this DISABLE_ROCKET_AUTH config, and our general PeachCloud config.yml should interact. Perhaps we should have One Config.yml To Rule Them All.

I think we were discussing this before at one point,
but maybe there is a way to use Figment for all of our configuration,
so any config values can be set programatically, by env var or by the .yml/.toml.

But maybe that should be a separate PR, if we go that direction at all, because it would be a big refactor to how we get the other configuration values.

hmm I'm now also wondering how this DISABLE_ROCKET_AUTH config, and our general PeachCloud config.yml should interact. Perhaps we should have One Config.yml To Rule Them All. I think we were discussing this before at one point, but maybe there is a way to use Figment for all of our configuration, so any config values can be set programatically, by env var or by the .yml/.toml. But maybe that should be a separate PR, if we go that direction at all, because it would be a big refactor to how we get the other configuration values.

glyph added 5 commits 2021-11-25 09:33:35 +00:00

c2b785f54b remove unnecessary websocket dependency

35ff408365 use rocket config to (en|dis)able auth

5c4ef4a529 update auth docs

d760f9f92c add init_rocket test wrapper

adc1a5bd77 update lockfile

glyph commented

2021-11-25 09:44:26 +00:00

@notplants

Thanks for the suggested changes! That was perfect feedback.

Auth is now defined in Rocket.toml: disabled by default when running in development mode, enabled by default when running in production mode. Those values are overruled by the ROCKET_DISABLE_AUTH env var (if set). I learned that the desired env var needs to be prefixed with ROCKET_ to be seamlessly included in the Rocket Config object.

I have also implemented your suggested init_test_rocket(disable_auth) wrapper. It merges the given disable_auth value into the Rocket Config (essentially replacing the default value). I think this gives us great amount of control.

I've added documentation of this to the README.

One other small change introduced since your last review: I removed the websocket dependency (we had previously removed the code but not the dep).

hmm I'm now also wondering how this DISABLE_ROCKET_AUTH config, and our general PeachCloud config.yml should interact. Perhaps we should have One Config.yml To Rule Them All.

Good question. I think we might be able to have one file and point to it with ROCKET_CONFIG. I'd prefer to discuss this in a dedicated issue (I'll open one now).

@notplants Thanks for the suggested changes! That was perfect feedback. Auth is now defined in `Rocket.toml`: disabled by default when running in `development` mode, enabled by default when running in `production` mode. Those values are overruled by the `ROCKET_DISABLE_AUTH` env var (if set). I learned that the desired env var needs to be prefixed with `ROCKET_` to be seamlessly included in the Rocket `Config` object. I have also implemented your suggested `init_test_rocket(disable_auth)` wrapper. It merges the given `disable_auth` value into the Rocket `Config` (essentially replacing the default value). I think this gives us great amount of control. I've added documentation of this to the README. One other small change introduced since your last review: I removed the `websocket` dependency (we had previously removed the code but not the dep). ----- > hmm I'm now also wondering how this DISABLE_ROCKET_AUTH config, and our general PeachCloud config.yml should interact. Perhaps we should have One Config.yml To Rule Them All. Good question. I think we might be able to have one file and point to it with `ROCKET_CONFIG`. I'd prefer to discuss this in a dedicated issue (I'll open one now).

glyph referenced this pull request

2021-11-25 09:49:02 +00:00

Define configuration architecture #37

notplants commented

2021-11-25 10:07:39 +00:00

Nice looks good. Cool bonus to have default development and production settings too. And we can continue the conversation about whether to also use figment for other peach configurations in the other issue.

👍 1

glyph merged commit c8d0a2ddf6 into main

2021-11-25 10:10:55 +00:00

glyph referenced this issue from a commit

2021-11-25 10:10:55 +00:00

Merge pull request 'Add ENV VAR to allow disabling Rocket authentication' (#36) from disable_auth into main

glyph deleted branch disable_auth

2021-11-25 10:10:55 +00:00

glyph referenced this pull request

2021-11-25 12:07:33 +00:00

Add env var to selectively disable Rocket auth #25

Sign in to join this conversation.