Notify api key not set hints #27515
Merged
Notify api key not set hints #27515
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
need to refresh this ns after changing the middleware since these add a bit more helpful doc strings. But capture previous function value so more annoying at repl
Previously when the MB_API_KEY was unset: ``` ❯ http POST "localhost:3000/api/notify/db/1?scan=schema" HTTP/1.1 403 Forbidden <other headers removed> Forbidden ``` And now: ``` ❯ http POST "localhost:3000/api/notify/db/1?scan=schema" HTTP/1.1 403 Forbidden <other headers removed> MB_API_KEY is not set. See https://www.metabase.com/docs/latest/configuring-metabase/environment-variables#mb_api_key for details ``` An annoying thing in the tests. We set `"-Dmb.api.key=test-api-key"` in the `:test` alias for CI, but not in regular dev mode. So annoyingly we either have tests that would fail in the repl in regular dev, or use `mt/with-temporary-setting-values [api-key "test-api-key"]` when writing tests that only matter for local stuff. c'est la vie
dpsutton
added
the
backport
|
No failed tests 🎉 |
calherries
reviewed
Jan 5, 2023
|
|
||
| (def api-key-not-set-message | ||
| "Message sent if an endpoint protected by this middleware is hit but MB_API_KEY is not set." | ||
| (format "MB_API_KEY is not set. See %s for details" mb-api-key-doc-url)) |
There was a problem hiding this comment.
I think this needs i18n. It should use tru instead of format and be a defn-, or in the body of enforce-api-key.
See other error messages as examples:
metabase/src/metabase/api/common.clj
Line 163 in 13e9e8e
There was a problem hiding this comment.
shouldn't it be deferred-tru?
There was a problem hiding this comment.
You could use deferred-tru, yes. The examples in api/common.clj use both tru and deferred-tru. So here you could use a def with deferred-tru and call str on it later, instead of defn- with tru. I wouldn't make a strong case for choosing one over the other though, other than defaulting to use tru if possible.
There was a problem hiding this comment.
Good call out on i18n. I ended up using a deferred-trs. Deferred because it's in a def and trs because we don't have a logged in user in the routes used by this middleware. It's an alternative auth from a logged in user (it's an api key) so we would fall back to the server locale in either case.
calherries
suggested changes
Jan 5, 2023
Using site locale because this in an `+apikey` context, not an `+auth` context. The importance of that is that there is no logged in user so no way to get a user locale, and it just falls back to the server locale anyways. So lets just use that.
calherries
approved these changes
Jan 5, 2023
An annoying bit here. `api-key` setting in the notify test is set in the
env in the `:test` alias.
```clojure
notify-test=> (mt/with-temporary-setting-values [api-key nil]
(mw.auth/api-key))
"test-api-key" ;; where did the nil go?
```
But there are two bugs in `mt/with-temporary-setting-values`:
1. It gets the current env value and then temporarily sets it to the
current value. Note that `do-with-temp-env-var-value` is called with
`env-var-value` which is the current value in the environment. All of
this work for nothing
```
- (if-let [env-var-value (and (not raw-setting?) (#'setting/env-var-value setting-k))]
- (do-with-temp-env-var-value setting env-var-value thunk)
+ (if (and (not raw-setting?) (#'setting/env-var-value setting-k))
+ (do-with-temp-env-var-value setting-k value thunk)
```
So cannot possibly do what we want.
2. The second bug is that the pathway through
`mt/with-temporary-setting-values` did not convert `:api-key` to
`:mb-api-key`. So even if it put our new value in above, it would put
`:api-key nil` in the env, but when we look for it it would look for
`:mb-api-key` and we would not find it.
Thus the use of `setting-env-map-name`. Then clean up the few other
places that did it kinda right, but not necessarily. Seems the "correct"
way to do this is get the setting name, munge it, prepend with mb. The
other call sites were using variants of `(keyword (str "mb-" (name
setting-name)))` which is close but could miss the munging.
There are two ways that we put things into the env/env so they appear as environmentally set variables. - explicitly with `mt/with-temp-env-var-value`. This simulates a user setting these values and they are specified in the env way: mb-email-smtp-port, "MB_DISABLE_SCHEDULER", etc. At the call site you are aware that they are env related so get the mb prefix - incidentally with `mt/with-temporary-setting-values`. Settings are always referred to by their "nice" internal name like `api-key`, etc. But if a setting is found to be an env variable, we override it in env/env. But we have to make sure the key is set properly with an mb- prefix. Owing to this, we have to conditionally add the mb- prefix if not already present.
The tests themselves are testing useful stuff: site-url doesn't return a broken value if it fails the verification when set as an env variable. But the tests themselves were a bit nonsensical in their setup: ``` @@ -62,21 +62,19 @@ (deftest site-url-settings-normalize (testing "We should normalize `site-url` when set via env var we should still normalize it (#9764)" (mt/with-temp-env-var-value [mb-site-url "localhost:3000/"] - (mt/with-temporary-setting-values [site-url nil] - (is (= "localhost:3000/" - (setting/get-value-of-type :string :site-url))) - (is (= "http://localhost:3000" - (public-settings/site-url))))))) + (is (= "localhost:3000/" + (setting/get-value-of-type :string :site-url))) + (is (= "http://localhost:3000" + (public-settings/site-url)))))) ``` Why would it set an env-variable version [mb-site-url "localhost:3000/"] then try to blank it out with `(mt/with-temporary-setting-values [site-url nil])`. Fixing the bug in how we set env var values made this nonsensical stuff break. Now we just have to do the obvious thing: set the env var to a bad value, assert that (setting/get-value-of-type :string :site-url) is that value, but assert that the _setting_ is a well-formatted value or nil if we can't solve it (the "asd_12w31%$;" case). Setting the env value then setting a temporary value to nil didn't make much sense. And the fix made that set the env var to nil.
Codecov ReportBase: 64.89% // Head: 64.96% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #27515 +/- ##
==========================================
+ Coverage 64.89% 64.96% +0.06%
==========================================
Files 3178 3178
Lines 92312 92484 +172
Branches 11733 11730 -3
==========================================
+ Hits 59909 60080 +171
- Misses 27567 27571 +4
+ Partials 4836 4833 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
this allows us to set non-mb-prefixed env vars. not sure if necessary but let's roll with it.
|
@dpsutton could not create a backport due to conflicts |
dpsutton
added a commit
that referenced
this pull request
Jan 6, 2023
* nit alignment * point to var so changes are picked up need to refresh this ns after changing the middleware since these add a bit more helpful doc strings. But capture previous function value so more annoying at repl * Ensure MB_API_KEY is set for notify endpoint Previously when the MB_API_KEY was unset: ``` ❯ http POST "localhost:3000/api/notify/db/1?scan=schema" HTTP/1.1 403 Forbidden <other headers removed> Forbidden ``` And now: ``` ❯ http POST "localhost:3000/api/notify/db/1?scan=schema" HTTP/1.1 403 Forbidden <other headers removed> MB_API_KEY is not set. See https://www.metabase.com/docs/latest/configuring-metabase/environment-variables#mb_api_key for details ``` An annoying thing in the tests. We set `"-Dmb.api.key=test-api-key"` in the `:test` alias for CI, but not in regular dev mode. So annoyingly we either have tests that would fail in the repl in regular dev, or use `mt/with-temporary-setting-values [api-key "test-api-key"]` when writing tests that only matter for local stuff. c'est la vie * Translate error message Using site locale because this in an `+apikey` context, not an `+auth` context. The importance of that is that there is no logged in user so no way to get a user locale, and it just falls back to the server locale anyways. So lets just use that. * Fix temp settings when they are env-based An annoying bit here. `api-key` setting in the notify test is set in the env in the `:test` alias. ```clojure notify-test=> (mt/with-temporary-setting-values [api-key nil] (mw.auth/api-key)) "test-api-key" ;; where did the nil go? ``` But there are two bugs in `mt/with-temporary-setting-values`: 1. It gets the current env value and then temporarily sets it to the current value. Note that `do-with-temp-env-var-value` is called with `env-var-value` which is the current value in the environment. All of this work for nothing ``` - (if-let [env-var-value (and (not raw-setting?) (#'setting/env-var-value setting-k))] - (do-with-temp-env-var-value setting env-var-value thunk) + (if (and (not raw-setting?) (#'setting/env-var-value setting-k)) + (do-with-temp-env-var-value setting-k value thunk) ``` So cannot possibly do what we want. 2. The second bug is that the pathway through `mt/with-temporary-setting-values` did not convert `:api-key` to `:mb-api-key`. So even if it put our new value in above, it would put `:api-key nil` in the env, but when we look for it it would look for `:mb-api-key` and we would not find it. Thus the use of `setting-env-map-name`. Then clean up the few other places that did it kinda right, but not necessarily. Seems the "correct" way to do this is get the setting name, munge it, prepend with mb. The other call sites were using variants of `(keyword (str "mb-" (name setting-name)))` which is close but could miss the munging. * Safely set env/env keys There are two ways that we put things into the env/env so they appear as environmentally set variables. - explicitly with `mt/with-temp-env-var-value`. This simulates a user setting these values and they are specified in the env way: mb-email-smtp-port, "MB_DISABLE_SCHEDULER", etc. At the call site you are aware that they are env related so get the mb prefix - incidentally with `mt/with-temporary-setting-values`. Settings are always referred to by their "nice" internal name like `api-key`, etc. But if a setting is found to be an env variable, we override it in env/env. But we have to make sure the key is set properly with an mb- prefix. Owing to this, we have to conditionally add the mb- prefix if not already present. * cleanup the constants a bit * Fix some site-url tests The tests themselves are testing useful stuff: site-url doesn't return a broken value if it fails the verification when set as an env variable. But the tests themselves were a bit nonsensical in their setup: ``` @@ -62,21 +62,19 @@ (deftest site-url-settings-normalize (testing "We should normalize `site-url` when set via env var we should still normalize it (#9764)" (mt/with-temp-env-var-value [mb-site-url "localhost:3000/"] - (mt/with-temporary-setting-values [site-url nil] - (is (= "localhost:3000/" - (setting/get-value-of-type :string :site-url))) - (is (= "http://localhost:3000" - (public-settings/site-url))))))) + (is (= "localhost:3000/" + (setting/get-value-of-type :string :site-url))) + (is (= "http://localhost:3000" + (public-settings/site-url)))))) ``` Why would it set an env-variable version [mb-site-url "localhost:3000/"] then try to blank it out with `(mt/with-temporary-setting-values [site-url nil])`. Fixing the bug in how we set env var values made this nonsensical stuff break. Now we just have to do the obvious thing: set the env var to a bad value, assert that (setting/get-value-of-type :string :site-url) is that value, but assert that the _setting_ is a well-formatted value or nil if we can't solve it (the "asd_12w31%$;" case). Setting the env value then setting a temporary value to nil didn't make much sense. And the fix made that set the env var to nil. * fixup last tests * Don't modify things from `with-temp-env-var-value` this allows us to set non-mb-prefixed env vars. not sure if necessary but let's roll with it. * fix last of the tests
dpsutton
added a commit
that referenced
this pull request
Jan 16, 2023
* nit alignment * point to var so changes are picked up need to refresh this ns after changing the middleware since these add a bit more helpful doc strings. But capture previous function value so more annoying at repl * Ensure MB_API_KEY is set for notify endpoint Previously when the MB_API_KEY was unset: ``` ❯ http POST "localhost:3000/api/notify/db/1?scan=schema" HTTP/1.1 403 Forbidden <other headers removed> Forbidden ``` And now: ``` ❯ http POST "localhost:3000/api/notify/db/1?scan=schema" HTTP/1.1 403 Forbidden <other headers removed> MB_API_KEY is not set. See https://www.metabase.com/docs/latest/configuring-metabase/environment-variables#mb_api_key for details ``` An annoying thing in the tests. We set `"-Dmb.api.key=test-api-key"` in the `:test` alias for CI, but not in regular dev mode. So annoyingly we either have tests that would fail in the repl in regular dev, or use `mt/with-temporary-setting-values [api-key "test-api-key"]` when writing tests that only matter for local stuff. c'est la vie * Translate error message Using site locale because this in an `+apikey` context, not an `+auth` context. The importance of that is that there is no logged in user so no way to get a user locale, and it just falls back to the server locale anyways. So lets just use that. * Fix temp settings when they are env-based An annoying bit here. `api-key` setting in the notify test is set in the env in the `:test` alias. ```clojure notify-test=> (mt/with-temporary-setting-values [api-key nil] (mw.auth/api-key)) "test-api-key" ;; where did the nil go? ``` But there are two bugs in `mt/with-temporary-setting-values`: 1. It gets the current env value and then temporarily sets it to the current value. Note that `do-with-temp-env-var-value` is called with `env-var-value` which is the current value in the environment. All of this work for nothing ``` - (if-let [env-var-value (and (not raw-setting?) (#'setting/env-var-value setting-k))] - (do-with-temp-env-var-value setting env-var-value thunk) + (if (and (not raw-setting?) (#'setting/env-var-value setting-k)) + (do-with-temp-env-var-value setting-k value thunk) ``` So cannot possibly do what we want. 2. The second bug is that the pathway through `mt/with-temporary-setting-values` did not convert `:api-key` to `:mb-api-key`. So even if it put our new value in above, it would put `:api-key nil` in the env, but when we look for it it would look for `:mb-api-key` and we would not find it. Thus the use of `setting-env-map-name`. Then clean up the few other places that did it kinda right, but not necessarily. Seems the "correct" way to do this is get the setting name, munge it, prepend with mb. The other call sites were using variants of `(keyword (str "mb-" (name setting-name)))` which is close but could miss the munging. * Safely set env/env keys There are two ways that we put things into the env/env so they appear as environmentally set variables. - explicitly with `mt/with-temp-env-var-value`. This simulates a user setting these values and they are specified in the env way: mb-email-smtp-port, "MB_DISABLE_SCHEDULER", etc. At the call site you are aware that they are env related so get the mb prefix - incidentally with `mt/with-temporary-setting-values`. Settings are always referred to by their "nice" internal name like `api-key`, etc. But if a setting is found to be an env variable, we override it in env/env. But we have to make sure the key is set properly with an mb- prefix. Owing to this, we have to conditionally add the mb- prefix if not already present. * cleanup the constants a bit * Fix some site-url tests The tests themselves are testing useful stuff: site-url doesn't return a broken value if it fails the verification when set as an env variable. But the tests themselves were a bit nonsensical in their setup: ``` @@ -62,21 +62,19 @@ (deftest site-url-settings-normalize (testing "We should normalize `site-url` when set via env var we should still normalize it (#9764)" (mt/with-temp-env-var-value [mb-site-url "localhost:3000/"] - (mt/with-temporary-setting-values [site-url nil] - (is (= "localhost:3000/" - (setting/get-value-of-type :string :site-url))) - (is (= "http://localhost:3000" - (public-settings/site-url))))))) + (is (= "localhost:3000/" + (setting/get-value-of-type :string :site-url))) + (is (= "http://localhost:3000" + (public-settings/site-url)))))) ``` Why would it set an env-variable version [mb-site-url "localhost:3000/"] then try to blank it out with `(mt/with-temporary-setting-values [site-url nil])`. Fixing the bug in how we set env var values made this nonsensical stuff break. Now we just have to do the obvious thing: set the env var to a bad value, assert that (setting/get-value-of-type :string :site-url) is that value, but assert that the _setting_ is a well-formatted value or nil if we can't solve it (the "asd_12w31%$;" case). Setting the env value then setting a temporary value to nil didn't make much sense. And the fix made that set the env var to nil. * fixup last tests * Don't modify things from `with-temp-env-var-value` this allows us to set non-mb-prefixed env vars. not sure if necessary but let's roll with it. * fix last of the tests
dpsutton
added a commit
that referenced
this pull request
Jan 16, 2023
* nit alignment * point to var so changes are picked up need to refresh this ns after changing the middleware since these add a bit more helpful doc strings. But capture previous function value so more annoying at repl * Ensure MB_API_KEY is set for notify endpoint Previously when the MB_API_KEY was unset: ``` ❯ http POST "localhost:3000/api/notify/db/1?scan=schema" HTTP/1.1 403 Forbidden <other headers removed> Forbidden ``` And now: ``` ❯ http POST "localhost:3000/api/notify/db/1?scan=schema" HTTP/1.1 403 Forbidden <other headers removed> MB_API_KEY is not set. See https://www.metabase.com/docs/latest/configuring-metabase/environment-variables#mb_api_key for details ``` An annoying thing in the tests. We set `"-Dmb.api.key=test-api-key"` in the `:test` alias for CI, but not in regular dev mode. So annoyingly we either have tests that would fail in the repl in regular dev, or use `mt/with-temporary-setting-values [api-key "test-api-key"]` when writing tests that only matter for local stuff. c'est la vie * Translate error message Using site locale because this in an `+apikey` context, not an `+auth` context. The importance of that is that there is no logged in user so no way to get a user locale, and it just falls back to the server locale anyways. So lets just use that. * Fix temp settings when they are env-based An annoying bit here. `api-key` setting in the notify test is set in the env in the `:test` alias. ```clojure notify-test=> (mt/with-temporary-setting-values [api-key nil] (mw.auth/api-key)) "test-api-key" ;; where did the nil go? ``` But there are two bugs in `mt/with-temporary-setting-values`: 1. It gets the current env value and then temporarily sets it to the current value. Note that `do-with-temp-env-var-value` is called with `env-var-value` which is the current value in the environment. All of this work for nothing ``` - (if-let [env-var-value (and (not raw-setting?) (#'setting/env-var-value setting-k))] - (do-with-temp-env-var-value setting env-var-value thunk) + (if (and (not raw-setting?) (#'setting/env-var-value setting-k)) + (do-with-temp-env-var-value setting-k value thunk) ``` So cannot possibly do what we want. 2. The second bug is that the pathway through `mt/with-temporary-setting-values` did not convert `:api-key` to `:mb-api-key`. So even if it put our new value in above, it would put `:api-key nil` in the env, but when we look for it it would look for `:mb-api-key` and we would not find it. Thus the use of `setting-env-map-name`. Then clean up the few other places that did it kinda right, but not necessarily. Seems the "correct" way to do this is get the setting name, munge it, prepend with mb. The other call sites were using variants of `(keyword (str "mb-" (name setting-name)))` which is close but could miss the munging. * Safely set env/env keys There are two ways that we put things into the env/env so they appear as environmentally set variables. - explicitly with `mt/with-temp-env-var-value`. This simulates a user setting these values and they are specified in the env way: mb-email-smtp-port, "MB_DISABLE_SCHEDULER", etc. At the call site you are aware that they are env related so get the mb prefix - incidentally with `mt/with-temporary-setting-values`. Settings are always referred to by their "nice" internal name like `api-key`, etc. But if a setting is found to be an env variable, we override it in env/env. But we have to make sure the key is set properly with an mb- prefix. Owing to this, we have to conditionally add the mb- prefix if not already present. * cleanup the constants a bit * Fix some site-url tests The tests themselves are testing useful stuff: site-url doesn't return a broken value if it fails the verification when set as an env variable. But the tests themselves were a bit nonsensical in their setup: ``` @@ -62,21 +62,19 @@ (deftest site-url-settings-normalize (testing "We should normalize `site-url` when set via env var we should still normalize it (#9764)" (mt/with-temp-env-var-value [mb-site-url "localhost:3000/"] - (mt/with-temporary-setting-values [site-url nil] - (is (= "localhost:3000/" - (setting/get-value-of-type :string :site-url))) - (is (= "http://localhost:3000" - (public-settings/site-url))))))) + (is (= "localhost:3000/" + (setting/get-value-of-type :string :site-url))) + (is (= "http://localhost:3000" + (public-settings/site-url)))))) ``` Why would it set an env-variable version [mb-site-url "localhost:3000/"] then try to blank it out with `(mt/with-temporary-setting-values [site-url nil])`. Fixing the bug in how we set env var values made this nonsensical stuff break. Now we just have to do the obvious thing: set the env var to a bad value, assert that (setting/get-value-of-type :string :site-url) is that value, but assert that the _setting_ is a well-formatted value or nil if we can't solve it (the "asd_12w31%$;" case). Setting the env value then setting a temporary value to nil didn't make much sense. And the fix made that set the env var to nil. * fixup last tests * Don't modify things from `with-temp-env-var-value` this allows us to set non-mb-prefixed env vars. not sure if necessary but let's roll with it. * fix last of the tests
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #15157
History: If you want to scan a database you can hit
api/notify/db/<db-id>?scan=schemato do a quick schema scan. This is exposed to programatic users with the user of an api key. Previously, we had a bug where if a request failed to include the key, but the instance also hadn't set a key, the two keys were (vacuously) identical, and the request succeeded. That was fixed a while ago and we require the key being set.This change gives a more informative error message if the key is not set. The behavior is still the same-- unauthenticated response, but now the body gives some insight:
Before
With this change:
This change is