feat: Setup metabase_read_only role
#2856
Conversation
|
Removed vultr server and associated DNS entries |
| The `metabase_read_only` role is granted to the `metabase_user` database user, which is manually set up on both staging and production with the following SQL - | ||
|
|
||
| ```sql | ||
| CREATE USER metabase_user WITH PASSWORD <$PASSWORD>; | ||
| GRANT metabase_read_only TO metabase_user; | ||
| ``` | ||
|
|
||
| The password for Staging and Production databases can be found the OSL 1Password account. |
There was a problem hiding this comment.
This is the solution I'm proposing for setting up the metabase_user user. Not perfect but hopefully workable?
Once this PR is merged I'll manually run the above SQL on staging and prod and save passwords to 1Password. We could put them in Pulumi secrets, but my concern would be that as there's no code-reference to them they would be sensible candidates to "tidy up" accidentally.
Chances are it will exist on staging and production and we'll never really need to deal with it locally.
There was a problem hiding this comment.
This seems like a good thoughtful approach and will make for less-hassle rotation if necesary in future, but not entirely following based on this doc how this user relates to existing metabase user role provisioned/managed by Pulumi - eg https://github.com/theopensystemslab/planx-new/blob/main/infrastructure/application/index.ts#L100
At minimum, do we need to think about more distinctive naming? At most, is there a way to consolidate responsibilities of each into a single user role?
There was a problem hiding this comment.
Yeah this caught me out initially also.
This section of code is generating a role and user to be used for the MB_DB_CONNECTION_URI env var (docs), which is responsible for the schema and role of the internal metabase db, where dashboards etc are stored - not the PlanX schema where data is accessed via Hasura / ShareDB.
I'll add a code comment to Pulumi to make this explicit, as well as document this in the "how to" in this PR.
Do you think this would be enough?
There was a problem hiding this comment.
Yep happy with extra docs here !
There was a problem hiding this comment.
Please see 4dd9b14
DafyddLlyr
force-pushed
the
dp/metabase-read-only-user
branch
from
047e9ee to
b27deee
Compare
| GRANT SELECT ON public.flows TO metabase_read_only; | ||
| GRANT SELECT ON public.published_flows TO metabase_read_only; | ||
| GRANT SELECT ON public.teams TO metabase_read_only; |
There was a problem hiding this comment.
This might actually be redundant - I think there might be enough information in the views? If it's simpler to remove this I'm all for it 👍
There was a problem hiding this comment.
I'll contact Ollie today, or manually check Metabase, to see if this is required 👍
There was a problem hiding this comment.
Removed - these aren't currently used. See https://opensystemslab.slack.com/archives/C5Q59R3HB/p1709720214019159 (OSL Slack)
There was a problem hiding this comment.
Update - in order to move this forward I've granted access to the tables analytics and analytics_logs. They are safe to read, and currently used by SQL questions. The aim would be that once we migrate to GUI questions, we can rely on summary tables and tidy this up.
@zz-hh-aa Indicated that lowcal_sessions is used, which we do not want, so I've intentionally left this out to force a transition to submission_services_summary.
If this breaking change it too harsh, we can toggle the PG username/password used in Metabase until this is resolved.
There was a problem hiding this comment.
I think I've removed lowcal_sessions as a source from all the questions now anyway, and am only using summary tables + analytics or analytics_logs, so don't think anything should break! All good on my end.
There was a problem hiding this comment.
Ah that's perfect @zz-hh-aa thanks very much!
|
Perfect - thanks very much @jessicamcinchak for the speedy re-review here ✅ |
What does this PR do?
metabase_read_onlydatabase roleTesting
I've gone ahead and set up the user and permissions outlined here on staging Metabase so that we can test a little (I'll revoke the changes before merging this PR).
It works as expected - for example, I can read
sessions_summary, but running SQL to select fromlowcal_sessionsgives a permissions error.