From d5936b25554f434d9e36e982db32ddbfac6a67a4 Mon Sep 17 00:00:00 2001
From: Konstantin Osipov <kostja@tarantool.org>
Date: Thu, 18 Dec 2014 00:11:16 +0300
Subject: [PATCH] Implement error checks in grant/revoke.
---
src/box/alter.cc | 3 ++-
src/box/lua/schema.lua | 27 ++++++++++++++++++++-------
src/errcode.h | 4 ++++
test/box/access.result | 1 +
test/box/access_misc.result | 3 +++
test/box/box.net.box.result | 3 +++
test/box/box.net.box.test.lua | 1 +
test/box/misc.result | 6 +++++-
test/box/protocol.result | 3 +++
test/box/protocol.test.lua | 1 +
test/box/role.result | 24 ++++++++++++++++++++++++
test/box/role.test.lua | 14 ++++++++++++++
test/box/session.result | 3 +++
test/box/session.test.lua | 1 +
test/replication/cluster.result | 3 +++
test/replication/cluster.test.py | 1 +
16 files changed, 89 insertions(+), 9 deletions(-)
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 2808315656..c2b1b97182 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1502,7 +1502,8 @@ priv_def_check(struct priv_def *priv)
role ? role->name :
int2str(priv->object_id));
}
- /* Only the creator of the role can grant or revoke it.
+ /*
+ * Only the creator of the role can grant or revoke it.
* Everyone can grant 'PUBLIC' role.
*/
if (role->owner != grantor->uid && grantor->uid != ADMIN &&
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index cd2303342d..6df1b9b413 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -1044,7 +1044,7 @@ box.schema.user.grant = function(user_name, privilege, object_type,
if uid == nil then
box.error(box.error.NO_SUCH_USER, user_name)
end
- privilege = privilege_resolve(privilege)
+ privilege_hex = privilege_resolve(privilege)
local oid = object_resolve(object_type, object_name)
if grantor == nil then
grantor = session.uid()
@@ -1060,11 +1060,19 @@ box.schema.user.grant = function(user_name, privilege, object_type,
else
old_privilege = 0
end
- privilege = bit.bor(privilege, old_privilege)
+ privilege_hex = bit.bor(privilege_hex, old_privilege)
-- do not execute a replace if it does not change anything
- -- XXX bug: new grantor replaces the old one, old grantor is lost
- if privilege ~= old_privilege then
- _priv:replace{grantor, uid, object_type, oid, privilege}
+ -- XXX bug if we decide to add a grant option: new grantor
+ -- replaces the old one, old grantor is lost
+ if privilege_hex ~= old_privilege then
+ _priv:replace{grantor, uid, object_type, oid, privilege_hex}
+ else
+ if object_type == 'role' then
+ box.error(box.error.ROLE_GRANTED, user_name, object_name)
+ else
+ box.error(box.error.PRIV_GRANTED, user_name, privilege,
+ object_type, object_name)
+ end
end
end
@@ -1082,13 +1090,18 @@ box.schema.user.revoke = function(user_name, privilege, object_type, object_name
if uid == nil then
box.error(box.error.NO_SUCH_USER, name)
end
- privilege = privilege_resolve(privilege)
local oid = object_resolve(object_type, object_name)
local _priv = box.space[box.schema.PRIV_ID]
local tuple = _priv:get{uid, object_type, oid}
if tuple == nil then
- return
+ if object_type == 'role' then
+ box.error(box.error.ROLE_NOT_GRANTED, user_name, object_name)
+ else
+ box.error(box.error.PRIV_NOT_GRANTED, user_name, privilege,
+ object_type, object_name)
+ end
end
+ privilege = privilege_resolve(privilege)
local old_privilege = tuple[5]
local grantor = tuple[1]
-- sic:
diff --git a/src/errcode.h b/src/errcode.h
index 5160e86988..a128fe3319 100644
--- a/src/errcode.h
+++ b/src/errcode.h
@@ -138,6 +138,10 @@ enum { TNT_ERRMSG_MAX = 512 };
/* 86 */_(ER_TUPLE_REF_OVERFLOW, 1, "Tuple reference counter overflow") \
/* 87 */_(ER_ROLE_LOOP, 2, "Granting role '%s' to role '%s' would create a loop") \
/* 88 */_(ER_GRANT, 2, "Incorrect grant arguments: %s") \
+ /* 89 */_(ER_PRIV_GRANTED, 2, "User '%s' already has %s access on %s '%s'") \
+ /* 90 */_(ER_ROLE_GRANTED, 2, "User '%s' already has role '%s'") \
+ /* 91 */_(ER_PRIV_NOT_GRANTED, 2, "User '%s' does not have %s access on %s '%s'") \
+ /* 92 */_(ER_ROLE_NOT_GRANTED, 2, "User '%s' does not have role '%s'") \
/*
* !IMPORTANT! Please follow instructions at start of the file
diff --git a/test/box/access.result b/test/box/access.result
index 975ddea6cc..2e5bf7f937 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -360,6 +360,7 @@ box.space._priv:select{id}
...
box.schema.user.grant('user', 'read', 'universe')
---
+- error: User 'user' already has read access on universe 'nil'
...
box.space._priv:select{id}
---
diff --git a/test/box/access_misc.result b/test/box/access_misc.result
index 27e7d7afb7..b181cbba87 100644
--- a/test/box/access_misc.result
+++ b/test/box/access_misc.result
@@ -53,6 +53,7 @@ box.schema.user.grant('testus', 'read', 'space', 'admin_space')
...
box.schema.user.grant('testus', 'read', 'space', 'admin_space')
---
+- error: User 'testus' already has read access on space 'admin_space'
...
session.su('testus')
---
@@ -84,6 +85,7 @@ box.schema.user.revoke('testus', 'read', 'space', 'admin_space')
...
box.schema.user.revoke('testus', 'read', 'space', 'admin_space')
---
+- error: User 'testus' does not have read access on space 'admin_space'
...
session.su('testus')
---
@@ -402,6 +404,7 @@ session.su('admin')
...
box.schema.user.revoke('testuser', 'read, write, execute', 'universe')
---
+- error: User 'testuser' does not have read, write, execute access on universe 'nil'
...
--
-- Check that itertors check privileges
diff --git a/test/box/box.net.box.result b/test/box/box.net.box.result
index 19202db6c0..448a5644ed 100644
--- a/test/box/box.net.box.result
+++ b/test/box/box.net.box.result
@@ -758,3 +758,6 @@ file_log:close()
---
- true
...
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+---
+...
diff --git a/test/box/box.net.box.test.lua b/test/box/box.net.box.test.lua
index a244dd421a..2839f9eb83 100644
--- a/test/box/box.net.box.test.lua
+++ b/test/box/box.net.box.test.lua
@@ -311,3 +311,4 @@ end;
--# setopt delimiter ''
file_log:close()
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
diff --git a/test/box/misc.result b/test/box/misc.result
index e08f06bee6..4a6b2751e5 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -207,8 +207,12 @@ t;
- 'box.error.USER_EXISTS : 46'
- 'box.error.WAL_IO : 40'
- 'box.error.CREATE_USER : 43'
+ - 'box.error.DROP_PRIMARY_KEY : 17'
+ - 'box.error.PRIV_GRANTED : 89'
- 'box.error.CREATE_SPACE : 9'
+ - 'box.error.PRIV_NOT_GRANTED : 91'
- 'box.error.GRANT : 88'
+ - 'box.error.ROLE_GRANTED : 90'
- 'box.error.TUPLE_REF_OVERFLOW : 86'
- 'box.error.UNKNOWN_SCHEMA_OBJECT : 49'
- 'box.error.PROC_LUA : 32'
@@ -258,7 +262,7 @@ t;
- 'box.error.ARG_TYPE : 26'
- 'box.error.INDEX_FIELD_COUNT : 39'
- 'box.error.READONLY : 7'
- - 'box.error.DROP_PRIMARY_KEY : 17'
+ - 'box.error.ROLE_NOT_GRANTED : 92'
- 'box.error.DROP_SPACE : 11'
- 'box.error.UNKNOWN_REQUEST_TYPE : 48'
- 'box.error.INVALID_XLOG_ORDER : 76'
diff --git a/test/box/protocol.result b/test/box/protocol.result
index 86698ffcea..0783bf6302 100644
--- a/test/box/protocol.result
+++ b/test/box/protocol.result
@@ -51,3 +51,6 @@ conn:close()
space:drop()
---
...
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+---
+...
diff --git a/test/box/protocol.test.lua b/test/box/protocol.test.lua
index 1349541bbc..307909c843 100644
--- a/test/box/protocol.test.lua
+++ b/test/box/protocol.test.lua
@@ -18,3 +18,4 @@ conn.space[space.id]:select(3, { iterator = 'LT' })
conn:close()
space:drop()
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
diff --git a/test/box/role.result b/test/box/role.result
index df3d02c808..310edbaabf 100644
--- a/test/box/role.result
+++ b/test/box/role.result
@@ -67,9 +67,11 @@ box.schema.user.grant('tester', 'execute', 'role', 'tester')
-- test granting a non-execute grant on a role - error
box.schema.user.grant('tester', 'write', 'role', 'iddqd')
---
+- error: User 'tester' already has role 'iddqd'
...
box.schema.user.grant('tester', 'read', 'role', 'iddqd')
---
+- error: User 'tester' already has role 'iddqd'
...
-- test granting role to a role
box.schema.user.grant('iddqd', 'execute', 'role', 'iddqd')
@@ -82,9 +84,11 @@ box.schema.user.grant('iddqd', 'iddqd')
...
box.schema.user.revoke('iddqd', 'iddqd')
---
+- error: User 'iddqd' does not have role 'iddqd'
...
box.schema.user.grant('tester', 'iddqd')
---
+- error: User 'tester' already has role 'iddqd'
...
box.schema.user.revoke('tester', 'iddqd')
---
@@ -631,10 +635,12 @@ box.schema.user.create('john')
box.session.su('john')
---
...
+-- error
box.schema.role.grant('grantee', 'role')
---
- error: Read access denied for user 'john' to space '_user'
...
+--
box.session.su('admin')
---
...
@@ -655,6 +661,24 @@ box.schema.role.grant('grantee', 'read', 'space', 'test')
---
- error: Read access denied for user 'john'
...
+--
+-- granting 'public' is however an exception - everyone
+-- can grant 'public' role, it's implicitly granted with
+-- a grant option.
+--
+box.schema.role.grant('grantee', 'public')
+---
+- error: User 'grantee' already has role 'public'
+...
+--
+-- revoking role 'public' is another deal - only the
+-- superuser can do that, and even that would be useless,
+-- since one can still re-grant it back to oneself.
+--
+box.schema.role.revoke('grantee', 'public')
+---
+- error: Create or drop access denied for user 'john'
+...
box.session.su('admin')
---
...
diff --git a/test/box/role.test.lua b/test/box/role.test.lua
index 3af64a559f..14acba2421 100644
--- a/test/box/role.test.lua
+++ b/test/box/role.test.lua
@@ -244,13 +244,27 @@ box.schema.role.grant('grantee', 'role')
box.schema.role.revoke('grantee', 'role')
box.schema.user.create('john')
box.session.su('john')
+-- error
box.schema.role.grant('grantee', 'role')
+--
box.session.su('admin')
_ = box.schema.space.create('test')
box.schema.user.grant('john', 'read,write,execute', 'universe')
box.session.su('john')
box.schema.role.grant('grantee', 'role')
box.schema.role.grant('grantee', 'read', 'space', 'test')
+--
+-- granting 'public' is however an exception - everyone
+-- can grant 'public' role, it's implicitly granted with
+-- a grant option.
+--
+box.schema.role.grant('grantee', 'public')
+--
+-- revoking role 'public' is another deal - only the
+-- superuser can do that, and even that would be useless,
+-- since one can still re-grant it back to oneself.
+--
+box.schema.role.revoke('grantee', 'public')
box.session.su('admin')
box.schema.user.drop('john')
diff --git a/test/box/session.result b/test/box/session.result
index a087de691b..49e15c2196 100644
--- a/test/box/session.result
+++ b/test/box/session.result
@@ -224,3 +224,6 @@ fiber = nil
session = nil
---
...
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+---
+...
diff --git a/test/box/session.test.lua b/test/box/session.test.lua
index da1bc63e8d..367755ebd1 100644
--- a/test/box/session.test.lua
+++ b/test/box/session.test.lua
@@ -88,3 +88,4 @@ session.uid()
session.user()
fiber = nil
session = nil
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
diff --git a/test/replication/cluster.result b/test/replication/cluster.result
index 8be88a7195..fc11db893f 100644
--- a/test/replication/cluster.result
+++ b/test/replication/cluster.result
@@ -131,3 +131,6 @@ box.info.vclock[11]
---
- 0
...
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+---
+...
diff --git a/test/replication/cluster.test.py b/test/replication/cluster.test.py
index aa41dc0a30..5beec7a8e8 100644
--- a/test/replication/cluster.test.py
+++ b/test/replication/cluster.test.py
@@ -98,3 +98,4 @@ replica.admin('box.info.vclock[%d]' % replica_id3)
# Cleanup
sys.stdout.pop_filter()
+master.admin("box.schema.user.revoke('guest', 'read,write,execute', 'universe')")
--
GitLab