Skip to content
Snippets Groups Projects
  • dpsutton's avatar
    55e063e5
    Notify api key not set hints (#27515) · 55e063e5
    dpsutton authored
    * 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
    Notify api key not set hints (#27515)
    dpsutton authored
    * 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
Code owners
Assign users and groups as approvers for specific file changes. Learn more.