Retrieve the API Key from the url parameters #416
Conversation
bidoubiwa
force-pushed
the
get-token-from-url
branch
from
a4698b6 to
477f5fb
Compare
bidoubiwa
force-pushed
the
get-token-from-url
branch
2 times, most recently
from
2dc6c01 to
75a53d8
Compare
bidoubiwa
force-pushed
the
get-token-from-url
branch
from
75a53d8 to
e7c3b59
Compare
bidoubiwa
force-pushed
the
get-token-from-url
branch
from
e7c3b59 to
44914ff
Compare
.github/workflows/tests.yml
Outdated
| @@ -83,7 +83,7 @@ jobs: | |||
| with: | |||
| start: yarn start:ci | |||
| wait-on: 'http://0.0.0.0:3000' | |||
| command: yarn cy:run | |||
| command: yarn cy:run:test-no-api-key | |||
There was a problem hiding this comment.
Can you double check that all the tests are being run? I recommend you check the cy:run command inside the package.json, which runs all the tests except some specific ones.
.github/workflows/tests.yml
Outdated
| options: --user 1001 | ||
| services: | ||
| meilisearch: | ||
| image: getmeili/meilisearch:v0.30.5 |
There was a problem hiding this comment.
Do you want to update this to v1?
.github/workflows/tests.yml
Outdated
| @@ -137,3 +137,45 @@ jobs: | |||
| with: | |||
| name: cypress-videos | |||
| path: cypress/videos | |||
| cypress_meilisearch-api-key-query-param: | |||
There was a problem hiding this comment.
Why didn't you put all of this inside the cypress_meilisearch-api-key above which already makes tests with a running Meilisearch instance + API key?
| cy.get('span').contains('Api Key').parent().click() | ||
| cy.get('div[aria-label=settings-api-key]').within(() => { | ||
| cy.get('input[name="apiKey"]').should('have.value', API_KEY) | ||
| cy.get('button').contains('Go').click() |
There was a problem hiding this comment.
You don't really need this last line :)
package.json
Outdated
| "cy:run:test-no-meilisearch": "cypress run --spec '**/*/test-no-meilisearch.cy.js'", | ||
| "cy:run:test-api-key-required": "cypress run --spec '**/*/test-api-key-required.cy.js'", | ||
| "cy:run:test-api-key-query-param": "cypress run --spec '**/*/test-api-key-query-param.cy.js'", | ||
| "cy:run:test-no-api-key": "cypress run --spec '**/*/test-no-api-key-required.cy.js'", | ||
| "cy:run": "cypress run --config excludeSpecPattern=['**/*/test-no-meilisearch.cy.js','**/*/test-api-key-required.cy.js']", |
There was a problem hiding this comment.
Initially I had those commands for those specific tests:
cy:run:test-no-meilisearch: tests when there is no Meilisearch instance runningcy:run:test-api-key-required: tests that require a Meilisearch instance running + an API keycy:run: tests that require a Meilisearch instance running (without an API key here for simplicity)
I had those 3 because I needed to run 3 different instances in the CI.
I don't really get why you added more commands in the package.json nor more jobs in the CI, but I'm available to discuss it of you want :)
There was a problem hiding this comment.
What do you think about making the mini-dashboard redirect to a clean URL (by removing the query parameter from it) and storing it in the local storage?
It is related to this message from @davelarkan (private link).
Is there a security concern here by reducing this friction? Will users understand that the master key in the URL can do damage if it's leaked?
|
Removing the query parameters from an url is not common if I'm not mistaken. Either way, it presents a security risk as it's not because we remove the api_key from the URL that the user will suddenly not share the original URL. if the user adds the |
|
bors merge |
|
Build succeeded:
|
What does this PR do?
This PR allow the mini-dashboard to receive an API Key (admin or master key) from an url parameter. This url parameter is
api_key. So you if you call the dashboard{url}/?api_key={admin_key}, the mini-dashboard will never show the popup that is asking for the API Key.See it live:
https://user-images.githubusercontent.com/6064892/223150249-a7492569-bff9-4523-acc8-cf17fc1a3e8a.mp4