[tarantool-patches] [PATCH v1 1/1] sql: check access rights of table in VIEW

[tarantool-patches] [PATCH v1 1/1] sql: check access rights of table in VIEW

[Kirill Shcherbatov]

When access is performed using VIEW, access rights should be
checked against table[s] which it is referencing, not against
VIEW itself. Added a test case to verify this behaviour.

Closes #4104
---
Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-4104-view-access-check
Issue: https://github.com/tarantool/tarantool/issues/4104

 test/sql/gh-4104-view-access-check.result   | 64 +++++++++++++++++++++
 test/sql/gh-4104-view-access-check.test.lua | 21 +++++++
 2 files changed, 85 insertions(+)
 create mode 100644 test/sql/gh-4104-view-access-check.result
 create mode 100644 test/sql/gh-4104-view-access-check.test.lua

diff --git a/test/sql/gh-4104-view-access-check.result b/test/sql/gh-4104-view-access-check.result
new file mode 100644
index 000000000..1eb9bebe8
--- /dev/null
+++ b/test/sql/gh-4104-view-access-check.result
@@ -0,0 +1,64 @@
+test_run = require('test_run').new()
+---
+...
+box.execute("CREATE TABLE supersecret(id INT PRIMARY KEY, data TEXT);")
+---
+- row_count: 1
+...
+box.execute("CREATE TABLE supersecret2(id INT PRIMARY KEY, data TEXT);")
+---
+- row_count: 1
+...
+box.execute("INSERT INTO supersecret VALUES(1, 'very very big secret');")
+---
+- row_count: 1
+...
+box.execute("INSERT INTO supersecret2 VALUES(1, 'very big secret 2');")
+---
+- row_count: 1
+...
+box.execute("CREATE VIEW supersecret_leak AS  SELECT * FROM supersecret, supersecret2;")
+---
+- row_count: 1
+...
+LISTEN = require('uri').parse(box.cfg.listen)
+---
+...
+remote = require 'net.box'
+---
+...
+cn = remote.connect(LISTEN.host, LISTEN.service)
+---
+...
+box.schema.user.grant('guest','read', 'space', 'SUPERSECRET_LEAK')
+---
+...
+cn:execute('SELECT * FROM SUPERSECRET_LEAK')
+---
+- error: Read access to space 'SUPERSECRET' is denied for user 'guest'
+...
+box.schema.user.grant('guest','read', 'space', 'SUPERSECRET')
+---
+...
+cn:execute('SELECT * FROM SUPERSECRET_LEAK')
+---
+- error: Read access to space 'SUPERSECRET2' is denied for user 'guest'
+...
+box.schema.user.revoke('guest','read', 'space', 'SUPERSECRET')
+---
+...
+box.schema.user.revoke('guest','read', 'space', 'SUPERSECRET_LEAK')
+---
+...
+box.execute("DROP VIEW supersecret_leak")
+---
+- row_count: 1
+...
+box.execute("DROP TABLE supersecret")
+---
+- row_count: 1
+...
+box.execute("DROP TABLE supersecret2")
+---
+- row_count: 1
+...
diff --git a/test/sql/gh-4104-view-access-check.test.lua b/test/sql/gh-4104-view-access-check.test.lua
new file mode 100644
index 000000000..2a44516ce
--- /dev/null
+++ b/test/sql/gh-4104-view-access-check.test.lua
@@ -0,0 +1,21 @@
+test_run = require('test_run').new()
+
+box.execute("CREATE TABLE supersecret(id INT PRIMARY KEY, data TEXT);")
+box.execute("CREATE TABLE supersecret2(id INT PRIMARY KEY, data TEXT);")
+box.execute("INSERT INTO supersecret VALUES(1, 'very very big secret');")
+box.execute("INSERT INTO supersecret2 VALUES(1, 'very big secret 2');")
+box.execute("CREATE VIEW supersecret_leak AS  SELECT * FROM supersecret, supersecret2;")
+LISTEN = require('uri').parse(box.cfg.listen)
+remote = require 'net.box'
+cn = remote.connect(LISTEN.host, LISTEN.service)
+
+box.schema.user.grant('guest','read', 'space', 'SUPERSECRET_LEAK')
+cn:execute('SELECT * FROM SUPERSECRET_LEAK')
+box.schema.user.grant('guest','read', 'space', 'SUPERSECRET')
+cn:execute('SELECT * FROM SUPERSECRET_LEAK')
+
+box.schema.user.revoke('guest','read', 'space', 'SUPERSECRET')
+box.schema.user.revoke('guest','read', 'space', 'SUPERSECRET_LEAK')
+box.execute("DROP VIEW supersecret_leak")
+box.execute("DROP TABLE supersecret")
+box.execute("DROP TABLE supersecret2")
-- 
2.21.0

[tarantool-patches] Re: [PATCH v1 1/1] sql: check access rights of table in VIEW

[Vladislav Shpilevoy]

Thanks for the patch!

On 24/04/2019 15:16, Kirill Shcherbatov wrote:
> When access is performed using VIEW, access rights should be
> checked against table[s] which it is referencing, not against
> VIEW itself. Added a test case to verify this behaviour.
> 
> Closes #4104
> ---
> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-4104-view-access-check
> Issue: https://github.com/tarantool/tarantool/issues/4104
> 
>  test/sql/gh-4104-view-access-check.result   | 64 +++++++++++++++++++++
>  test/sql/gh-4104-view-access-check.test.lua | 21 +++++++
>  2 files changed, 85 insertions(+)
>  create mode 100644 test/sql/gh-4104-view-access-check.result
>  create mode 100644 test/sql/gh-4104-view-access-check.test.lua
> 
> diff --git a/test/sql/gh-4104-view-access-check.result b/test/sql/gh-4104-view-access-check.result
> new file mode 100644
> index 000000000..1eb9bebe8
> --- /dev/null
> +++ b/test/sql/gh-4104-view-access-check.result
> @@ -0,0 +1,64 @@
> +test_run = require('test_run').new()

1. You do not use this object, so it can be omitted.

> +---
> +...
> +box.execute("CREATE TABLE supersecret(id INT PRIMARY KEY, data TEXT);")
> +---
> +- row_count: 1
> +...
> +box.execute("CREATE TABLE supersecret2(id INT PRIMARY KEY, data TEXT);")
> +---
> +- row_count: 1
> +...
> +box.execute("INSERT INTO supersecret VALUES(1, 'very very big secret');")
> +---
> +- row_count: 1
> +...
> +box.execute("INSERT INTO supersecret2 VALUES(1, 'very big secret 2');")
> +---
> +- row_count: 1
> +...
> +box.execute("CREATE VIEW supersecret_leak AS  SELECT * FROM supersecret, supersecret2;")
> +---
> +- row_count: 1
> +...
> +LISTEN = require('uri').parse(box.cfg.listen)
> +---
> +...
> +remote = require 'net.box'
> +---
> +...
> +cn = remote.connect(LISTEN.host, LISTEN.service)

2. You pass here box.cfg.listen directly, without splitting into
parts.

Please, consider my review fixes below and on the branch:

===================================================================
diff --git a/test/sql/gh-4104-view-access-check.result b/test/sql/gh-4104-view-access-check.result
index 1eb9bebe8..d38b633c3 100644
--- a/test/sql/gh-4104-view-access-check.result
+++ b/test/sql/gh-4104-view-access-check.result
@@ -1,6 +1,3 @@
-test_run = require('test_run').new()
----
-...
 box.execute("CREATE TABLE supersecret(id INT PRIMARY KEY, data TEXT);")
 ---
 - row_count: 1
@@ -21,13 +18,10 @@ box.execute("CREATE VIEW supersecret_leak AS  SELECT * FROM supersecret, superse
 ---
 - row_count: 1
 ...
-LISTEN = require('uri').parse(box.cfg.listen)
----
-...
 remote = require 'net.box'
 ---
 ...
-cn = remote.connect(LISTEN.host, LISTEN.service)
+cn = remote.connect(box.cfg.listen)
 ---
 ...
 box.schema.user.grant('guest','read', 'space', 'SUPERSECRET_LEAK')
diff --git a/test/sql/gh-4104-view-access-check.test.lua b/test/sql/gh-4104-view-access-check.test.lua
index 2a44516ce..d1d19fc28 100644
--- a/test/sql/gh-4104-view-access-check.test.lua
+++ b/test/sql/gh-4104-view-access-check.test.lua
@@ -1,13 +1,10 @@
-test_run = require('test_run').new()
-
 box.execute("CREATE TABLE supersecret(id INT PRIMARY KEY, data TEXT);")
 box.execute("CREATE TABLE supersecret2(id INT PRIMARY KEY, data TEXT);")
 box.execute("INSERT INTO supersecret VALUES(1, 'very very big secret');")
 box.execute("INSERT INTO supersecret2 VALUES(1, 'very big secret 2');")
 box.execute("CREATE VIEW supersecret_leak AS  SELECT * FROM supersecret, supersecret2;")
-LISTEN = require('uri').parse(box.cfg.listen)
 remote = require 'net.box'
-cn = remote.connect(LISTEN.host, LISTEN.service)
+cn = remote.connect(box.cfg.listen)
 
 box.schema.user.grant('guest','read', 'space', 'SUPERSECRET_LEAK')
 cn:execute('SELECT * FROM SUPERSECRET_LEAK')

[tarantool-patches] Re: [PATCH v1 1/1] sql: check access rights of table in VIEW

[Kirill Shcherbatov]

On 24.04.2019 16:48, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>> +test_run = require('test_run').new()
> 
> 1. You do not use this object, so it can be omitted.

> 2. You pass here box.cfg.listen directly, without splitting into
> parts.
> 
> Please, consider my review fixes below and on the branch:

Looks good for me, thanks. Amend.

[tarantool-patches] Re: [PATCH v1 1/1] sql: check access rights of table in VIEW

[Vladislav Shpilevoy]

LGTM.

On 24/04/2019 17:02, Kirill Shcherbatov wrote:
> On 24.04.2019 16:48, Vladislav Shpilevoy wrote:
>> Thanks for the patch!
>>> +test_run = require('test_run').new()
>>
>> 1. You do not use this object, so it can be omitted.
> 
>> 2. You pass here box.cfg.listen directly, without splitting into
>> parts.
>>
>> Please, consider my review fixes below and on the branch:
> 
> Looks good for me, thanks. Amend.
> 

[tarantool-patches] Re: [PATCH v1 1/1] sql: check access rights of table in VIEW

[Kirill Yukhin]

Hello,

On 24 Apr 15:16, Kirill Shcherbatov wrote:
> When access is performed using VIEW, access rights should be
> checked against table[s] which it is referencing, not against
> VIEW itself. Added a test case to verify this behaviour.
> 
> Closes #4104
> ---
> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-4104-view-access-check
> Issue: https://github.com/tarantool/tarantool/issues/4104

I've checked your patch into master and 2.1 branch.

--
Regards, Kirill Yukhin