Dave, * Dave Page (dp...@pgadmin.org) wrote: > OK, so before I start hacking again, here's a proposal based on my > understanding of folks comments, and so open questions. If I can get > agreement and answers, I'll be able to break out vi again without > (hopefully) too many more revisions: > > pg_read_all_stats: Will have C-coded access to pg_stats views and > pg_*_size that are currently hard-coded to superuser
Not quite sure what you mean here by 'C-coded access to pg_stats *views*', but I'm guessing that isn't exactly what you meant since I'm sure we aren't going to change how view permissions are done in this patch. I take it you mean access to the functions under the views? If so, I believe this is correct. > pg_read_all_settings: Will have C-coded access to read GUCs that are > currently hard-coded to the superuser Right. > pg_monitor: Will include pg_read_all_stats and pg_read_all_settings, > and all explicitly GRANTable rights, e.g. in contrib modules. Right. > Patch to be rebased on Simon's updated version. Right. > Questions: > > - pg_stat_statements has a hard-coded superuser check. Shall I remove > that, and include REVOKE ALL FROM ALL and then GRANT to pg_monitor? pg_stat_statements shouldn't have ever had that superuser check and it shouldn't have ever used '==' for the user check, it should have been using 'has_privs_of_role()' from the start, which means that the superuser check isn't necessary. I don't think we should remove that check, nor should we REVOKE EXECUTE from public for the function. We *should* add a hard-coded role check to allow another role which isn't a member of the role whose query it is to view the query. That shouldn't be pg_monitor, of course (as discussed). I don't think pg_read_all_stats or pg_read_all_settings really covers this case either- this is more like pg_read_all_queries and should also be used for pg_stat_activity. That would then be granted to pg_monitor. > - pgrowlocks has hard-coded access to superuser and users with SELECT > on the table being examined. How should this be handled? I don't see any hard-coded superuser check? There is a pg_class_aclcheck() for SELECT rights on the table. I like the idea of having another way to grant access to run this function on a table besides giving SELECT rights on the entire table to the user. This would fall under the mandate of the role described in your next bullet, in my view. > - Stephen suggested a separate role for functions that can lock > tables. Is this still desired, or shall we just grant access to > pg_monitor (I think the latter is fine)? Right, I was thinking something like pg_stat_all_tables or pg_stat_scan_tables or similar. We would add that (perhaps also with a SELECT check like pgrowlocks has) for the other functions like pgstattuple and pg_freespacemap and pg_visibility. > - Based on Peter's concerns, is pg_read_all_stats the right name? > Maybe pg_read_monitoring_stats? I'd rather not get too wrapped up in the naming.. The documentation is the important part here, imv. Thanks! Stephen
signature.asc
Description: Digital signature
