Skip to content
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.

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

Add check access database for system tables #2856

Conversation

zhang2014
Copy link
Contributor

I'm not sure it's necessary. It helps, though, to shield messy data from multiple user scenarios.

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

@zhang2014
Copy link
Contributor Author

zhang2014 commented Aug 13, 2018

I don't know how to check system.processes table

@zhang2014 zhang2014 force-pushed the feature/add_check_access_database_for_system_tables branch from 01660c6 to 2611769 Compare August 13, 2018 09:22
@zhang2014 zhang2014 force-pushed the feature/add_check_access_database_for_system_tables branch from 2611769 to 59b2581 Compare August 13, 2018 09:36
@filimonov
Copy link
Contributor

I think system database should be generally available for all the users - as it contain system.number, system.one and other very handy tables. Only few tables can be considered as potentially unsafe: system.query_log, system.zookeeper. System.processes can be limited to current user only for users with limited access.

@@ -618,6 +618,13 @@ void Context::checkDatabaseAccessRights(const std::string & database_name) const
checkDatabaseAccessRightsImpl(database_name);
}

bool Context::isDatabaseAccessRights(const String & database_name) const
Copy link
Member

Choose a reason for hiding this comment

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

isDatabaseAccessRights -> hasDatabaseAccessRights

@alexey-milovidov
Copy link
Member

@filimonov This pull request just filters content of system tables, that contain information about another tables. All system tables remain available, but only with information about tables that the user is allowed to see.

@alexey-milovidov
Copy link
Member

@zhang2014 We can't filter system.processes, system.query_log, because they don't contain information about database. Even more, the query can use multiple databases. Lets just leave it as is.

@alexey-milovidov
Copy link
Member

@zhang2014 The only missing part is integration test.

@zhang2014 zhang2014 force-pushed the feature/add_check_access_database_for_system_tables branch from 430b42c to e51264c Compare August 14, 2018 03:28
@alexey-milovidov alexey-milovidov merged commit f42bd36 into ClickHouse:master Aug 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants