set embedding-secret-key when embedding is enabled #40813
Merged
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
|
npretto
force-pushed
the
embed-homepage-fix-secret-key-be
branch
from
fd1556d
to
dda6cca
Compare
npretto
commented
Mar 29, 2024
| cy.request("PUT", "/api/setting/embedding-secret-key", { | ||
| value: METABASE_SECRET_KEY, | ||
| }); | ||
| cy.request("PUT", "/api/setting/enable-embedding", { value: true }).then( |
There was a problem hiding this comment.
I wanted to make sure we don't run into race conditions between the two calls, making the tests flaky
npretto
changed the title
noahmoss
reviewed
Mar 29, 2024
src/metabase/embed/settings.clj
Outdated
Comment on lines
30
to
31
| (when (and new-value (str/blank? (setting/get :embedding-secret-key))) | ||
| (setting/set! :embedding-secret-key (crypto-random/hex 32))) |
There was a problem hiding this comment.
In general we prefer using the automatically-generated getter/setter functions for settings which are the same as the setting name, with a ! suffix for the setter.
Suggested change
| (when (and new-value (str/blank? (setting/get :embedding-secret-key))) | |
| (setting/set! :embedding-secret-key (crypto-random/hex 32))) | |
| (when (and new-value (str/blank? (embedding-secret-key))) | |
| (embedding-secret-key! (crypto-random/hex 32))) |
(This assumes that embedding-secret-key is defined in the same namespace... if not you'll have to include the namespace as well.)
deniskaber
approved these changes
Apr 3, 2024
npretto
force-pushed
the
embed-homepage-dismiss
branch
2 times, most recently
from
31bda47
to
3faee3a
Compare
npretto
force-pushed
the
embed-homepage-fix-secret-key-be
branch
from
dca0965
to
cad018c
Compare
deniskaber
pushed a commit
that referenced
this pull request
Apr 5, 2024
* create ui for the full embedding homepage, without any logic (#40251) * wip ui for the full embedding homepage * adds missing link + fix keys * remove done TODO * Update frontend/src/metabase/home/components/EmbedHomepage/InteractiveTabContent.tsx Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev> * destructure props in the function arguments * rename customerFacingAnalyticsDocsUrl to analyticsDocsUrl --------- Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev> * add new settings for embedding homepage (#40455) * remove old minimal version * adds new embedding homepage settings and sets them at the end of the setup * comment e2e test * add tests for embedding homepage flags * more tests * remove unit tests related to old minimal version * fixes flags descriptions * :keyword for embedding-homepage Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev> * replace reloadSettings and refreshSettingsList with initializeSettings * `value => value === true` -> `Boolean` * toMatchObject -> toEqual for more strict tests * remove useless undefined check for settings, the component is conditionally rendered based on a setting so it means they're loaded anyway * fix: use :hidden as we changed the type to :keyword * specifically check for the `embedding` feature in the tokenFeatures object instead of checking if any feature is active * remove double \n Co-authored-by: Noah Moss <32746338+noahmoss@users.noreply.github.com> --------- Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev> Co-authored-by: Noah Moss <32746338+noahmoss@users.noreply.github.com> * connect settings to the embed homepage and show it (#40528) * fix nested a warnings in browser console * connect settings to the embed homepage and show it * fix stories * remove localStorage.clear() as we're not using it in the logic anymore * dismiss without feedback for the embedding homepage (#40587) * feat: add dismiss functionality to the embed homepage * update setup e2e to check for the dismissal of embedding homepage * use List from metabase/ui (#40638) * set embedding-secret-key when embedding is enabled (#40813) * set embedding-secret-key when embedding is enabled * use "named" setters and getters as suggested by Noah * Fix tests for ms1 after updating on master (#41069) --------- Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev> Co-authored-by: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in 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.
Note: this is stacked on top of a bunch of PRs from #40005
Description
In add new settings for embedding homepage I enable embedding when the user is interested in embedding, but I don't set the secret key.
This cause the embed modal to crash as it expects to find it.
The solution I attempted in this PR is to move the logic to set the secret key from the FE to the BE, on the setter of the setting, so that when the embedding is enabled, if no secret key is found, a new one is created.
I think this is a more robust solution now that we have two places to enable embedding.
How to verify
or