Implement permissions on system tables part II

[danielmewes]

In 2.3 we ended up disallowing system table access for non-admin users for simplicity reasons.

Copying the original proposal from #5463 here:

• Read permissions on system tables will be assigned through the usual mechanism (i.e. the "rethinkdb" database is treated like any other database)
• Nobody can write to a system table, except for the admin user who can write to all system tables
• One exception from the rule: Users can update their own document in the users table, as long as they have read and write permissions on the users table as granted through the usual mechanism.

Note that operations such as r.table(...).config().update(...) or r.table(...).reconfigure(...) can still be performed by non-admin users, if the user has config permissions on the affected table.

Additionally we should filter jobs output so users only see their own queries unless they are admin.

[VeXocide]

In CR 3702 by @danielmewes.

[marshall007]

@danielmewes will this also ensure the output of .dbList() and .tableList() is filtered to only include tables the user has permissions to read?

[danielmewes]

@marshall007 The current implementation has all tables and databases visible to everybody, even if they don't have any permissions to read or write to them.

Do you have a use case for which you'd want these commands to filter our inaccessible tables?

Note that even if we filter out tables with no permissions, your application would often still need to handle permission errors when accessing the table(s), since a user might only have read but not write or config permissions. There are probably a few cases where all you need is read permissions, and there it would usually work out.

[marshall007]

@danielmewes I don't have any specific use-cases in mind. It just seems like more appropriate behavior since otherwise (currently) there are pitfalls associated with .tableList().forEach(...) and permissions.

It would be a nice solution to #5946 and even more handy once changefeeds are supported in #5517.

You make a good point though that depending on what you're trying to do you may wish to filter based on permissions other than read. Perhaps it would make sense to add an optarg? I guess the main point here is that db/tableList are not easy to use once you start adding permissions, but they should be.

[mlucy]

I think hiding tables based on permissions would be pretty confusing, because people with permissions set incorrectly will think their tables just disappeared. Adding an optarg to tableList and dbList that you can set to only get tables you can read from, write to, or administrate seems like a reasonable option though.

[danielmewes]

I opened a new issue #6006 for the tableList/dbList discussion.

[danielmewes]

One more thought: We currently have three _debug tables in the rethinkdb database: _debug_scratch (basically an in-memory table for testing that you can save arbitrary documents in), _debug_table_status (extended version of table_status) and _debug_stats (extended version of stats).

My opinion is that we should disallow access to these for all non-admin users. While I don't think there's much harm that a user could do by accessing these tables (other than maybe overload memory by writing to _debug_scratch or using it as a hidden channel), forbidding access feels safer to me. Someone might grant access to the rethinkdb database for a user, and not be aware of the existence of the _debug tables (since they are hidden and not documented).

On the other hand, restricting access to the admin user only would also mean that you cannot override this restriction by granting permissions on a _debug table explicitly (due to the way permission checking is implemented).

Does anyone think it would be better to allow access to these tables for non-admin users?

[VeXocide]

Note that this comment is essentially a brain dump resulting from a discussion @danielmewes and I had in person.

• The system database and tables will only have read and write permissions, the config permission can be set but since no configuration changes can currently be made to either the database or tables this doesn't have any impact.
• The system database and tables will not inherit permissions from the global scope, this means global read and write permissions can be set for real tables and will not influence configuration permissions through writing to the system tables.
• Writing to either the db_config or table_config will also check whether you have config permissions to these objects. The object is considered the primary and the system tables a view on these.
• r.grant will check whether you have permissions on the permissions system table, here the system table is considered the primary and grant a shortcut to writing to this table.
• As stated above we will limit all _debug tables to the admin user only.

[VeXocide]

We're making two more changes to the system table permissions.

• Somewhere along the way, we decided to hide the admin user from the permissions table, we will add it back in with two fake entries indicating all permissions.
• We'll add a user key to the permission table documents containing the user.

[VeXocide]

Merged into next via commit 3ab9225, adding and porting tests to YAML is next.

[VeXocide]

For the record, the relevant reviews are CR 3704 and CR 3748.

[danielmewes]

@VeXocide Do you remember if the tests were good to merge? Unfortunately all review data is gone.

[VeXocide]

@danielmewes It seems I merged these into next via commit 3ab9225 but forgot to close the issue, guess I'll do so now