这是indexloc提供的服务，不要输入任何密码
Skip to content

rename access-key to admin-secret (close #1347) #1540

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 55 commits into from
Feb 14, 2019
Merged

rename access-key to admin-secret (close #1347) #1540

merged 55 commits into from
Feb 14, 2019

Conversation

@nizar-m
Copy link
Contributor

@nizar-m nizar-m commented Jan 31, 2019 •
edited by shahidhk
Loading

Description

Rename the admin secret key header used to access GraphQL engine from X-Hasura-Access-Key to X-Hasura-Admin-Secret

What component does this PR affect?

  • Server
  • Console
  • CLI
  • Docs
  • Community Content
  • Build System

Requires changes from other components? If yes, please mark the components:

  • Server
  • Console
  • CLI
  • Docs
  • Community Content
  • Build System

Related Issue

#1347

Solution and Design

  • Server
    • Change envVar HASURA_GRAPHQL_ACCESS_KEY to HASURA_GRAPHQL_ADMIN_SECRET
    • Change flag --access-key to --admin-secret
    • Change the admin secret key header from X-Hasura-Access-Key to X-Hasura-Admin-Secret
    • Server will still support the following (even though they are deprecated)
      • header X-Hasura-Access-Key
      • EnvVar HASURA_GRAPHQL_ACCESS_KEY
      • flag --access-key
  • Update references in the docs
    • Change pngs dev-mode-role-header-access-key.png and access-key-console.png
  • Console
  • Cli
    • Change cli flags --access-key to --admin-secret
    • Tests with hasura cli for both --access-key and --admin-secret flags
    • Test config.yaml with both access_key and admin_secret
  • Install manifests
    • Azure
    • Docker run
    • Docker compose
    • Kubernetes
    • DigitalOcean

Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Docs update
  • Community content

Checklist:

  • I have read the contributing guide and my code conforms to the guidelines.
  • This change requires a change in the documentation.
  • I have updated the documentation accordingly.
  • I have added required tests.

@nizar-m
1) Support both X-Hasura-Access-Key and X-Hasura-Admin-Secret as head…
89012bd 
…ers for access key in the server

2) Change X-Hasura-Access-Key to X-Hasura-Admin-Secret everywhere else
@nizar-m
In server and docs
f42d064 
1) Change EnvVar HASURA_GRAPHQL_ACCESS_KEY to HASURA_GRAPHQL_ADMIN_SECRET
2) Change flag --access-key to --admin-secret
@nizar-m
Test to check whether header X-Hasura-Access-Key still works
ede9ff4
@hasura-bot
Copy link
Contributor

hasura-bot commented Jan 31, 2019

Review app for commit ede9ff4 deployed to Heroku: https://hge-ci-pull-1540.herokuapp.com
Docker image for server: hasura/graphql-engine:pull1540-ede9ff4

@patcito patcito mentioned this pull request Jan 31, 2019
Closed
@hasura-bot
Copy link
Contributor

hasura-bot commented Jan 31, 2019

Review app for commit 614219e deployed to Heroku: https://hge-ci-pull-1540.herokuapp.com
Docker image for server: hasura/graphql-engine:pull1540-614219e

@nizar-m nizar-m force-pushed the issue-1347 branch 2 times, most recently from ed4fa6b to 9bb863d Compare January 31, 2019 17:28
@hasura-bot
Copy link
Contributor

hasura-bot commented Jan 31, 2019

Review app for commit 9bb863d deployed to Heroku: https://hge-ci-pull-1540.herokuapp.com
Docker image for server: hasura/graphql-engine:pull1540-9bb863d

@nizar-m
Add tests for
cd157e1 
  1) Flags --access-key and --admin-secret
  2) EnvVars HASURA_GRAPHQL_ACCESS_KEY and HASURA_GRAPHQL_ADMIN_SECRET
@nizar-m
Change access key to admin secret in community folder
c148e26
@nizar-m
1) Change 'access key' to 'admin secret' in cli
af611d8 
2) Support both --access-key and --admin-secret key flags for admin secret in cli commands
@nizar-m
Change 'access key' to 'admin secret' in console
05494fc
@hasura-bot
Copy link
Contributor

hasura-bot commented Feb 3, 2019

Review app for commit 443eae0 deployed to Heroku: https://hge-ci-pull-1540.herokuapp.com
Docker image for server: hasura/graphql-engine:pull1540-443eae0

@shahidhk shahidhk added c/console Related to console c/cli Related to CLI c/server Related to server labels Feb 3, 2019
@shahidhk
Copy link
Member

shahidhk commented Feb 3, 2019 •
edited
Loading

Reviews required from @0x777 @shahidhk @rikinsk and @praveenweb before merging.
Add ok-to-merge label only after these reviews.

@shahidhk shahidhk added the s/do-not-merge Do not merge this pull request to master label Feb 3, 2019
@praveenweb
Copy link
Member

praveenweb commented Feb 4, 2019

@nizar-m - console has to be backwards compatible. Global replace of x-hasura-access-key to x-hasura-admin-secret might not work.

@nizar-m nizar-m changed the title Rename X-Hasura-Access-Key to X-Hasura-Admin-Secret (close #1347) Rename Access-Key to Admin-Secret (close #1347) Feb 4, 2019
praveenweb
praveenweb reviewed Feb 4, 2019
View reviewed changes
praveenweb
praveenweb reviewed Feb 4, 2019
View reviewed changes
@hasura-bot
Copy link
Contributor

hasura-bot commented Feb 12, 2019

Review app for commit 5467900 deployed to Heroku: https://hge-ci-pull-1540.herokuapp.com
Docker image for server: hasura/graphql-engine:pull1540-5467900

@hasura-bot
Copy link
Contributor

hasura-bot commented Feb 12, 2019

Review app for commit 877bd98 deployed to Heroku: https://hge-ci-pull-1540.herokuapp.com
Docker image for server: hasura/graphql-engine:pull1540-877bd98

@hasura-bot
Copy link
Contributor

hasura-bot commented Feb 12, 2019

Review app for commit 4af3059 deployed to Heroku: https://hge-ci-pull-1540.herokuapp.com
Docker image for server: hasura/graphql-engine:pull1540-4af3059

0x777
0x777 previously approved these changes Feb 12, 2019
View reviewed changes
@dsandip
Copy link
Member

dsandip commented Feb 13, 2019

@shahidhk @praveenweb Are CLI/Console reviews needed? If yes, please do so?

praveenweb
praveenweb reviewed Feb 14, 2019
View reviewed changes
praveenweb
praveenweb previously approved these changes Feb 14, 2019
View reviewed changes
Copy link
Member

@praveenweb praveenweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Sidenote - Existing users who are logged in might be logged out, because of the change in localstorage variables.

@hasura-bot
Copy link
Contributor

hasura-bot commented Feb 14, 2019

Review app for commit d2d2fa1 deployed to Heroku: https://hge-ci-pull-1540.herokuapp.com
Docker image for server: hasura/graphql-engine:pull1540-d2d2fa1

@hasura-bot
Copy link
Contributor

hasura-bot commented Feb 14, 2019

Review app for commit 1f656e0 deployed to Heroku: https://hge-ci-pull-1540.herokuapp.com
Docker image for server: hasura/graphql-engine:pull1540-1f656e0

@shahidhk shahidhk changed the title Rename Access-Key to Admin-Secret (close #1347) rename access-key to admin-secret (close #1347) Feb 14, 2019
shahidhk
shahidhk approved these changes Feb 14, 2019
View reviewed changes
Copy link
Member

@shahidhk shahidhk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on the CLI changes.

Ran a basic sanity check with server, console, and cli to check backwards compatibility.

@shahidhk shahidhk merged commit f83a8e5 into hasura:master Feb 14, 2019
@hasura-bot
Copy link
Contributor

hasura-bot commented Feb 14, 2019

Review app https://hge-ci-pull-1540.herokuapp.com is deleted

@rizrmd
Copy link

rizrmd commented Feb 14, 2019 •
edited
Loading

Does this means we should add X-Hasura-Admin-Secret to our graphql queries on each client ?

I am asking because currently we need to provide X-Hasura-Access-Key on each query headers to make it work, I think it would make sense if we should not provide this at all when querying, as admin-secret is used to access hasura console.

Pardon me if it were suggested before.

@shahidhk
Copy link
Member

shahidhk commented Feb 25, 2019

@rizkyramadhan You should not use AccessKey/AdminSecret to query data from client side. It is your super-admin-master-password. Anyone can wipe out your database if they have access to this key/secret.

Setup authentication/access control: https://docs.hasura.io/1.0/graphql/manual/auth/index.html

hasura-bot pushed a commit that referenced this pull request Jan 16, 2025
@jberryman @hasura-bot
un-revert #1511, fixing performance MBS regression (#1540)
a92c456 
Running `build_serialize_roundtrip` on chinook locally I see 181ms ->
54ms which tracks with
https://psychic-doodle-372212k.pages.github.io/dev/bench/ so this should
be good

<!-- The PR description should answer 2 important questions: -->

### What

<!-- What is this PR trying to accomplish (and why, if it's not
obvious)? -->

<!-- Consider: do we need to add a changelog entry? -->

<!-- Does this PR introduce new validation that might break old builds?
-->

<!-- Consider: do we need to put new checks behind a flag? -->

### How

<!-- How is it trying to accomplish it (what are the implementation
steps)? -->

V3_GIT_ORIGIN_REV_ID: 47fba2c5db7dcb0da42ab17132c18ce7f798bebc
hasura-bot pushed a commit that referenced this pull request Jan 27, 2025
@danieljharvey @hasura-bot
Remove accidentally added sql crate file (#1543)
c36fc8c 
<!-- The PR description should answer 2 important questions: -->

### What

This was added in #1540 by accident, Cargo ignores it, removing.

V3_GIT_ORIGIN_REV_ID: 06546990dc8fd72c57c4700b9d874cc3787c71c4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shahidhk shahidhk shahidhk approved these changes

@0x777 0x777 0x777 left review comments

@praveenweb praveenweb praveenweb left review comments

@wawhal wawhal Awaiting requested review from wawhal

@tirumaraiselvan tirumaraiselvan Awaiting requested review from tirumaraiselvan

+1 more reviewer

@rikinsk-zz rikinsk-zz rikinsk-zz left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@nizar-m nizar-m

@wawhal wawhal

Labels

c/cli Related to CLI c/console Related to console c/server Related to server s/do-not-merge Do not merge this pull request to master

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.