[tarantool-patches] [PATCH v2] Introduce separate entity object types for entity privileges.

Tarantool development patches archive
 help / color / mirror / Atom feed

* [tarantool-patches] [PATCH v2] Introduce separate entity object types for entity privileges.
@ 2018-08-02 10:55 Serge Petrenko
  2018-08-07 16:38 ` Vladimir Davydov
  0 siblings, 1 reply; 6+ messages in thread
From: Serge Petrenko @ 2018-08-02 10:55 UTC (permalink / raw)
  To: tarantool-patches, kostja; +Cc: Serge Petrenko

When granting or revoking a privilege on an entire entity, id 0 was used
to indicate the fact that we don't grant a privilege on a single object,
but on a whole entity. This caused confusion, because for entity USER,
for example, id 0 is a valid object id (user 'guest' uses it).
Any non-zero id dedicated to this cause obviously may be confused as well.
Fix this by creating separate schema_object_types for entities:
SC_ENTITY_SPACE, SC_ENTITY_USER, etc.

Closes: #3574
Prerequisite: #3524
---
https://github.com/tarantool/tarantool/tree/sergepetrenko/gh-3574-whole-entity-types
https://github.com/tarantool/tarantool/issues/3574

Changes in v2: 
  - keep only old syntax for granting access
    to all entities
  - add an upgrade script to alter indices of spaces
    _priv and _vpriv to store 'scalar' in object_id field,
    and use an asterisk ('*') in object_id to indicate
    granting on an entire entity.
  - keep the new entity types in priv_def and use them
    internally.

 src/box/alter.cc             |  27 ++++++++++++++++++++++++++-
 src/box/bootstrap.snap       | Bin 1540 -> 1557 bytes
 src/box/lua/schema.lua       |  41 ++++++++++++++++++++++++-----------------
 src/box/lua/upgrade.lua      |  37 +++++++++++++++++++++++++++++++++++++
 src/box/schema.cc            |  18 ++++++++++++++++++
 src/box/schema.h             |  23 +++++++++++++----------
 src/box/schema_def.c         |  39 +++++++++++++++++++++++++++++++--------
 src/box/schema_def.h         |  28 ++++++++++++++++++++++------
 src/box/user.cc              |  27 +++++++++++++++------------
 test/box-py/bootstrap.result |  14 +++++++-------
 test/box/access.result       |   6 +++---
 test/box/access_misc.result  |   8 ++++----
 test/box/alter.result        |   8 ++++----
 test/xlog/upgrade.result     |  14 +++++++-------
 14 files changed, 211 insertions(+), 79 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 3007a131d..6057b66c9 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2529,6 +2529,31 @@ on_replace_dd_collation(struct trigger * /* trigger */, void *event)
 	}
 }
 
+void
+priv_def_try_fill(struct priv_def *priv, struct tuple *tuple)
+{
+	const char *object_id = tuple_field(tuple, BOX_PRIV_FIELD_OBJECT_ID);
+	if (object_id == NULL) {
+		tnt_raise(ClientError, ER_NO_SUCH_FIELD,
+			  BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE);
+	}
+	/*
+	 * When granting or revoking privileges on a whole entity we
+	 * pass an asterisk ('*') to object_id to indicate grant on every
+	 * object of that entity. So check for that first.
+	 */
+	if (mp_typeof(*object_id) == MP_STR) {
+		priv->object_id = 0;
+		priv->object_type = schema_entity_type(priv->object_type);
+	} else if (mp_typeof(*object_id) == MP_UINT) {
+		priv->object_id = mp_decode_uint(&object_id);
+	} else {
+		tnt_raise(ClientError, ER_FIELD_TYPE,
+			  BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE,
+			  field_type_strs[FIELD_TYPE_UNSIGNED]);
+	}
+}
+
 /**
  * Create a privilege definition from tuple.
  */
@@ -2539,8 +2564,8 @@ priv_def_create_from_tuple(struct priv_def *priv, struct tuple *tuple)
 	priv->grantee_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_UID);
 	const char *object_type =
 		tuple_field_cstr_xc(tuple, BOX_PRIV_FIELD_OBJECT_TYPE);
-	priv->object_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_OBJECT_ID);
 	priv->object_type = schema_object_type(object_type);
+	priv_def_try_fill(priv, tuple);
 	if (priv->object_type == SC_UNKNOWN) {
 		tnt_raise(ClientError, ER_UNKNOWN_SCHEMA_OBJECT,
 			  object_type);
diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
index b610828c9c9ae9a22acdd8c150c16c6838b7a273..da6961d1be7621fe9821623c7c2d57125e8ac7bc 100644
GIT binary patch
delta 1552
zcmV+r2JiWV43!L!7=JM>GBYwQXE`%5Ib$<nWMc|RZgX^DZewLSAZ0N(H8nRmV=XXd
zF*hwVWjJFkVlgvjEihy=V`616Vl-qkHVRflY;R+0Iv{&}3JTS_3%bn(ngGty8d0aE
z0000004TLD{QyvnEC6=>*h}EFr~oj`FvARU1MnJQi4e@o&wmVLh3G~crY-}4Qt)1A
zB2#HdNhp@x{7t9y{=-^iy?G~GRT*HEED-ei@Dn^G44RYT!B-?xN&&+Fx&Y1qV@|wy
z6m_Jn@7Frx341NeT$e$x{G;bbkGWq_9B~MwrUdvJKu^0mC)0U%u6xqfb3Hyc=uW3S
z?Yy*~VIJ-(N`EEV@r?j@mYTMGX?c^oS(oYGod@w`90IW?o~7muA2}mX0M&<$IO}p(
zf4&sYv(&5wsva}nU)$;MEH#TN5iEy0m!_+prRGbuYQ<Q1#lkEYO69^Y)ISUiiUp=t
zC>0okQ!15=OOySbmm=QXK9owS5|kbsI5j<pFj+#A<bTA{p5xH}*!A7AT`g}iJZp;q
zZa*(m3p`8BSuZbcKQF_xf+#w$E@QP&S5FE&OU<%B*MW5z3(smw@be+qmpn7j&(6bi
z4t2HV0FPP-q?QbLmYSt_Yv-+BSSX~f6!0uH^?h+1Um~{s3}1ZqeLFvQuFf=|{j%>m
z@5t{w$A3eAY2$e)R8<J*e6DdD-_^ohB@MWH-}BP;7}LqF7(2TzM;yY$&#@t$W%_kH
z9$%7&x;hfzS!$A(2E;oVI=}4a<;%;?_$pE>?lXx9syH+_Gd3aW42YQzE)T8_E)F-H
zZMkTx<ymTaw;U~*R7{GMf&o{c3QV2A(*~c@i+>4a(%@NY`j|3&7;fLG#dK@AP_j^}
zP;!~^J6oZkGC}22X(6$YN~9HQM=JH^bEJ+Wj3U%Z@+>t|M)c-`<Vzb_mpY#R-E$PB
z68(HF5MbZ$e*LnL=OE!wjv*E2foG|C>lf>C_fV+XIPfer%Uc*!XHsf;ldcAyrKaq~
zTz|`(6uRQYTqhKE{4`zxF)J9%3;Y*e!9Y}Om3pCY>tRqbsThdDE*Ta}CiN~&>g}76
z!4jk9%y>A~28QcJ%EhV0$D*{Nl2mw>nl_~poi;_)WA0d&u^?M<U+bo*s7Oh*@hmk*
z^!ff;)@2Y1&r<Wm(u(_9C-##iKP!s~oPR~~_fYj}UL1E0XYo~%x@vJBSVCtS?4Grf
z@GLby$YL|n#zmT&8l1t!jMuTP=pcEc^R-#cNb1fwio7!{#Iv85xz4|oot5G~aD->6
zNt2TTsE)CLA*jJzWK0dr004jhC;$-#M=54i4-$aDIEte%3SbZhL<l$vP80xu;D10^
zK@mhP7zBVEle%IQ1m-ef91jM-02ly+*uYMhP>(DFTh=t}nh{on7iA*E0z$QhnjhC8
zCtYIjZW*~2>%o=401{W2UfNK%SuRO#7z?U1OHmr$_k9#!4DKOfeHdDiLMUMhE_CY3
zQ}wM+3}S$xrp{sO0%D-Qg!Tg?N`D#c#11D%H1uMldybk2C_2GIa1@o`AvlV1g6&6a
zBm#;e0oq=kL|+Xk(;`Axi4jzulb=M-vXNUm0-}edv^<XF;UF|bEL;aGO3HyUXd&f5
z88k6w(AXZb_cg2b20{u-Z}C<nz{iAARh?|geYYKQ#VFi+XhmiG4;^%v=zqu$=*R1g
zez1GMebM(LHVMWg)roWz-LV|EY6+@Ey-NL+1I(uM&?2Z6(jNJW%xscWl(`{?w{V+H
z{3j}V(4Y_$d?5u8Nb<Kvi(X_xgWx*i-)Jo|L*giI$9)9W!7HhLCvO%PO>J)qU^;iT
zG&tlm=>j9yIEcC(gPg(fIe%Gp29&-BccznI$Z20Zp2%7_$W9>{;D>@m9ppFRLi`BZ
zxQFm2G$}GSoq&fM>MYyVZ?D8K%fMRllKy8xF*61u0i|dk(Um!gfT;c_$vzr)bR`hg
z8QidjRzibA!Kym`2j>)8MFY}}Gzpe5HBi$3iFF|fX;^3K2nxO;&<T@dg3AHb5UuS&
CN52^W

delta 1534
zcmV<a1p)e%41^4j7=JJ=G%zh^V>dT4F*z~{Np5p=VQyn(Iv_b=V_`LAF*YqTV`gS8
zG-hTvEnzrjFfCy?W;Qf4VP!dGGGq!?Lu_wjYdRo%F*+bOeF_TIx(m9^1&9F7A|rpb
zr2qf`001bpFZ}>e{VM<lJ=RLl7I6XqUp$Ch%_7lYJ_Lj_@PFoXP~+we(A76EO7~<&
z)F3;RA|;uhw(`4WgTRw>I&<{eU?~Ai$VOI~v629dx2JaIm@kx4N&&zCxB$xlBK}6v
zM#}nreKVfmYggvF>;bcooE<&pjzw|&ydgCd;B)Xi4C|Up=iRyONm;Mu=<J|49rmyj
z!~XSg?^aVP(0>n31mId~$~K1OP2OJJrGIzc!y|v*5Q}mxHFpG|K|%#jeQ^A;E^l>*
z3*Bp}NefgxX1+gm^KmUTgDMc1Jsb?P)oZC4!mL!?OgGcvU!7uNP_0u4V%4c@$$Tde
z<JJuBKo=+bJ1@oWHsew%rA$(CR4!bhCq$aiEJ3j}Cx2ttc)M<~yh-n@9Ru2aUZxgY
zOU+j=F>OCDy|bcHe0;hLW1g*=6kJQqvOo9n=`!@rT1v3PA=sEaGtUmrgF1V*+HwF$
z%o|ck2Ck)MDdN~U>lpNk)Rh9)Qd8p>$FU`TvVVQ}JNw3+pEy`&7|(v!cb#wKcU}Xc
zztr*E^M9!-1a3aZxQyRo-mQ)X;JxvQVS5a1E-Z#;*X8(mp!hlb(b=Wn_TxWHa?e&p
z0<NVdiD5jvlVdHR&M*6U`SOA@wvN<F<4PlXstt_|t42g+g<yeLeOP%|b(q;~x1wEl
zEj90~M>ms_NyVg8C=MtXRLX=hZO}2jlujiL*MCw|r<8%iFyl-urCG}bk_A!)lFN(Z
z*9rxd2`ZdQ%ZOz}8m(9>QmHeYBvm9q6rtA9wbTrW=uAh+mpZa8Z9M*WuhEnWw8OPP
z0Ken?`duE+M1Fc0sWuO;rRJ>TPnWxUKDEZdwbU$UUQcC(VtJFR2G>&aT`%TX-lUJ$
zzkkQk8h`!+Ak42Gb5O~I9X_GL4xhR(RGq@`<8YEmeGLBa%Ed{YaV#4wH7X6(wXrob
zRx(mCOfa|?Bo}qm;#z9jPz5?Ii>k-mKV61Bw$iw6MNm)>lUj2vHAVFK{#e#!kBV!l
ziDGG`ab1f2WXaFUg5t{}`TM8d6EBXt_J3uuRg$)9X&lOg&M*+4wU)S+njl=^i<EJZ
zX2ppwLyQ}LBXiMF<cgb*y<$jG_{C2oetkYX_VcpV`IoM<QW}RMaV<4zXt3a^V}T&3
z(OhIq3CsWhfB+}}AqGb&W<?JYfWSD4qc93!7zRWLI0_9I0RX{)ke~>l77Rj$ynhqj
zxeG%W18Ui|uoin^FOJH&6BE><l!R4e54%@5i^3tJ2=RjuhoRy}$qP!PWxP;CvBe2+
zB`AQ$m7GU4)NS@llAFhZ>1-;h#Us!zr2@yPWOP}$xk=>TS*Ej6Q8ZgBx&}sXh}Wym
z;Yvs=Xm6s&oTIkS5VXc28)t^2+kc4qR4r1#;*gdpU~^0s$pG8pm?G)gVGer7p5*j@
zrQSOro_cGm&fR}<@b8h^=iXW!%4reE=RqJFW<g443Z&4$8`^`Azzc12c=p(Su(w)T
z1)jmCmtK6UOn@l~=HxoFllhK1;>sqt`1BcS0HC@As3ba#z^Ko9Pi8avYk$Oa!!S#&
zSx3_!A8xyrpfT#3Y0zk_Hi}Y>p%$pI61F+5M3N&%2xKJAC;$CDD}<6?=$#5uoTZ5Z
zCNj}EgB|fKv=$L@ag>MEJ_76DnN*W1Z`O`MZ9ptwx|K1yxMV1V1(Ltx%(4y7obk4E
zW?=^8GIQ=kv4A0`wS7N1*>`Z!opukPx!_g@c{8|RJi<`!QS!z#<&+X#nnV8V6uo^%
zFL^|hfwSZ-+|LAZ&d5i?Zt>-$tMH_1UnT!Yb{@SussOv@WfJ50JyfLCQd9Bw_N-dh
k2=V>Mm|$S01}gnuSl5<E4okR>OyKJnJxL~`9Mur5?b8(5ga7~l

diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index b9b8c9004..294dcf4d3 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -1809,9 +1809,9 @@ local function object_resolve(object_type, object_name)
         return 0
     end
     if object_type == 'space' then
-        if object_name == nil or object_name == 0 then
-            return 0
-        end
+	if object_name == nil then
+	    return '*'
+	end
         local space = box.space[object_name]
         if  space == nil then
             box.error(box.error.NO_SUCH_SPACE, object_name)
@@ -1819,9 +1819,9 @@ local function object_resolve(object_type, object_name)
         return space.id
     end
     if object_type == 'function' then
-        if object_name == nil or object_name == 0 then
-            return 0
-        end
+	if object_name == nil then
+	    return '*'
+	end
         local _vfunc = box.space[box.schema.VFUNC_ID]
         local func
         if type(object_name) == 'string' then
@@ -1836,8 +1836,8 @@ local function object_resolve(object_type, object_name)
         end
     end
     if object_type == 'sequence' then
-        if object_name == nil or object_name == 0 then
-            return 0
+        if object_name == nil then
+            return '*'
         end
         local seq = sequence_resolve(object_name)
         if seq == nil then
@@ -1846,6 +1846,9 @@ local function object_resolve(object_type, object_name)
         return seq
     end
     if object_type == 'role' then
+	if object_name == nil then
+	    return '*'
+	end
         local _vuser = box.space[box.schema.VUSER_ID]
         local role
         if type(object_name) == 'string' then
@@ -1867,6 +1870,9 @@ local function object_name(object_type, object_id)
     if object_type == 'universe' then
         return ""
     end
+    if object_id == '*' then
+        return '*'
+    end
     local space
     if object_type == 'space' then
         space = box.space._vspace
@@ -2079,9 +2085,8 @@ local function grant(uid, name, privilege, object_type,
         object_name = privilege
         privilege = 'execute'
     end
-    local privilege_hex = privilege_check(privilege, object_type)
-
     local oid = object_resolve(object_type, object_name)
+    local privilege_hex = privilege_check(privilege, object_type)
     options = options or {}
     if options.grantor == nil then
         options.grantor = session.euid()
@@ -2106,10 +2111,10 @@ local function grant(uid, name, privilege, object_type,
         _priv:replace{options.grantor, uid, object_type, oid, privilege_hex}
     elseif not options.if_not_exists then
             if object_type == 'role' then
-                box.error(box.error.ROLE_GRANTED, name, object_name)
+                box.error(box.error.ROLE_GRANTED, name, object_name or '*')
             else
                 box.error(box.error.PRIV_GRANTED, name, privilege,
-                          object_type, object_name)
+                          object_type, object_name or '*')
             end
     end
 end
@@ -2122,9 +2127,9 @@ local function revoke(uid, name, privilege, object_type, object_name, options)
         object_name = privilege
         privilege = 'execute'
     end
-    local privilege_hex = privilege_check(privilege, object_type)
     options = options or {}
     local oid = object_resolve(object_type, object_name)
+    local privilege_hex = privilege_check(privilege, object_type)
     local _priv = box.space[box.schema.PRIV_ID]
     local _vpriv = box.space[box.schema.VPRIV_ID]
     local tuple = _vpriv:get{uid, object_type, oid}
@@ -2134,10 +2139,10 @@ local function revoke(uid, name, privilege, object_type, object_name, options)
             return
         end
         if object_type == 'role' then
-            box.error(box.error.ROLE_NOT_GRANTED, name, object_name)
+            box.error(box.error.ROLE_NOT_GRANTED, name, object_name or '*')
         else
             box.error(box.error.PRIV_NOT_GRANTED, name, privilege,
-                      object_type, object_name)
+                      object_type, object_name or '*')
         end
     end
     local old_privilege = tuple[5]
@@ -2153,13 +2158,14 @@ local function revoke(uid, name, privilege, object_type, object_name, options)
             return
         end
         box.error(box.error.PRIV_NOT_GRANTED, name, privilege,
-                  object_type, object_name)
+                  object_type, object_name or '*')
     end
     if privilege_hex ~= 0 then
         _priv:replace{grantor, uid, object_type, oid, privilege_hex}
     else
         _priv:delete{uid, object_type, oid}
     end
+
 end
 
 local function drop(uid, opts)
@@ -2194,7 +2200,8 @@ local function drop(uid, opts)
     for k, tuple in pairs(privs) do
 	-- we need an additional box.session.su() here, because of
 	-- unnecessary check for privilege PRIV_REVOKE in priv_def_check()
-        box.session.su("admin", revoke, uid, uid, tuple[5], tuple[3], tuple[4])
+        local oid = tuple[4] ~= '*' and tuple[4] or nil
+        box.session.su("admin", revoke, uid, uid, tuple[5], tuple[3], oid)
     end
     box.space[box.schema.USER_ID]:delete{uid}
 end
diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index 0293f6ef8..4acf20152 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -964,6 +964,42 @@ local function upgrade_to_1_10_0()
     create_vsequence_space()
 end
 
+--------------------------------------------------------------------------------
+--- Tarantool 1.10.1
+--------------------------------------------------------------------------------
+local function upgrade_space_priv_to_1_10_1()
+    local _priv = box.space._priv
+    local _vpriv = box.space._vpriv
+    local f = _priv:format()
+
+    f[4].type = 'scalar'
+    _priv:format(f)
+    f = _vpriv:format()
+    f[4].type = 'scalar'
+    _vpriv:format(f)
+    _priv.index.primary:alter{parts={2, 'unsigned', 3, 'string', 4, 'scalar'}}
+    _vpriv.index.primary:alter{parts={2, 'unsigned', 3, 'string', 4, 'scalar'}}
+    _priv.index.object:alter{parts={3, 'string', 4, 'scalar'}}
+    _vpriv.index.object:alter{parts={3, 'string', 4, 'scalar'}}
+end
+
+local function upgrade_privs_to_1_10_1()
+    local _priv = box.space._priv
+
+    for _, priv in _priv:pairs() do
+        if priv[4] == 0 then
+            if priv[3] ~= 'universe' and priv[3] ~= 'user' and priv[3] ~= 'role' then
+                _priv:delete{priv[2], priv[3], priv[4]}
+		_priv:insert{priv[1], priv[2], priv[3], '*', priv[5]}
+            end
+        end
+    end
+end
+
+local function upgrade_to_1_10_1()
+    upgrade_space_priv_to_1_10_1()
+    upgrade_privs_to_1_10_1()
+end
 
 local function get_version()
     local version = box.space._schema:get{'version'}
@@ -991,6 +1027,7 @@ local function upgrade(options)
         {version = mkversion(1, 7, 6), func = upgrade_to_1_7_6, auto = false},
         {version = mkversion(1, 7, 7), func = upgrade_to_1_7_7, auto = true},
         {version = mkversion(1, 10, 0), func = upgrade_to_1_10_0, auto = true},
+        {version = mkversion(1, 10, 1), func = upgrade_to_1_10_1, auto = true},
     }
 
     for _, handler in ipairs(handlers) do
diff --git a/src/box/schema.cc b/src/box/schema.cc
index 433f52c08..02f55aaad 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -536,8 +536,26 @@ schema_find_name(enum schema_object_type type, uint32_t object_id)
 	switch (type) {
 	case SC_UNIVERSE:
 		return "";
+	case SC_ENTITY_SPACE:
+		return "SPACE";
+	case SC_ENTITY_FUNCTION:
+		return "FUNCTION";
+	case SC_ENTITY_SEQUENCE:
+		return "SEQUENCE";
+	case SC_ENTITY_ROLE:
+		return "ROLE";
+	case SC_ENTITY_USER:
+		return "USER";
 	case SC_SPACE:
 		{
+			/*
+			 * Even though we have a separate type
+			 * for grants on entire entity, we have to
+			 * leave the check for an empty object_id in place,
+			 * because without it recover from an old version WAL 
+			 * fails even before the upgrade script is being run.
+			 * Same applies to SC_FUNCTION and SC_SEQUENCE below.
+			 */
 			if (object_id == 0)
 				return "SPACE";
 			struct space *space = space_by_id(object_id);
diff --git a/src/box/schema.h b/src/box/schema.h
index 0822262d0..f1735ff34 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -250,16 +250,19 @@ static inline
 struct access *
 entity_access_get(enum schema_object_type type)
 {
-       switch (type) {
-       case SC_SPACE:
-               return entity_access.space;
-       case SC_FUNCTION:
-               return entity_access.function;
-       case SC_SEQUENCE:
-               return entity_access.sequence;
-       default:
-               return NULL;
-       }
+	switch (type) {
+	case SC_SPACE:
+	case SC_ENTITY_SPACE:
+		return entity_access.space;
+	case SC_FUNCTION:
+	case SC_ENTITY_FUNCTION:
+		return entity_access.function;
+	case SC_SEQUENCE:
+	case SC_ENTITY_SEQUENCE:
+		return entity_access.sequence;
+	default:
+		return NULL;
+	}
 }
 
 #endif /* INCLUDES_TARANTOOL_BOX_SCHEMA_H */
diff --git a/src/box/schema_def.c b/src/box/schema_def.c
index 97c074ab2..51d418e42 100644
--- a/src/box/schema_def.c
+++ b/src/box/schema_def.c
@@ -31,16 +31,39 @@
 #include "schema_def.h"
 
 static const char *object_type_strs[] = {
-	/* [SC_UKNNOWN]         = */ "unknown",
-	/* [SC_UNIVERSE]        = */ "universe",
-	/* [SC_SPACE]           = */ "space",
-	/* [SC_FUNCTION]        = */ "function",
-	/* [SC_USER]            = */ "user",
-	/* [SC_ROLE]            = */ "role",
-	/* [SC_SEQUENCE]        = */ "sequence",
-	/* [SC_COLLATION]       = */ "collation",
+	/* [SC_UKNNOWN]		    = */ "unknown",
+	/* [SC_UNIVERSE]	    = */ "universe",
+	/* [SC_SPACE]		    = */ "space",
+	/* [SC_ENTITY_SPACE]	    = */ "all spaces",
+	/* [SC_FUNCTION]	    = */ "function",
+	/* [SC_ENTITY_FUNCTION]	    = */ "all functions",
+	/* [SC_USER]		    = */ "user",
+	/* [SC_ENTITY_USER]	    = */ "all users",
+	/* [SC_ROLE]		    = */ "role",
+	/* [SC_ENTITY_ROLE]	    = */ "all roles",
+	/* [SC_SEQUENCE]	    = */ "sequence",
+	/* [SC_ENTITY_SEQUENCE]	    = */ "all sequences",
+	/* [SC_COLLATION]	    = */ "collation",
+	/* [SC_ENTITY_COLLATION]    = */ "all collations",
 };
 
+enum schema_object_type
+schema_entity_type(enum schema_object_type type)
+{
+	switch(type) {
+	case SC_SPACE:
+	case SC_FUNCTION:
+	case SC_USER:
+	case SC_ROLE:
+	case SC_SEQUENCE:
+	case SC_COLLATION:
+		return type + 1;
+		break;
+	default:
+		unreachable();
+	}
+}
+
 enum schema_object_type
 schema_object_type(const char *name)
 {
diff --git a/src/box/schema_def.h b/src/box/schema_def.h
index 2edb8d37f..9b5bd6864 100644
--- a/src/box/schema_def.h
+++ b/src/box/schema_def.h
@@ -218,19 +218,35 @@ enum {
  *
  * Use 0 for unknown to use the same index consistently
  * even when there are more object types in the future.
+ *
+ * When adding new types please follow this rule:
+ * SC_ENTITY_NEW_OBJECT = SC_NEW_OBJECT + 1
+ * schema_entity_type() relies on this convention.
  */
 enum schema_object_type {
 	SC_UNKNOWN = 0,
 	SC_UNIVERSE = 1,
 	SC_SPACE = 2,
-	SC_FUNCTION = 3,
-	SC_USER = 4,
-	SC_ROLE = 5,
-	SC_SEQUENCE = 6,
-	SC_COLLATION = 7,
-	schema_object_type_MAX = 8
+	SC_ENTITY_SPACE = 3,
+	SC_FUNCTION = 4,
+	SC_ENTITY_FUNCTION = 5,
+	SC_USER = 6,
+	SC_ENTITY_USER = 7,
+	SC_ROLE = 8,
+	SC_ENTITY_ROLE = 9,
+	SC_SEQUENCE = 10,
+	SC_ENTITY_SEQUENCE = 11,
+	SC_COLLATION = 12,
+	SC_ENTITY_COLLATION = 13,
+	schema_object_type_MAX = 13
 };
 
+/**
+ * Given a object type, return an entity type it belongs to.
+ */
+enum schema_object_type
+schema_entity_type(enum schema_object_type type);
+
 enum schema_object_type
 schema_object_type(const char *name);
 
diff --git a/src/box/user.cc b/src/box/user.cc
index fbf06566a..eec785652 100644
--- a/src/box/user.cc
+++ b/src/box/user.cc
@@ -207,12 +207,23 @@ access_find(struct priv_def *priv)
 		access = universe.access;
 		break;
 	}
+	case SC_ENTITY_SPACE:
+	{
+		access = entity_access.space;
+		break;
+	}
+	case SC_ENTITY_FUNCTION:
+	{
+		access = entity_access.function;
+		break;
+	}
+	case SC_ENTITY_SEQUENCE:
+	{
+		access = entity_access.sequence;
+		break;
+	}
 	case SC_SPACE:
 	{
-		if (priv->object_id == 0) {
-			access = entity_access.space;
-			break;
-		}
 		struct space *space = space_by_id(priv->object_id);
 		if (space)
 			access = space->access;
@@ -220,10 +231,6 @@ access_find(struct priv_def *priv)
 	}
 	case SC_FUNCTION:
 	{
-		if (priv->object_id == 0) {
-			access = entity_access.function;
-			break;
-		}
 		struct func *func = func_by_id(priv->object_id);
 		if (func)
 			access = func->access;
@@ -231,10 +238,6 @@ access_find(struct priv_def *priv)
 	}
 	case SC_SEQUENCE:
 	{
-		if (priv->object_id == 0) {
-			access = entity_access.sequence;
-			break;
-		}
 		struct sequence *seq = sequence_by_id(priv->object_id);
 		if (seq)
 			access = seq->access;
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index 16c2027cf..1641c318e 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -5,7 +5,7 @@ box.space._schema:select{}
 ---
 - - ['cluster', '<cluster uuid>']
   - ['max_id', 511]
-  - ['version', 1, 10, 0]
+  - ['version', 1, 10, 1]
 ...
 box.space._cluster:select{}
 ---
@@ -58,10 +58,10 @@ box.space._space:select{}
         'type': 'string'}, {'name': 'auth', 'type': 'map'}]]
   - [312, 1, '_priv', 'memtx', 0, {}, [{'name': 'grantor', 'type': 'unsigned'}, {
         'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
-      {'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
+      {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
   - [313, 1, '_vpriv', 'sysview', 0, {}, [{'name': 'grantor', 'type': 'unsigned'},
       {'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
-      {'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
+      {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
   - [320, 1, '_cluster', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'uuid',
         'type': 'string'}]]
   - [330, 1, '_truncate', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'count',
@@ -104,13 +104,13 @@ box.space._index:select{}
   - [305, 1, 'owner', 'tree', {'unique': false}, [[1, 'unsigned']]]
   - [305, 2, 'name', 'tree', {'unique': true}, [[2, 'string']]]
   - [312, 0, 'primary', 'tree', {'unique': true}, [[1, 'unsigned'], [2, 'string'],
-      [3, 'unsigned']]]
+      [3, 'scalar']]]
   - [312, 1, 'owner', 'tree', {'unique': false}, [[0, 'unsigned']]]
-  - [312, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'unsigned']]]
+  - [312, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'scalar']]]
   - [313, 0, 'primary', 'tree', {'unique': true}, [[1, 'unsigned'], [2, 'string'],
-      [3, 'unsigned']]]
+      [3, 'scalar']]]
   - [313, 1, 'owner', 'tree', {'unique': false}, [[0, 'unsigned']]]
-  - [313, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'unsigned']]]
+  - [313, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'scalar']]]
   - [320, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
   - [320, 1, 'uuid', 'tree', {'unique': true}, [[1, 'string']]]
   - [330, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
diff --git a/test/box/access.result b/test/box/access.result
index f4669a4a3..687bbf579 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -504,7 +504,7 @@ box.space._priv:select{id}
 ...
 box.schema.user.grant('user', 'read', 'universe')
 ---
-- error: User 'user' already has read access on universe 'nil'
+- error: User 'user' already has read access on universe '*'
 ...
 box.space._priv:select{id}
 ---
@@ -690,7 +690,7 @@ box.schema.user.grant('guest', 'read,write,execute', 'universe')
 ...
 box.schema.user.grant('guest', 'read,write,execute', 'universe')
 ---
-- error: User 'guest' already has read,write,execute access on universe 'nil'
+- error: User 'guest' already has read,write,execute access on universe '*'
 ...
 box.schema.user.grant('guest', 'read,write,execute', 'universe', '', { if_not_exists = true })
 ---
@@ -703,7 +703,7 @@ box.schema.user.revoke('guest', 'usage,session', 'universe')
 ...
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 ---
-- error: User 'guest' does not have read,write,execute access on universe 'nil'
+- error: User 'guest' does not have read,write,execute access on universe '*'
 ...
 box.schema.user.revoke('guest', 'read,write,execute', 'universe', '', { if_exists = true })
 ---
diff --git a/test/box/access_misc.result b/test/box/access_misc.result
index 2d87fa2d5..7d6aa0a4b 100644
--- a/test/box/access_misc.result
+++ b/test/box/access_misc.result
@@ -436,7 +436,7 @@ box.schema.user.revoke('testuser', 'usage,session', 'universe')
 ...
 box.schema.user.revoke('testuser', 'read, write, execute', 'universe')
 ---
-- error: User 'testuser' does not have read, write, execute access on universe 'nil'
+- error: User 'testuser' does not have read, write, execute access on universe '*'
 ...
 box.schema.user.revoke('testuser', 'create', 'universe')
 ---
@@ -797,10 +797,10 @@ box.space._space:select()
         'type': 'string'}, {'name': 'auth', 'type': 'map'}]]
   - [312, 1, '_priv', 'memtx', 0, {}, [{'name': 'grantor', 'type': 'unsigned'}, {
         'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
-      {'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
+      {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
   - [313, 1, '_vpriv', 'sysview', 0, {}, [{'name': 'grantor', 'type': 'unsigned'},
       {'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
-      {'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
+      {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
   - [320, 1, '_cluster', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'uuid',
         'type': 'string'}]]
   - [330, 1, '_truncate', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'count',
@@ -853,7 +853,7 @@ box.schema.user.grant('tester', 'read', 'universe')
 -- error: the privilege is not granted
 box.schema.user.revoke('tester', 'create', 'universe')
 ---
-- error: User 'tester' does not have create access on universe 'nil'
+- error: User 'tester' does not have create access on universe '*'
 ...
 -- no error: if_exists clause
 box.schema.user.revoke('tester', 'create', 'universe', nil, { if_exists = true })
diff --git a/test/box/alter.result b/test/box/alter.result
index eb7014d8b..36bdb5fd3 100644
--- a/test/box/alter.result
+++ b/test/box/alter.result
@@ -214,13 +214,13 @@ _index:select{}
   - [305, 1, 'owner', 'tree', {'unique': false}, [[1, 'unsigned']]]
   - [305, 2, 'name', 'tree', {'unique': true}, [[2, 'string']]]
   - [312, 0, 'primary', 'tree', {'unique': true}, [[1, 'unsigned'], [2, 'string'],
-      [3, 'unsigned']]]
+      [3, 'scalar']]]
   - [312, 1, 'owner', 'tree', {'unique': false}, [[0, 'unsigned']]]
-  - [312, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'unsigned']]]
+  - [312, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'scalar']]]
   - [313, 0, 'primary', 'tree', {'unique': true}, [[1, 'unsigned'], [2, 'string'],
-      [3, 'unsigned']]]
+      [3, 'scalar']]]
   - [313, 1, 'owner', 'tree', {'unique': false}, [[0, 'unsigned']]]
-  - [313, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'unsigned']]]
+  - [313, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'scalar']]]
   - [320, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
   - [320, 1, 'uuid', 'tree', {'unique': true}, [[1, 'string']]]
   - [330, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
diff --git a/test/xlog/upgrade.result b/test/xlog/upgrade.result
index f02996bba..8f5b4ad26 100644
--- a/test/xlog/upgrade.result
+++ b/test/xlog/upgrade.result
@@ -36,7 +36,7 @@ box.space._schema:select()
 ---
 - - ['cluster', '<server_uuid>']
   - ['max_id', 513]
-  - ['version', 1, 10, 0]
+  - ['version', 1, 10, 1]
 ...
 box.space._space:select()
 ---
@@ -85,10 +85,10 @@ box.space._space:select()
         'type': 'string'}, {'name': 'auth', 'type': 'map'}]]
   - [312, 1, '_priv', 'memtx', 0, {}, [{'name': 'grantor', 'type': 'unsigned'}, {
         'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
-      {'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
+      {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
   - [313, 1, '_vpriv', 'sysview', 0, {}, [{'name': 'grantor', 'type': 'unsigned'},
       {'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
-      {'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
+      {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
   - [320, 1, '_cluster', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'uuid',
         'type': 'string'}]]
   - [330, 1, '_truncate', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'count',
@@ -134,13 +134,13 @@ box.space._index:select()
   - [305, 1, 'owner', 'tree', {'unique': false}, [[1, 'unsigned']]]
   - [305, 2, 'name', 'tree', {'unique': true}, [[2, 'string']]]
   - [312, 0, 'primary', 'tree', {'unique': true}, [[1, 'unsigned'], [2, 'string'],
-      [3, 'unsigned']]]
+      [3, 'scalar']]]
   - [312, 1, 'owner', 'tree', {'unique': false}, [[0, 'unsigned']]]
-  - [312, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'unsigned']]]
+  - [312, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'scalar']]]
   - [313, 0, 'primary', 'tree', {'unique': true}, [[1, 'unsigned'], [2, 'string'],
-      [3, 'unsigned']]]
+      [3, 'scalar']]]
   - [313, 1, 'owner', 'tree', {'unique': false}, [[0, 'unsigned']]]
-  - [313, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'unsigned']]]
+  - [313, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'scalar']]]
   - [320, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
   - [320, 1, 'uuid', 'tree', {'unique': true}, [[1, 'string']]]
   - [330, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
-- 
2.15.2 (Apple Git-101.1)

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [tarantool-patches] [PATCH v2] Introduce separate entity object types for entity privileges.
  2018-08-02 10:55 [tarantool-patches] [PATCH v2] Introduce separate entity object types for entity privileges Serge Petrenko
@ 2018-08-07 16:38 ` Vladimir Davydov
  2018-08-14 13:41   ` Serge Petrenko
  0 siblings, 1 reply; 6+ messages in thread
From: Vladimir Davydov @ 2018-08-07 16:38 UTC (permalink / raw)
  To: Serge Petrenko; +Cc: tarantool-patches, kostja

On Thu, Aug 02, 2018 at 01:55:58PM +0300, Serge Petrenko wrote:
> When granting or revoking a privilege on an entire entity, id 0 was used
> to indicate the fact that we don't grant a privilege on a single object,
> but on a whole entity. This caused confusion, because for entity USER,
> for example, id 0 is a valid object id (user 'guest' uses it).
> Any non-zero id dedicated to this cause obviously may be confused as well.
> Fix this by creating separate schema_object_types for entities:
> SC_ENTITY_SPACE, SC_ENTITY_USER, etc.
> 
> Closes: #3574
> Prerequisite: #3524
> ---
> https://github.com/tarantool/tarantool/tree/sergepetrenko/gh-3574-whole-entity-types
> https://github.com/tarantool/tarantool/issues/3574
> 
> Changes in v2: 
>   - keep only old syntax for granting access
>     to all entities
>   - add an upgrade script to alter indices of spaces
>     _priv and _vpriv to store 'scalar' in object_id field,
>     and use an asterisk ('*') in object_id to indicate
>     granting on an entire entity.

Hmm, I wonder what happens if someone creates an object (say space) and
names it "*" ...

>   - keep the new entity types in priv_def and use them
>     internally.
> 
>  src/box/alter.cc             |  27 ++++++++++++++++++++++++++-
>  src/box/bootstrap.snap       | Bin 1540 -> 1557 bytes
>  src/box/lua/schema.lua       |  41 ++++++++++++++++++++++++-----------------
>  src/box/lua/upgrade.lua      |  37 +++++++++++++++++++++++++++++++++++++
>  src/box/schema.cc            |  18 ++++++++++++++++++
>  src/box/schema.h             |  23 +++++++++++++----------
>  src/box/schema_def.c         |  39 +++++++++++++++++++++++++++++++--------
>  src/box/schema_def.h         |  28 ++++++++++++++++++++++------
>  src/box/user.cc              |  27 +++++++++++++++------------
>  test/box-py/bootstrap.result |  14 +++++++-------
>  test/box/access.result       |   6 +++---
>  test/box/access_misc.result  |   8 ++++----
>  test/box/alter.result        |   8 ++++----
>  test/xlog/upgrade.result     |  14 +++++++-------
>  14 files changed, 211 insertions(+), 79 deletions(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 3007a131d..6057b66c9 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -2529,6 +2529,31 @@ on_replace_dd_collation(struct trigger * /* trigger */, void *event)
>  	}
>  }
>  
> +void
> +priv_def_try_fill(struct priv_def *priv, struct tuple *tuple)

This function should be static. Anyway, I don't think it's worth
factoring this code out, because it's pretty straightforward and
executed only in priv_def_create_form_tuple().

> +{
> +	const char *object_id = tuple_field(tuple, BOX_PRIV_FIELD_OBJECT_ID);
> +	if (object_id == NULL) {
> +		tnt_raise(ClientError, ER_NO_SUCH_FIELD,
> +			  BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE);
> +	}

Please use tuple_field_u32() and tuple_field_str() instead.

> +	/*
> +	 * When granting or revoking privileges on a whole entity we
> +	 * pass an asterisk ('*') to object_id to indicate grant on every
> +	 * object of that entity. So check for that first.
> +	 */
> +	if (mp_typeof(*object_id) == MP_STR) {

Shouldn't you check that the string is actually "*"?

> +		priv->object_id = 0;
> +		priv->object_type = schema_entity_type(priv->object_type);

This assumes that priv->object_type is initialized before
priv_def_try_fill(). IMO this makes the function protocol
rather abstruse. Another reason not to factor it out.

> +	} else if (mp_typeof(*object_id) == MP_UINT) {
> +		priv->object_id = mp_decode_uint(&object_id);
> +	} else {
> +		tnt_raise(ClientError, ER_FIELD_TYPE,
> +			  BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE,
> +			  field_type_strs[FIELD_TYPE_UNSIGNED]);
> +	}
> +}
> +
>  /**
>   * Create a privilege definition from tuple.
>   */
> @@ -2539,8 +2564,8 @@ priv_def_create_from_tuple(struct priv_def *priv, struct tuple *tuple)
>  	priv->grantee_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_UID);
>  	const char *object_type =
>  		tuple_field_cstr_xc(tuple, BOX_PRIV_FIELD_OBJECT_TYPE);
> -	priv->object_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_OBJECT_ID);
>  	priv->object_type = schema_object_type(object_type);
> +	priv_def_try_fill(priv, tuple);
>  	if (priv->object_type == SC_UNKNOWN) {
>  		tnt_raise(ClientError, ER_UNKNOWN_SCHEMA_OBJECT,
>  			  object_type);
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index b9b8c9004..294dcf4d3 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -1809,9 +1809,9 @@ local function object_resolve(object_type, object_name)
>          return 0
>      end
>      if object_type == 'space' then
> -        if object_name == nil or object_name == 0 then
> -            return 0
> -        end
> +	if object_name == nil then
> +	    return '*'
> +	end

Bad indentation - we use spaces in Lua code, not tabs.

>          local space = box.space[object_name]
>          if  space == nil then
>              box.error(box.error.NO_SUCH_SPACE, object_name)
> @@ -1819,9 +1819,9 @@ local function object_resolve(object_type, object_name)
>          return space.id
>      end
>      if object_type == 'function' then
> -        if object_name == nil or object_name == 0 then
> -            return 0
> -        end
> +	if object_name == nil then
> +	    return '*'
> +	end

Ditto.

>          local _vfunc = box.space[box.schema.VFUNC_ID]
>          local func
>          if type(object_name) == 'string' then
> @@ -1836,8 +1836,8 @@ local function object_resolve(object_type, object_name)
>          end
>      end
>      if object_type == 'sequence' then
> -        if object_name == nil or object_name == 0 then
> -            return 0
> +        if object_name == nil then
> +            return '*'
>          end
>          local seq = sequence_resolve(object_name)
>          if seq == nil then
> @@ -1846,6 +1846,9 @@ local function object_resolve(object_type, object_name)
>          return seq
>      end
>      if object_type == 'role' then
> +	if object_name == nil then
> +	    return '*'
> +	end

Ditto.

>          local _vuser = box.space[box.schema.VUSER_ID]
>          local role
>          if type(object_name) == 'string' then
> @@ -1867,6 +1870,9 @@ local function object_name(object_type, object_id)
>      if object_type == 'universe' then
>          return ""
>      end
> +    if object_id == '*' then
> +        return '*'
> +    end
>      local space
>      if object_type == 'space' then
>          space = box.space._vspace
> @@ -2079,9 +2085,8 @@ local function grant(uid, name, privilege, object_type,
>          object_name = privilege
>          privilege = 'execute'
>      end
> -    local privilege_hex = privilege_check(privilege, object_type)
> -
>      local oid = object_resolve(object_type, object_name)
> +    local privilege_hex = privilege_check(privilege, object_type)

I don't understand why you need this.

>      options = options or {}
>      if options.grantor == nil then
>          options.grantor = session.euid()
> @@ -2106,10 +2111,10 @@ local function grant(uid, name, privilege, object_type,
>          _priv:replace{options.grantor, uid, object_type, oid, privilege_hex}
>      elseif not options.if_not_exists then
>              if object_type == 'role' then
> -                box.error(box.error.ROLE_GRANTED, name, object_name)
> +                box.error(box.error.ROLE_GRANTED, name, object_name or '*')
>              else
>                  box.error(box.error.PRIV_GRANTED, name, privilege,
> -                          object_type, object_name)
> +                          object_type, object_name or '*')

Instead you could set object_name to '*' in the beginning of
grant/revoke functions. This would make your patch shorter,
because you wouldn't have to do it here, nor in object_resolve.

>              end
>      end
>  end
> @@ -2122,9 +2127,9 @@ local function revoke(uid, name, privilege, object_type, object_name, options)
>          object_name = privilege
>          privilege = 'execute'
>      end
> -    local privilege_hex = privilege_check(privilege, object_type)
>      options = options or {}
>      local oid = object_resolve(object_type, object_name)
> +    local privilege_hex = privilege_check(privilege, object_type)

I don't understand why you need this.

>      local _priv = box.space[box.schema.PRIV_ID]
>      local _vpriv = box.space[box.schema.VPRIV_ID]
>      local tuple = _vpriv:get{uid, object_type, oid}
> @@ -2134,10 +2139,10 @@ local function revoke(uid, name, privilege, object_type, object_name, options)
>              return
>          end
>          if object_type == 'role' then
> -            box.error(box.error.ROLE_NOT_GRANTED, name, object_name)
> +            box.error(box.error.ROLE_NOT_GRANTED, name, object_name or '*')
>          else
>              box.error(box.error.PRIV_NOT_GRANTED, name, privilege,
> -                      object_type, object_name)
> +                      object_type, object_name or '*')
>          end
>      end
>      local old_privilege = tuple[5]
> @@ -2153,13 +2158,14 @@ local function revoke(uid, name, privilege, object_type, object_name, options)
>              return
>          end
>          box.error(box.error.PRIV_NOT_GRANTED, name, privilege,
> -                  object_type, object_name)
> +                  object_type, object_name or '*')

This hunk as well as the one above wouldn't be needed if you set
object_name to '*' in the beginning of the function (also, see my
comment to grant function).

>      end
>      if privilege_hex ~= 0 then
>          _priv:replace{grantor, uid, object_type, oid, privilege_hex}
>      else
>          _priv:delete{uid, object_type, oid}
>      end
> +

Extraneous change. Please remove.

>  end
>  
>  local function drop(uid, opts)
> @@ -2194,7 +2200,8 @@ local function drop(uid, opts)
>      for k, tuple in pairs(privs) do
>  	-- we need an additional box.session.su() here, because of
>  	-- unnecessary check for privilege PRIV_REVOKE in priv_def_check()
> -        box.session.su("admin", revoke, uid, uid, tuple[5], tuple[3], tuple[4])
> +        local oid = tuple[4] ~= '*' and tuple[4] or nil
> +        box.session.su("admin", revoke, uid, uid, tuple[5], tuple[3], oid)

Why? The 'revoke' function should be able to interpret '*' correctly.

>      end
>      box.space[box.schema.USER_ID]:delete{uid}
>  end
> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index 0293f6ef8..4acf20152 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -964,6 +964,42 @@ local function upgrade_to_1_10_0()
>      create_vsequence_space()
>  end
>  
> +--------------------------------------------------------------------------------
> +--- Tarantool 1.10.1
> +--------------------------------------------------------------------------------
> +local function upgrade_space_priv_to_1_10_1()
> +    local _priv = box.space._priv
> +    local _vpriv = box.space._vpriv
> +    local f = _priv:format()
> +
> +    f[4].type = 'scalar'
> +    _priv:format(f)
> +    f = _vpriv:format()
> +    f[4].type = 'scalar'
> +    _vpriv:format(f)
> +    _priv.index.primary:alter{parts={2, 'unsigned', 3, 'string', 4, 'scalar'}}
> +    _vpriv.index.primary:alter{parts={2, 'unsigned', 3, 'string', 4, 'scalar'}}
> +    _priv.index.object:alter{parts={3, 'string', 4, 'scalar'}}
> +    _vpriv.index.object:alter{parts={3, 'string', 4, 'scalar'}}
> +end
> +
> +local function upgrade_privs_to_1_10_1()

No need to split this code in two functions.

> +    local _priv = box.space._priv
> +
> +    for _, priv in _priv:pairs() do

AFAICS interpretation of '0' as 'all objects of a kind' was added after
1.10.1 i.e. there was no Tarantool release that would use this feature.
So I guess you don't need to upgrade existing privileges, neither do you
need to preserve any code that handles '0' object id. Please consult
with Georgy and Kostja regarding this.

> +        if priv[4] == 0 then
> +            if priv[3] ~= 'universe' and priv[3] ~= 'user' and priv[3] ~= 'role' then

Please rewrite it as

  if priv[4] == 0 and priv[3] ~= 'universe' and ... then

> +                _priv:delete{priv[2], priv[3], priv[4]}
> +		_priv:insert{priv[1], priv[2], priv[3], '*', priv[5]}

Bad indentation - we use spaces in Lua code, not tabs.

> +            end
> +        end
> +    end
> +end
> +
> +local function upgrade_to_1_10_1()
> +    upgrade_space_priv_to_1_10_1()
> +    upgrade_privs_to_1_10_1()
> +end
>  
>  local function get_version()
>      local version = box.space._schema:get{'version'}
> @@ -991,6 +1027,7 @@ local function upgrade(options)
>          {version = mkversion(1, 7, 6), func = upgrade_to_1_7_6, auto = false},
>          {version = mkversion(1, 7, 7), func = upgrade_to_1_7_7, auto = true},
>          {version = mkversion(1, 10, 0), func = upgrade_to_1_10_0, auto = true},
> +        {version = mkversion(1, 10, 1), func = upgrade_to_1_10_1, auto = true},
>      }
>  
>      for _, handler in ipairs(handlers) do
> diff --git a/src/box/schema.cc b/src/box/schema.cc
> index 433f52c08..02f55aaad 100644
> --- a/src/box/schema.cc
> +++ b/src/box/schema.cc
> @@ -536,8 +536,26 @@ schema_find_name(enum schema_object_type type, uint32_t object_id)
>  	switch (type) {
>  	case SC_UNIVERSE:
>  		return "";
> +	case SC_ENTITY_SPACE:
> +		return "SPACE";
> +	case SC_ENTITY_FUNCTION:
> +		return "FUNCTION";
> +	case SC_ENTITY_SEQUENCE:
> +		return "SEQUENCE";
> +	case SC_ENTITY_ROLE:
> +		return "ROLE";
> +	case SC_ENTITY_USER:
> +		return "USER";

Shouldn't it be "*"?

>  	case SC_SPACE:
>  		{
> +			/*
> +			 * Even though we have a separate type
> +			 * for grants on entire entity, we have to
> +			 * leave the check for an empty object_id in place,
> +			 * because without it recover from an old version WAL 
> +			 * fails even before the upgrade script is being run.
> +			 * Same applies to SC_FUNCTION and SC_SEQUENCE below.
> +			 */
>  			if (object_id == 0)
>  				return "SPACE";
>  			struct space *space = space_by_id(object_id);
> diff --git a/src/box/schema.h b/src/box/schema.h
> index 0822262d0..f1735ff34 100644
> --- a/src/box/schema.h
> +++ b/src/box/schema.h
> @@ -250,16 +250,19 @@ static inline
>  struct access *
>  entity_access_get(enum schema_object_type type)
>  {
> -       switch (type) {
> -       case SC_SPACE:
> -               return entity_access.space;
> -       case SC_FUNCTION:
> -               return entity_access.function;
> -       case SC_SEQUENCE:
> -               return entity_access.sequence;
> -       default:
> -               return NULL;
> -       }
> +	switch (type) {
> +	case SC_SPACE:
> +	case SC_ENTITY_SPACE:
> +		return entity_access.space;
> +	case SC_FUNCTION:
> +	case SC_ENTITY_FUNCTION:
> +		return entity_access.function;
> +	case SC_SEQUENCE:
> +	case SC_ENTITY_SEQUENCE:
> +		return entity_access.sequence;
> +	default:
> +		return NULL;
> +	}
>  }
>  
>  #endif /* INCLUDES_TARANTOOL_BOX_SCHEMA_H */
> diff --git a/src/box/schema_def.c b/src/box/schema_def.c
> index 97c074ab2..51d418e42 100644
> --- a/src/box/schema_def.c
> +++ b/src/box/schema_def.c
> @@ -31,16 +31,39 @@
>  #include "schema_def.h"
>  
>  static const char *object_type_strs[] = {
> -	/* [SC_UKNNOWN]         = */ "unknown",
> -	/* [SC_UNIVERSE]        = */ "universe",
> -	/* [SC_SPACE]           = */ "space",
> -	/* [SC_FUNCTION]        = */ "function",
> -	/* [SC_USER]            = */ "user",
> -	/* [SC_ROLE]            = */ "role",
> -	/* [SC_SEQUENCE]        = */ "sequence",
> -	/* [SC_COLLATION]       = */ "collation",
> +	/* [SC_UKNNOWN]		    = */ "unknown",
> +	/* [SC_UNIVERSE]	    = */ "universe",
> +	/* [SC_SPACE]		    = */ "space",
> +	/* [SC_ENTITY_SPACE]	    = */ "all spaces",
> +	/* [SC_FUNCTION]	    = */ "function",
> +	/* [SC_ENTITY_FUNCTION]	    = */ "all functions",
> +	/* [SC_USER]		    = */ "user",
> +	/* [SC_ENTITY_USER]	    = */ "all users",
> +	/* [SC_ROLE]		    = */ "role",
> +	/* [SC_ENTITY_ROLE]	    = */ "all roles",
> +	/* [SC_SEQUENCE]	    = */ "sequence",
> +	/* [SC_ENTITY_SEQUENCE]	    = */ "all sequences",
> +	/* [SC_COLLATION]	    = */ "collation",
> +	/* [SC_ENTITY_COLLATION]    = */ "all collations",

Why? I don't think that you need to report "all spaces" anywhere.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [tarantool-patches] [PATCH v2] Introduce separate entity object types for entity privileges.
  2018-08-07 16:38 ` Vladimir Davydov
@ 2018-08-14 13:41   ` Serge Petrenko
  2018-08-17  9:19     ` Vladimir Davydov
  0 siblings, 1 reply; 6+ messages in thread
From: Serge Petrenko @ 2018-08-14 13:41 UTC (permalink / raw)
  To: Vladimir Davydov; +Cc: tarantool-patches, kostja

[-- Attachment #1: Type: text/plain, Size: 47413 bytes --]

Hi! Thankyou for review!

Please see my answers regarding extra code in grant/revoke and object_resolve
functions below.
The new diff is also below.

> 7 авг. 2018 г., в 19:38, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а):
> 
> On Thu, Aug 02, 2018 at 01:55:58PM +0300, Serge Petrenko wrote:
>> When granting or revoking a privilege on an entire entity, id 0 was used
>> to indicate the fact that we don't grant a privilege on a single object,
>> but on a whole entity. This caused confusion, because for entity USER,
>> for example, id 0 is a valid object id (user 'guest' uses it).
>> Any non-zero id dedicated to this cause obviously may be confused as well.
>> Fix this by creating separate schema_object_types for entities:
>> SC_ENTITY_SPACE, SC_ENTITY_USER, etc.
>> 
>> Closes: #3574
>> Prerequisite: #3524
>> ---
>> https://github.com/tarantool/tarantool/tree/sergepetrenko/gh-3574-whole-entity-types
>> https://github.com/tarantool/tarantool/issues/3574
>> 
>> Changes in v2: 
>>  - keep only old syntax for granting access
>>    to all entities
>>  - add an upgrade script to alter indices of spaces
>>    _priv and _vpriv to store 'scalar' in object_id field,
>>    and use an asterisk ('*') in object_id to indicate
>>    granting on an entire entity.
> 
> Hmm, I wonder what happens if someone creates an object (say space) and
> names it "*» …
It is ok, when operating with a space named ‘*’, ‘*’ is in object_name, when operating
on a whole entity, ‘*’ is set internally in object_id field. So there are no conflicts here.
> 
>>  - keep the new entity types in priv_def and use them
>>    internally.
>> 
>> src/box/alter.cc             |  27 ++++++++++++++++++++++++++-
>> src/box/bootstrap.snap       | Bin 1540 -> 1557 bytes
>> src/box/lua/schema.lua       |  41 ++++++++++++++++++++++++-----------------
>> src/box/lua/upgrade.lua      |  37 +++++++++++++++++++++++++++++++++++++
>> src/box/schema.cc            |  18 ++++++++++++++++++
>> src/box/schema.h             |  23 +++++++++++++----------
>> src/box/schema_def.c         |  39 +++++++++++++++++++++++++++++++--------
>> src/box/schema_def.h         |  28 ++++++++++++++++++++++------
>> src/box/user.cc              |  27 +++++++++++++++------------
>> test/box-py/bootstrap.result |  14 +++++++-------
>> test/box/access.result       |   6 +++---
>> test/box/access_misc.result  |   8 ++++----
>> test/box/alter.result        |   8 ++++----
>> test/xlog/upgrade.result     |  14 +++++++-------
>> 14 files changed, 211 insertions(+), 79 deletions(-)
>> 
>> diff --git a/src/box/alter.cc b/src/box/alter.cc
>> index 3007a131d..6057b66c9 100644
>> --- a/src/box/alter.cc
>> +++ b/src/box/alter.cc
>> @@ -2529,6 +2529,31 @@ on_replace_dd_collation(struct trigger * /* trigger */, void *event)
>> 	}
>> }
>> 
>> +void
>> +priv_def_try_fill(struct priv_def *priv, struct tuple *tuple)
> 
> This function should be static. Anyway, I don't think it's worth
> factoring this code out, because it's pretty straightforward and
> executed only in priv_def_create_form_tuple().
> 
I removed the function.
>> +{
>> +	const char *object_id = tuple_field(tuple, BOX_PRIV_FIELD_OBJECT_ID);
>> +	if (object_id == NULL) {
>> +		tnt_raise(ClientError, ER_NO_SUCH_FIELD,
>> +			  BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE);
>> +	}
> 
> Please use tuple_field_u32() and tuple_field_str() instead.
> 
>> +	/*
>> +	 * When granting or revoking privileges on a whole entity we
>> +	 * pass an asterisk ('*') to object_id to indicate grant on every
>> +	 * object of that entity. So check for that first.
>> +	 */
>> +	if (mp_typeof(*object_id) == MP_STR) {
> 
> Shouldn't you check that the string is actually "*»?

Added a check

> 
>> +		priv->object_id = 0;
>> +		priv->object_type = schema_entity_type(priv->object_type);
> 
> This assumes that priv->object_type is initialized before
> priv_def_try_fill(). IMO this makes the function protocol
> rather abstruse. Another reason not to factor it out.
> 

Done.
>> +	} else if (mp_typeof(*object_id) == MP_UINT) {
>> +		priv->object_id = mp_decode_uint(&object_id);
>> +	} else {
>> +		tnt_raise(ClientError, ER_FIELD_TYPE,
>> +			  BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE,
>> +			  field_type_strs[FIELD_TYPE_UNSIGNED]);
>> +	}
>> +}
>> +
>> /**
>>  * Create a privilege definition from tuple.
>>  */
>> @@ -2539,8 +2564,8 @@ priv_def_create_from_tuple(struct priv_def *priv, struct tuple *tuple)
>> 	priv->grantee_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_UID);
>> 	const char *object_type =
>> 		tuple_field_cstr_xc(tuple, BOX_PRIV_FIELD_OBJECT_TYPE);
>> -	priv->object_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_OBJECT_ID);
>> 	priv->object_type = schema_object_type(object_type);
>> +	priv_def_try_fill(priv, tuple);
>> 	if (priv->object_type == SC_UNKNOWN) {
>> 		tnt_raise(ClientError, ER_UNKNOWN_SCHEMA_OBJECT,
>> 			  object_type);
>> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
>> index b9b8c9004..294dcf4d3 100644
>> --- a/src/box/lua/schema.lua
>> +++ b/src/box/lua/schema.lua
>> @@ -1809,9 +1809,9 @@ local function object_resolve(object_type, object_name)
>>         return 0
>>     end
>>     if object_type == 'space' then
>> -        if object_name == nil or object_name == 0 then
>> -            return 0
>> -        end
>> +	if object_name == nil then
>> +	    return '*'
>> +	end
> 
> Bad indentation - we use spaces in Lua code, not tabs.
> 

Fixed.

>>         local space = box.space[object_name]
>>         if  space == nil then
>>             box.error(box.error.NO_SUCH_SPACE, object_name)
>> @@ -1819,9 +1819,9 @@ local function object_resolve(object_type, object_name)
>>         return space.id
>>     end
>>     if object_type == 'function' then
>> -        if object_name == nil or object_name == 0 then
>> -            return 0
>> -        end
>> +	if object_name == nil then
>> +	    return '*'
>> +	end
> 
> Ditto

Fixed

> 
>>         local _vfunc = box.space[box.schema.VFUNC_ID]
>>         local func
>>         if type(object_name) == 'string' then
>> @@ -1836,8 +1836,8 @@ local function object_resolve(object_type, object_name)
>>         end
>>     end
>>     if object_type == 'sequence' then
>> -        if object_name == nil or object_name == 0 then
>> -            return 0
>> +        if object_name == nil then
>> +            return '*'
>>         end
>>         local seq = sequence_resolve(object_name)
>>         if seq == nil then
>> @@ -1846,6 +1846,9 @@ local function object_resolve(object_type, object_name)
>>         return seq
>>     end
>>     if object_type == 'role' then
>> +	if object_name == nil then
>> +	    return '*'
>> +	end
> 
> Ditto.
> 

Fixed

>>         local _vuser = box.space[box.schema.VUSER_ID]
>>         local role
>>         if type(object_name) == 'string' then
>> @@ -1867,6 +1870,9 @@ local function object_name(object_type, object_id)
>>     if object_type == 'universe' then
>>         return ""
>>     end
>> +    if object_id == '*' then
>> +        return '*'
>> +    end
>>     local space
>>     if object_type == 'space' then
>>         space = box.space._vspace
>> @@ -2079,9 +2085,8 @@ local function grant(uid, name, privilege, object_type,
>>         object_name = privilege
>>         privilege = 'execute'
>>     end
>> -    local privilege_hex = privilege_check(privilege, object_type)
>> -
>>     local oid = object_resolve(object_type, object_name)
>> +    local privilege_hex = privilege_check(privilege, object_type)
> 
> I don't understand why you need this.

Removed, no need to do this.
> 
>>     options = options or {}
>>     if options.grantor == nil then
>>         options.grantor = session.euid()
>> @@ -2106,10 +2111,10 @@ local function grant(uid, name, privilege, object_type,
>>         _priv:replace{options.grantor, uid, object_type, oid, privilege_hex}
>>     elseif not options.if_not_exists then
>>             if object_type == 'role' then
>> -                box.error(box.error.ROLE_GRANTED, name, object_name)
>> +                box.error(box.error.ROLE_GRANTED, name, object_name or '*')
>>             else
>>                 box.error(box.error.PRIV_GRANTED, name, privilege,
>> -                          object_type, object_name)
>> +                          object_type, object_name or '*')
> 
> Instead you could set object_name to '*' in the beginning of
> grant/revoke functions. This would make your patch shorter,
> because you wouldn't have to do it here, nor in object_resolve.
> 

If I set object_name to ‘*’ the case of granting to an entity as a whole will
Be indistinguishable from granting to an object named ‘*’. So I have to handle
This case like I did. I changed the code in object_resolve a little bit to save some
Extra lines.
>>             end
>>     end
>> end
>> @@ -2122,9 +2127,9 @@ local function revoke(uid, name, privilege, object_type, object_name, options)
>>         object_name = privilege
>>         privilege = 'execute'
>>     end
>> -    local privilege_hex = privilege_check(privilege, object_type)
>>     options = options or {}
>>     local oid = object_resolve(object_type, object_name)
>> +    local privilege_hex = privilege_check(privilege, object_type)
> 
> I don't understand why you need this.

Removed.
> 
>>     local _priv = box.space[box.schema.PRIV_ID]
>>     local _vpriv = box.space[box.schema.VPRIV_ID]
>>     local tuple = _vpriv:get{uid, object_type, oid}
>> @@ -2134,10 +2139,10 @@ local function revoke(uid, name, privilege, object_type, object_name, options)
>>             return
>>         end
>>         if object_type == 'role' then
>> -            box.error(box.error.ROLE_NOT_GRANTED, name, object_name)
>> +            box.error(box.error.ROLE_NOT_GRANTED, name, object_name or '*')
>>         else
>>             box.error(box.error.PRIV_NOT_GRANTED, name, privilege,
>> -                      object_type, object_name)
>> +                      object_type, object_name or '*')
>>         end
>>     end
>>     local old_privilege = tuple[5]
>> @@ -2153,13 +2158,14 @@ local function revoke(uid, name, privilege, object_type, object_name, options)
>>             return
>>         end
>>         box.error(box.error.PRIV_NOT_GRANTED, name, privilege,
>> -                  object_type, object_name)
>> +                  object_type, object_name or '*')
> 
> This hunk as well as the one above wouldn't be needed if you set
> object_name to '*' in the beginning of the function (also, see my
> comment to grant function).

Answered above.
> 
>>     end
>>     if privilege_hex ~= 0 then
>>         _priv:replace{grantor, uid, object_type, oid, privilege_hex}
>>     else
>>         _priv:delete{uid, object_type, oid}
>>     end
>> +
> 
> Extraneous change. Please remove.

Done

> 
>> end
>> 
>> local function drop(uid, opts)
>> @@ -2194,7 +2200,8 @@ local function drop(uid, opts)
>>     for k, tuple in pairs(privs) do
>> 	-- we need an additional box.session.su() here, because of
>> 	-- unnecessary check for privilege PRIV_REVOKE in priv_def_check()
>> -        box.session.su("admin", revoke, uid, uid, tuple[5], tuple[3], tuple[4])
>> +        local oid = tuple[4] ~= '*' and tuple[4] or nil
>> +        box.session.su("admin", revoke, uid, uid, tuple[5], tuple[3], oid)
> 
> Why? The 'revoke' function should be able to interpret '*' correctly.
> 

No, answered above.
>>     end
>>     box.space[box.schema.USER_ID]:delete{uid}
>> end
>> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
>> index 0293f6ef8..4acf20152 100644
>> --- a/src/box/lua/upgrade.lua
>> +++ b/src/box/lua/upgrade.lua
>> @@ -964,6 +964,42 @@ local function upgrade_to_1_10_0()
>>     create_vsequence_space()
>> end
>> 
>> +--------------------------------------------------------------------------------
>> +--- Tarantool 1.10.1
>> +--------------------------------------------------------------------------------
>> +local function upgrade_space_priv_to_1_10_1()
>> +    local _priv = box.space._priv
>> +    local _vpriv = box.space._vpriv
>> +    local f = _priv:format()
>> +
>> +    f[4].type = 'scalar'
>> +    _priv:format(f)
>> +    f = _vpriv:format()
>> +    f[4].type = 'scalar'
>> +    _vpriv:format(f)
>> +    _priv.index.primary:alter{parts={2, 'unsigned', 3, 'string', 4, 'scalar'}}
>> +    _vpriv.index.primary:alter{parts={2, 'unsigned', 3, 'string', 4, 'scalar'}}
>> +    _priv.index.object:alter{parts={3, 'string', 4, 'scalar'}}
>> +    _vpriv.index.object:alter{parts={3, 'string', 4, 'scalar'}}
>> +end
>> +
>> +local function upgrade_privs_to_1_10_1()
> 
> No need to split this code in two functions.
> 
>> +    local _priv = box.space._priv
>> +
>> +    for _, priv in _priv:pairs() do
> 
> AFAICS interpretation of '0' as 'all objects of a kind' was added after
> 1.10.1 i.e. there was no Tarantool release that would use this feature.
> So I guess you don't need to upgrade existing privileges, neither do you
> need to preserve any code that handles '0' object id. Please consult
> with Georgy and Kostja regarding this.
> 
>> +        if priv[4] == 0 then
>> +            if priv[3] ~= 'universe' and priv[3] ~= 'user' and priv[3] ~= 'role' then
> 
> Please rewrite it as
> 
>  if priv[4] == 0 and priv[3] ~= 'universe' and ... then
> 
>> +                _priv:delete{priv[2], priv[3], priv[4]}
>> +		_priv:insert{priv[1], priv[2], priv[3], '*', priv[5]}
> 
> Bad indentation - we use spaces in Lua code, not tabs.
> 

Removed the privilege upgrade function together with some leftover code handling 
Id 0.
>> +            end
>> +        end
>> +    end
>> +end
>> +
>> +local function upgrade_to_1_10_1()
>> +    upgrade_space_priv_to_1_10_1()
>> +    upgrade_privs_to_1_10_1()
>> +end
>> 
>> local function get_version()
>>     local version = box.space._schema:get{'version'}
>> @@ -991,6 +1027,7 @@ local function upgrade(options)
>>         {version = mkversion(1, 7, 6), func = upgrade_to_1_7_6, auto = false},
>>         {version = mkversion(1, 7, 7), func = upgrade_to_1_7_7, auto = true},
>>         {version = mkversion(1, 10, 0), func = upgrade_to_1_10_0, auto = true},
>> +        {version = mkversion(1, 10, 1), func = upgrade_to_1_10_1, auto = true},
>>     }
>> 
>>     for _, handler in ipairs(handlers) do
>> diff --git a/src/box/schema.cc b/src/box/schema.cc
>> index 433f52c08..02f55aaad 100644
>> --- a/src/box/schema.cc
>> +++ b/src/box/schema.cc
>> @@ -536,8 +536,26 @@ schema_find_name(enum schema_object_type type, uint32_t object_id)
>> 	switch (type) {
>> 	case SC_UNIVERSE:
>> 		return "";
>> +	case SC_ENTITY_SPACE:
>> +		return "SPACE";
>> +	case SC_ENTITY_FUNCTION:
>> +		return "FUNCTION";
>> +	case SC_ENTITY_SEQUENCE:
>> +		return "SEQUENCE";
>> +	case SC_ENTITY_ROLE:
>> +		return "ROLE";
>> +	case SC_ENTITY_USER:
>> +		return "USER";
> 
> Shouldn't it be "*»?
It should, indeed. Fixed.

>> 	case SC_SPACE:
>> 		{
>> +			/*
>> +			 * Even though we have a separate type
>> +			 * for grants on entire entity, we have to
>> +			 * leave the check for an empty object_id in place,
>> +			 * because without it recover from an old version WAL 
>> +			 * fails even before the upgrade script is being run.
>> +			 * Same applies to SC_FUNCTION and SC_SEQUENCE below.
>> +			 */
>> 			if (object_id == 0)
>> 				return "SPACE";
>> 			struct space *space = space_by_id(object_id);
>> diff --git a/src/box/schema.h b/src/box/schema.h
>> index 0822262d0..f1735ff34 100644
>> --- a/src/box/schema.h
>> +++ b/src/box/schema.h
>> @@ -250,16 +250,19 @@ static inline
>> struct access *
>> entity_access_get(enum schema_object_type type)
>> {
>> -       switch (type) {
>> -       case SC_SPACE:
>> -               return entity_access.space;
>> -       case SC_FUNCTION:
>> -               return entity_access.function;
>> -       case SC_SEQUENCE:
>> -               return entity_access.sequence;
>> -       default:
>> -               return NULL;
>> -       }
>> +	switch (type) {
>> +	case SC_SPACE:
>> +	case SC_ENTITY_SPACE:
>> +		return entity_access.space;
>> +	case SC_FUNCTION:
>> +	case SC_ENTITY_FUNCTION:
>> +		return entity_access.function;
>> +	case SC_SEQUENCE:
>> +	case SC_ENTITY_SEQUENCE:
>> +		return entity_access.sequence;
>> +	default:
>> +		return NULL;
>> +	}
>> }
>> 
>> #endif /* INCLUDES_TARANTOOL_BOX_SCHEMA_H */
>> diff --git a/src/box/schema_def.c b/src/box/schema_def.c
>> index 97c074ab2..51d418e42 100644
>> --- a/src/box/schema_def.c
>> +++ b/src/box/schema_def.c
>> @@ -31,16 +31,39 @@
>> #include "schema_def.h"
>> 
>> static const char *object_type_strs[] = {
>> -	/* [SC_UKNNOWN]         = */ "unknown",
>> -	/* [SC_UNIVERSE]        = */ "universe",
>> -	/* [SC_SPACE]           = */ "space",
>> -	/* [SC_FUNCTION]        = */ "function",
>> -	/* [SC_USER]            = */ "user",
>> -	/* [SC_ROLE]            = */ "role",
>> -	/* [SC_SEQUENCE]        = */ "sequence",
>> -	/* [SC_COLLATION]       = */ "collation",
>> +	/* [SC_UKNNOWN]		    = */ "unknown",
>> +	/* [SC_UNIVERSE]	    = */ "universe",
>> +	/* [SC_SPACE]		    = */ "space",
>> +	/* [SC_ENTITY_SPACE]	    = */ "all spaces",
>> +	/* [SC_FUNCTION]	    = */ "function",
>> +	/* [SC_ENTITY_FUNCTION]	    = */ "all functions",
>> +	/* [SC_USER]		    = */ "user",
>> +	/* [SC_ENTITY_USER]	    = */ "all users",
>> +	/* [SC_ROLE]		    = */ "role",
>> +	/* [SC_ENTITY_ROLE]	    = */ "all roles",
>> +	/* [SC_SEQUENCE]	    = */ "sequence",
>> +	/* [SC_ENTITY_SEQUENCE]	    = */ "all sequences",
>> +	/* [SC_COLLATION]	    = */ "collation",
>> +	/* [SC_ENTITY_COLLATION]    = */ "all collations",
> 
> Why? I don't think that you need to report "all spaces" anywhere.

fixed.

Here’s the new diff:

 src/box/alter.cc             |  33 ++++++++++++++++++++++++++++++++-
 src/box/bootstrap.snap       | Bin 1540 -> 1557 bytes
 src/box/lua/schema.lua       |  43 ++++++++++++++++++++++---------------------
 src/box/lua/upgrade.lua      |  23 +++++++++++++++++++++++
 src/box/schema.cc            |  16 ++++++++++------
 src/box/schema.h             |  23 +++++++++++++----------
 src/box/schema_def.c         |  39 +++++++++++++++++++++++++++++++--------
 src/box/schema_def.h         |  28 ++++++++++++++++++++++------
 src/box/user.cc              |  27 +++++++++++++++------------
 test/box-py/bootstrap.result |  14 +++++++-------
 test/box/access.result       |   6 +++---
 test/box/access_misc.result  |   8 ++++----
 test/box/alter.result        |   8 ++++----
 test/xlog/upgrade.result     |  14 +++++++-------
 14 files changed, 193 insertions(+), 89 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 3007a131d..7d65cab3a 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2537,10 +2537,41 @@ priv_def_create_from_tuple(struct priv_def *priv, struct tuple *tuple)
 {
 	priv->grantor_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_ID);
 	priv->grantee_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_UID);
+
 	const char *object_type =
 		tuple_field_cstr_xc(tuple, BOX_PRIV_FIELD_OBJECT_TYPE);
-	priv->object_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_OBJECT_ID);
 	priv->object_type = schema_object_type(object_type);
+
+	const char *object_id = tuple_field(tuple, BOX_PRIV_FIELD_OBJECT_ID);
+	if (object_id == NULL) {
+	    tnt_raise(ClientError, ER_NO_SUCH_FIELD,
+		      BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE);
+	}
+	/*
+	 * When granting or revoking privileges on a whole entity
+	 * we pass an asterisk ('*') to object_id to indicate
+	 * grant on every object of that entity.
+	 * So check for that first.
+	 */
+	if (mp_typeof(*object_id) == MP_STR) {
+		object_id = tuple_field_cstr_xc(tuple, BOX_PRIV_FIELD_OBJECT_ID);
+		if(*object_id == '*' && strlen(object_id) == 1) {
+			priv->object_id = 0;
+			priv->object_type = schema_entity_type(priv->object_type);
+		} else {
+			tnt_raise(ClientError, ER_FIELD_TYPE,
+				  BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE,
+				  field_type_strs[FIELD_TYPE_UNSIGNED]);
+		}
+	} else if (mp_typeof(*object_id) == MP_UINT) {
+		priv->object_id = tuple_field_u32_xc(tuple,
+						     BOX_PRIV_FIELD_OBJECT_ID);
+	} else {
+		tnt_raise(ClientError, ER_FIELD_TYPE,
+			  BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE,
+			  field_type_strs[FIELD_TYPE_UNSIGNED]);
+	}
+
 	if (priv->object_type == SC_UNKNOWN) {
 		tnt_raise(ClientError, ER_UNKNOWN_SCHEMA_OBJECT,
 			  object_type);
diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
index b610828c9c9ae9a22acdd8c150c16c6838b7a273..da6961d1be7621fe9821623c7c2d57125e8ac7bc 100644
GIT binary patch
delta 1552
zcmV+r2JiWV43!L!7=JM>GBYwQXE`%5Ib$<nWMc|RZgX^DZewLSAZ0N(H8nRmV=XXd
zF*hwVWjJFkVlgvjEihy=V`616Vl-qkHVRflY;R+0Iv{&}3JTS_3%bn(ngGty8d0aE
z0000004TLD{QyvnEC6=>*h}EFr~oj`FvARU1MnJQi4e@o&wmVLh3G~crY-}4Qt)1A
zB2#HdNhp@x{7t9y{=-^iy?G~GRT*HEED-ei@Dn^G44RYT!B-?xN&&+Fx&Y1qV@|wy
z6m_Jn@7Frx341NeT$e$x{G;bbkGWq_9B~MwrUdvJKu^0mC)0U%u6xqfb3Hyc=uW3S
z?Yy*~VIJ-(N`EEV@r?j@mYTMGX?c^oS(oYGod@w`90IW?o~7muA2}mX0M&<$IO}p(
zf4&sYv(&5wsva}nU)$;MEH#TN5iEy0m!_+prRGbuYQ<Q1#lkEYO69^Y)ISUiiUp=t
zC>0okQ!15=OOySbmm=QXK9owS5|kbsI5j<pFj+#A<bTA{p5xH}*!A7AT`g}iJZp;q
zZa*(m3p`8BSuZbcKQF_xf+#w$E@QP&S5FE&OU<%B*MW5z3(smw@be+qmpn7j&(6bi
z4t2HV0FPP-q?QbLmYSt_Yv-+BSSX~f6!0uH^?h+1Um~{s3}1ZqeLFvQuFf=|{j%>m
z@5t{w$A3eAY2$e)R8<J*e6DdD-_^ohB@MWH-}BP;7}LqF7(2TzM;yY$&#@t$W%_kH
z9$%7&x;hfzS!$A(2E;oVI=}4a<;%;?_$pE>?lXx9syH+_Gd3aW42YQzE)T8_E)F-H
zZMkTx<ymTaw;U~*R7{GMf&o{c3QV2A(*~c@i+>4a(%@NY`j|3&7;fLG#dK@AP_j^}
zP;!~^J6oZkGC}22X(6$YN~9HQM=JH^bEJ+Wj3U%Z@+>t|M)c-`<Vzb_mpY#R-E$PB
z68(HF5MbZ$e*LnL=OE!wjv*E2foG|C>lf>C_fV+XIPfer%Uc*!XHsf;ldcAyrKaq~
zTz|`(6uRQYTqhKE{4`zxF)J9%3;Y*e!9Y}Om3pCY>tRqbsThdDE*Ta}CiN~&>g}76
z!4jk9%y>A~28QcJ%EhV0$D*{Nl2mw>nl_~poi;_)WA0d&u^?M<U+bo*s7Oh*@hmk*
z^!ff;)@2Y1&r<Wm(u(_9C-##iKP!s~oPR~~_fYj}UL1E0XYo~%x@vJBSVCtS?4Grf
z@GLby$YL|n#zmT&8l1t!jMuTP=pcEc^R-#cNb1fwio7!{#Iv85xz4|oot5G~aD->6
zNt2TTsE)CLA*jJzWK0dr004jhC;$-#M=54i4-$aDIEte%3SbZhL<l$vP80xu;D10^
zK@mhP7zBVEle%IQ1m-ef91jM-02ly+*uYMhP>(DFTh=t}nh{on7iA*E0z$QhnjhC8
zCtYIjZW*~2>%o=401{W2UfNK%SuRO#7z?U1OHmr$_k9#!4DKOfeHdDiLMUMhE_CY3
zQ}wM+3}S$xrp{sO0%D-Qg!Tg?N`D#c#11D%H1uMldybk2C_2GIa1@o`AvlV1g6&6a
zBm#;e0oq=kL|+Xk(;`Axi4jzulb=M-vXNUm0-}edv^<XF;UF|bEL;aGO3HyUXd&f5
z88k6w(AXZb_cg2b20{u-Z}C<nz{iAARh?|geYYKQ#VFi+XhmiG4;^%v=zqu$=*R1g
zez1GMebM(LHVMWg)roWz-LV|EY6+@Ey-NL+1I(uM&?2Z6(jNJW%xscWl(`{?w{V+H
z{3j}V(4Y_$d?5u8Nb<Kvi(X_xgWx*i-)Jo|L*giI$9)9W!7HhLCvO%PO>J)qU^;iT
zG&tlm=>j9yIEcC(gPg(fIe%Gp29&-BccznI$Z20Zp2%7_$W9>{;D>@m9ppFRLi`BZ
zxQFm2G$}GSoq&fM>MYyVZ?D8K%fMRllKy8xF*61u0i|dk(Um!gfT;c_$vzr)bR`hg
z8QidjRzibA!Kym`2j>)8MFY}}Gzpe5HBi$3iFF|fX;^3K2nxO;&<T@dg3AHb5UuS&
CN52^W

delta 1534
zcmV<a1p)e%41^4j7=JJ=G%zh^V>dT4F*z~{Np5p=VQyn(Iv_b=V_`LAF*YqTV`gS8
zG-hTvEnzrjFfCy?W;Qf4VP!dGGGq!?Lu_wjYdRo%F*+bOeF_TIx(m9^1&9F7A|rpb
zr2qf`001bpFZ}>e{VM<lJ=RLl7I6XqUp$Ch%_7lYJ_Lj_@PFoXP~+we(A76EO7~<&
z)F3;RA|;uhw(`4WgTRw>I&<{eU?~Ai$VOI~v629dx2JaIm@kx4N&&zCxB$xlBK}6v
zM#}nreKVfmYggvF>;bcooE<&pjzw|&ydgCd;B)Xi4C|Up=iRyONm;Mu=<J|49rmyj
z!~XSg?^aVP(0>n31mId~$~K1OP2OJJrGIzc!y|v*5Q}mxHFpG|K|%#jeQ^A;E^l>*
z3*Bp}NefgxX1+gm^KmUTgDMc1Jsb?P)oZC4!mL!?OgGcvU!7uNP_0u4V%4c@$$Tde
z<JJuBKo=+bJ1@oWHsew%rA$(CR4!bhCq$aiEJ3j}Cx2ttc)M<~yh-n@9Ru2aUZxgY
zOU+j=F>OCDy|bcHe0;hLW1g*=6kJQqvOo9n=`!@rT1v3PA=sEaGtUmrgF1V*+HwF$
z%o|ck2Ck)MDdN~U>lpNk)Rh9)Qd8p>$FU`TvVVQ}JNw3+pEy`&7|(v!cb#wKcU}Xc
zztr*E^M9!-1a3aZxQyRo-mQ)X;JxvQVS5a1E-Z#;*X8(mp!hlb(b=Wn_TxWHa?e&p
z0<NVdiD5jvlVdHR&M*6U`SOA@wvN<F<4PlXstt_|t42g+g<yeLeOP%|b(q;~x1wEl
zEj90~M>ms_NyVg8C=MtXRLX=hZO}2jlujiL*MCw|r<8%iFyl-urCG}bk_A!)lFN(Z
z*9rxd2`ZdQ%ZOz}8m(9>QmHeYBvm9q6rtA9wbTrW=uAh+mpZa8Z9M*WuhEnWw8OPP
z0Ken?`duE+M1Fc0sWuO;rRJ>TPnWxUKDEZdwbU$UUQcC(VtJFR2G>&aT`%TX-lUJ$
zzkkQk8h`!+Ak42Gb5O~I9X_GL4xhR(RGq@`<8YEmeGLBa%Ed{YaV#4wH7X6(wXrob
zRx(mCOfa|?Bo}qm;#z9jPz5?Ii>k-mKV61Bw$iw6MNm)>lUj2vHAVFK{#e#!kBV!l
ziDGG`ab1f2WXaFUg5t{}`TM8d6EBXt_J3uuRg$)9X&lOg&M*+4wU)S+njl=^i<EJZ
zX2ppwLyQ}LBXiMF<cgb*y<$jG_{C2oetkYX_VcpV`IoM<QW}RMaV<4zXt3a^V}T&3
z(OhIq3CsWhfB+}}AqGb&W<?JYfWSD4qc93!7zRWLI0_9I0RX{)ke~>l77Rj$ynhqj
zxeG%W18Ui|uoin^FOJH&6BE><l!R4e54%@5i^3tJ2=RjuhoRy}$qP!PWxP;CvBe2+
zB`AQ$m7GU4)NS@llAFhZ>1-;h#Us!zr2@yPWOP}$xk=>TS*Ej6Q8ZgBx&}sXh}Wym
z;Yvs=Xm6s&oTIkS5VXc28)t^2+kc4qR4r1#;*gdpU~^0s$pG8pm?G)gVGer7p5*j@
zrQSOro_cGm&fR}<@b8h^=iXW!%4reE=RqJFW<g443Z&4$8`^`Azzc12c=p(Su(w)T
z1)jmCmtK6UOn@l~=HxoFllhK1;>sqt`1BcS0HC@As3ba#z^Ko9Pi8avYk$Oa!!S#&
zSx3_!A8xyrpfT#3Y0zk_Hi}Y>p%$pI61F+5M3N&%2xKJAC;$CDD}<6?=$#5uoTZ5Z
zCNj}EgB|fKv=$L@ag>MEJ_76DnN*W1Z`O`MZ9ptwx|K1yxMV1V1(Ltx%(4y7obk4E
zW?=^8GIQ=kv4A0`wS7N1*>`Z!opukPx!_g@c{8|RJi<`!QS!z#<&+X#nnV8V6uo^%
zFL^|hfwSZ-+|LAZ&d5i?Zt>-$tMH_1UnT!Yb{@SussOv@WfJ50JyfLCQd9Bw_N-dh
k2=V>Mm|$S01}gnuSl5<E4okR>OyKJnJxL~`9Mur5?b8(5ga7~l

diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index b9b8c9004..361810c79 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -1801,17 +1801,19 @@ local function privilege_name(privilege)
 end
 
 local function object_resolve(object_type, object_name)
+    if object_name ~= nil and type(object_name) ~= 'string'
+            and type(object_name) ~= 'number' then
+        box.error(box.error.ILLEGAL_PARAMS, "wrong object name type")
+    end
     if object_type == 'universe' then
-        if object_name ~= nil and type(object_name) ~= 'string'
-                and type(object_name) ~= 'number' then
-            box.error(box.error.ILLEGAL_PARAMS, "wrong object name type")
-        end
         return 0
     end
+    if (object_type == 'space' or object_type == 'function' or
+       object_type == 'sequence' or object_type == 'role') and
+       object_name == nil then
+        return '*'
+    end
     if object_type == 'space' then
-        if object_name == nil or object_name == 0 then
-            return 0
-        end
         local space = box.space[object_name]
         if  space == nil then
             box.error(box.error.NO_SUCH_SPACE, object_name)
@@ -1819,9 +1821,6 @@ local function object_resolve(object_type, object_name)
         return space.id
     end
     if object_type == 'function' then
-        if object_name == nil or object_name == 0 then
-            return 0
-        end
         local _vfunc = box.space[box.schema.VFUNC_ID]
         local func
         if type(object_name) == 'string' then
@@ -1836,9 +1835,6 @@ local function object_resolve(object_type, object_name)
         end
     end
     if object_type == 'sequence' then
-        if object_name == nil or object_name == 0 then
-            return 0
-        end
         local seq = sequence_resolve(object_name)
         if seq == nil then
             box.error(box.error.NO_SUCH_SEQUENCE, object_name)
@@ -1867,6 +1863,9 @@ local function object_name(object_type, object_id)
     if object_type == 'universe' then
         return ""
     end
+    if object_id == '*' then
+        return '*'
+    end
     local space
     if object_type == 'space' then
         space = box.space._vspace
@@ -2106,10 +2105,10 @@ local function grant(uid, name, privilege, object_type,
         _priv:replace{options.grantor, uid, object_type, oid, privilege_hex}
     elseif not options.if_not_exists then
             if object_type == 'role' then
-                box.error(box.error.ROLE_GRANTED, name, object_name)
+                box.error(box.error.ROLE_GRANTED, name, object_name or '*')
             else
                 box.error(box.error.PRIV_GRANTED, name, privilege,
-                          object_type, object_name)
+                          object_type, object_name or '*')
             end
     end
 end
@@ -2134,10 +2133,10 @@ local function revoke(uid, name, privilege, object_type, object_name, options)
             return
         end
         if object_type == 'role' then
-            box.error(box.error.ROLE_NOT_GRANTED, name, object_name)
+            box.error(box.error.ROLE_NOT_GRANTED, name, object_name or '*')
         else
             box.error(box.error.PRIV_NOT_GRANTED, name, privilege,
-                      object_type, object_name)
+                      object_type, object_name or '*')
         end
     end
     local old_privilege = tuple[5]
@@ -2153,13 +2152,14 @@ local function revoke(uid, name, privilege, object_type, object_name, options)
             return
         end
         box.error(box.error.PRIV_NOT_GRANTED, name, privilege,
-                  object_type, object_name)
+                  object_type, object_name or '*')
     end
     if privilege_hex ~= 0 then
         _priv:replace{grantor, uid, object_type, oid, privilege_hex}
     else
         _priv:delete{uid, object_type, oid}
     end
+
 end
 
 local function drop(uid, opts)
@@ -2192,9 +2192,10 @@ local function drop(uid, opts)
     local privs = _vpriv.index.primary:select{uid}
 
     for k, tuple in pairs(privs) do
-	-- we need an additional box.session.su() here, because of
-	-- unnecessary check for privilege PRIV_REVOKE in priv_def_check()
-        box.session.su("admin", revoke, uid, uid, tuple[5], tuple[3], tuple[4])
+        -- we need an additional box.session.su() here, because of
+        -- unnecessary check for privilege PRIV_REVOKE in priv_def_check()
+        local oid = tuple[4] ~= '*' and tuple[4] or nil
+        box.session.su("admin", revoke, uid, uid, tuple[5], tuple[3], oid)
     end
     box.space[box.schema.USER_ID]:delete{uid}
 end
diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index 0293f6ef8..5dbc09bbb 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -964,6 +964,28 @@ local function upgrade_to_1_10_0()
     create_vsequence_space()
 end
 
+--------------------------------------------------------------------------------
+--- Tarantool 1.10.1
+--------------------------------------------------------------------------------
+local function upgrade_space_priv_to_1_10_1()
+    local _priv = box.space._priv
+    local _vpriv = box.space._vpriv
+    local f = _priv:format()
+
+    f[4].type = 'scalar'
+    _priv:format(f)
+    f = _vpriv:format()
+    f[4].type = 'scalar'
+    _vpriv:format(f)
+    _priv.index.primary:alter{parts={2, 'unsigned', 3, 'string', 4, 'scalar'}}
+    _vpriv.index.primary:alter{parts={2, 'unsigned', 3, 'string', 4, 'scalar'}}
+    _priv.index.object:alter{parts={3, 'string', 4, 'scalar'}}
+    _vpriv.index.object:alter{parts={3, 'string', 4, 'scalar'}}
+end
+
+local function upgrade_to_1_10_1()
+    upgrade_space_priv_to_1_10_1()
+end
 
 local function get_version()
     local version = box.space._schema:get{'version'}
@@ -991,6 +1013,7 @@ local function upgrade(options)
         {version = mkversion(1, 7, 6), func = upgrade_to_1_7_6, auto = false},
         {version = mkversion(1, 7, 7), func = upgrade_to_1_7_7, auto = true},
         {version = mkversion(1, 10, 0), func = upgrade_to_1_10_0, auto = true},
+        {version = mkversion(1, 10, 1), func = upgrade_to_1_10_1, auto = true},
     }
 
     for _, handler in ipairs(handlers) do
diff --git a/src/box/schema.cc b/src/box/schema.cc
index 433f52c08..9958e9016 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -536,10 +536,18 @@ schema_find_name(enum schema_object_type type, uint32_t object_id)
 	switch (type) {
 	case SC_UNIVERSE:
 		return "";
+	case SC_ENTITY_SPACE:
+		return "";
+	case SC_ENTITY_FUNCTION:
+		return "";
+	case SC_ENTITY_SEQUENCE:
+		return "";
+	case SC_ENTITY_ROLE:
+		return "";
+	case SC_ENTITY_USER:
+		return "";
 	case SC_SPACE:
 		{
-			if (object_id == 0)
-				return "SPACE";
 			struct space *space = space_by_id(object_id);
 			if (space == NULL)
 				break;
@@ -547,8 +555,6 @@ schema_find_name(enum schema_object_type type, uint32_t object_id)
 		}
 	case SC_FUNCTION:
 		{
-			if (object_id == 0)
-				return "FUNCTION";
 			struct func *func = func_by_id(object_id);
 			if (func == NULL)
 				break;
@@ -556,8 +562,6 @@ schema_find_name(enum schema_object_type type, uint32_t object_id)
 		}
 	case SC_SEQUENCE:
 		{
-			if (object_id == 0)
-				return "SEQUENCE";
 			struct sequence *seq = sequence_by_id(object_id);
 			if (seq == NULL)
 				break;
diff --git a/src/box/schema.h b/src/box/schema.h
index 0822262d0..f1735ff34 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -250,16 +250,19 @@ static inline
 struct access *
 entity_access_get(enum schema_object_type type)
 {
-       switch (type) {
-       case SC_SPACE:
-               return entity_access.space;
-       case SC_FUNCTION:
-               return entity_access.function;
-       case SC_SEQUENCE:
-               return entity_access.sequence;
-       default:
-               return NULL;
-       }
+	switch (type) {
+	case SC_SPACE:
+	case SC_ENTITY_SPACE:
+		return entity_access.space;
+	case SC_FUNCTION:
+	case SC_ENTITY_FUNCTION:
+		return entity_access.function;
+	case SC_SEQUENCE:
+	case SC_ENTITY_SEQUENCE:
+		return entity_access.sequence;
+	default:
+		return NULL;
+	}
 }
 
 #endif /* INCLUDES_TARANTOOL_BOX_SCHEMA_H */
diff --git a/src/box/schema_def.c b/src/box/schema_def.c
index 97c074ab2..18ec6c8d2 100644
--- a/src/box/schema_def.c
+++ b/src/box/schema_def.c
@@ -31,16 +31,39 @@
 #include "schema_def.h"
 
 static const char *object_type_strs[] = {
-	/* [SC_UKNNOWN]         = */ "unknown",
-	/* [SC_UNIVERSE]        = */ "universe",
-	/* [SC_SPACE]           = */ "space",
-	/* [SC_FUNCTION]        = */ "function",
-	/* [SC_USER]            = */ "user",
-	/* [SC_ROLE]            = */ "role",
-	/* [SC_SEQUENCE]        = */ "sequence",
-	/* [SC_COLLATION]       = */ "collation",
+	/* [SC_UKNNOWN]		    = */ "unknown",
+	/* [SC_UNIVERSE]	    = */ "universe",
+	/* [SC_SPACE]		    = */ "space",
+	/* [SC_ENTITY_SPACE]	    = */ "space",
+	/* [SC_FUNCTION]	    = */ "function",
+	/* [SC_ENTITY_FUNCTION]	    = */ "function",
+	/* [SC_USER]		    = */ "user",
+	/* [SC_ENTITY_USER]	    = */ "user",
+	/* [SC_ROLE]		    = */ "role",
+	/* [SC_ENTITY_ROLE]	    = */ "role",
+	/* [SC_SEQUENCE]	    = */ "sequence",
+	/* [SC_ENTITY_SEQUENCE]	    = */ "sequence",
+	/* [SC_COLLATION]	    = */ "collation",
+	/* [SC_ENTITY_COLLATION]    = */ "collation",
 };
 
+enum schema_object_type
+schema_entity_type(enum schema_object_type type)
+{
+	switch(type) {
+	case SC_SPACE:
+	case SC_FUNCTION:
+	case SC_USER:
+	case SC_ROLE:
+	case SC_SEQUENCE:
+	case SC_COLLATION:
+		return type + 1;
+		break;
+	default:
+		unreachable();
+	}
+}
+
 enum schema_object_type
 schema_object_type(const char *name)
 {
diff --git a/src/box/schema_def.h b/src/box/schema_def.h
index 2edb8d37f..9b5bd6864 100644
--- a/src/box/schema_def.h
+++ b/src/box/schema_def.h
@@ -218,19 +218,35 @@ enum {
  *
  * Use 0 for unknown to use the same index consistently
  * even when there are more object types in the future.
+ *
+ * When adding new types please follow this rule:
+ * SC_ENTITY_NEW_OBJECT = SC_NEW_OBJECT + 1
+ * schema_entity_type() relies on this convention.
  */
 enum schema_object_type {
 	SC_UNKNOWN = 0,
 	SC_UNIVERSE = 1,
 	SC_SPACE = 2,
-	SC_FUNCTION = 3,
-	SC_USER = 4,
-	SC_ROLE = 5,
-	SC_SEQUENCE = 6,
-	SC_COLLATION = 7,
-	schema_object_type_MAX = 8
+	SC_ENTITY_SPACE = 3,
+	SC_FUNCTION = 4,
+	SC_ENTITY_FUNCTION = 5,
+	SC_USER = 6,
+	SC_ENTITY_USER = 7,
+	SC_ROLE = 8,
+	SC_ENTITY_ROLE = 9,
+	SC_SEQUENCE = 10,
+	SC_ENTITY_SEQUENCE = 11,
+	SC_COLLATION = 12,
+	SC_ENTITY_COLLATION = 13,
+	schema_object_type_MAX = 13
 };
 
+/**
+ * Given a object type, return an entity type it belongs to.
+ */
+enum schema_object_type
+schema_entity_type(enum schema_object_type type);
+
 enum schema_object_type
 schema_object_type(const char *name);
 
diff --git a/src/box/user.cc b/src/box/user.cc
index fbf06566a..eec785652 100644
--- a/src/box/user.cc
+++ b/src/box/user.cc
@@ -207,12 +207,23 @@ access_find(struct priv_def *priv)
 		access = universe.access;
 		break;
 	}
+	case SC_ENTITY_SPACE:
+	{
+		access = entity_access.space;
+		break;
+	}
+	case SC_ENTITY_FUNCTION:
+	{
+		access = entity_access.function;
+		break;
+	}
+	case SC_ENTITY_SEQUENCE:
+	{
+		access = entity_access.sequence;
+		break;
+	}
 	case SC_SPACE:
 	{
-		if (priv->object_id == 0) {
-			access = entity_access.space;
-			break;
-		}
 		struct space *space = space_by_id(priv->object_id);
 		if (space)
 			access = space->access;
@@ -220,10 +231,6 @@ access_find(struct priv_def *priv)
 	}
 	case SC_FUNCTION:
 	{
-		if (priv->object_id == 0) {
-			access = entity_access.function;
-			break;
-		}
 		struct func *func = func_by_id(priv->object_id);
 		if (func)
 			access = func->access;
@@ -231,10 +238,6 @@ access_find(struct priv_def *priv)
 	}
 	case SC_SEQUENCE:
 	{
-		if (priv->object_id == 0) {
-			access = entity_access.sequence;
-			break;
-		}
 		struct sequence *seq = sequence_by_id(priv->object_id);
 		if (seq)
 			access = seq->access;
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index 16c2027cf..1641c318e 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -5,7 +5,7 @@ box.space._schema:select{}
 ---
 - - ['cluster', '<cluster uuid>']
   - ['max_id', 511]
-  - ['version', 1, 10, 0]
+  - ['version', 1, 10, 1]
 ...
 box.space._cluster:select{}
 ---
@@ -58,10 +58,10 @@ box.space._space:select{}
         'type': 'string'}, {'name': 'auth', 'type': 'map'}]]
   - [312, 1, '_priv', 'memtx', 0, {}, [{'name': 'grantor', 'type': 'unsigned'}, {
         'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
-      {'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
+      {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
   - [313, 1, '_vpriv', 'sysview', 0, {}, [{'name': 'grantor', 'type': 'unsigned'},
       {'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
-      {'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
+      {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
   - [320, 1, '_cluster', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'uuid',
         'type': 'string'}]]
   - [330, 1, '_truncate', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'count',
@@ -104,13 +104,13 @@ box.space._index:select{}
   - [305, 1, 'owner', 'tree', {'unique': false}, [[1, 'unsigned']]]
   - [305, 2, 'name', 'tree', {'unique': true}, [[2, 'string']]]
   - [312, 0, 'primary', 'tree', {'unique': true}, [[1, 'unsigned'], [2, 'string'],
-      [3, 'unsigned']]]
+      [3, 'scalar']]]
   - [312, 1, 'owner', 'tree', {'unique': false}, [[0, 'unsigned']]]
-  - [312, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'unsigned']]]
+  - [312, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'scalar']]]
   - [313, 0, 'primary', 'tree', {'unique': true}, [[1, 'unsigned'], [2, 'string'],
-      [3, 'unsigned']]]
+      [3, 'scalar']]]
   - [313, 1, 'owner', 'tree', {'unique': false}, [[0, 'unsigned']]]
-  - [313, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'unsigned']]]
+  - [313, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'scalar']]]
   - [320, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
   - [320, 1, 'uuid', 'tree', {'unique': true}, [[1, 'string']]]
   - [330, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
diff --git a/test/box/access.result b/test/box/access.result
index f4669a4a3..687bbf579 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -504,7 +504,7 @@ box.space._priv:select{id}
 ...
 box.schema.user.grant('user', 'read', 'universe')
 ---
-- error: User 'user' already has read access on universe 'nil'
+- error: User 'user' already has read access on universe '*'
 ...
 box.space._priv:select{id}
 ---
@@ -690,7 +690,7 @@ box.schema.user.grant('guest', 'read,write,execute', 'universe')
 ...
 box.schema.user.grant('guest', 'read,write,execute', 'universe')
 ---
-- error: User 'guest' already has read,write,execute access on universe 'nil'
+- error: User 'guest' already has read,write,execute access on universe '*'
 ...
 box.schema.user.grant('guest', 'read,write,execute', 'universe', '', { if_not_exists = true })
 ---
@@ -703,7 +703,7 @@ box.schema.user.revoke('guest', 'usage,session', 'universe')
 ...
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 ---
-- error: User 'guest' does not have read,write,execute access on universe 'nil'
+- error: User 'guest' does not have read,write,execute access on universe '*'
 ...
 box.schema.user.revoke('guest', 'read,write,execute', 'universe', '', { if_exists = true })
 ---
diff --git a/test/box/access_misc.result b/test/box/access_misc.result
index 2d87fa2d5..7d6aa0a4b 100644
--- a/test/box/access_misc.result
+++ b/test/box/access_misc.result
@@ -436,7 +436,7 @@ box.schema.user.revoke('testuser', 'usage,session', 'universe')
 ...
 box.schema.user.revoke('testuser', 'read, write, execute', 'universe')
 ---
-- error: User 'testuser' does not have read, write, execute access on universe 'nil'
+- error: User 'testuser' does not have read, write, execute access on universe '*'
 ...
 box.schema.user.revoke('testuser', 'create', 'universe')
 ---
@@ -797,10 +797,10 @@ box.space._space:select()
         'type': 'string'}, {'name': 'auth', 'type': 'map'}]]
   - [312, 1, '_priv', 'memtx', 0, {}, [{'name': 'grantor', 'type': 'unsigned'}, {
         'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
-      {'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
+      {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
   - [313, 1, '_vpriv', 'sysview', 0, {}, [{'name': 'grantor', 'type': 'unsigned'},
       {'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
-      {'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
+      {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
   - [320, 1, '_cluster', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'uuid',
         'type': 'string'}]]
   - [330, 1, '_truncate', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'count',
@@ -853,7 +853,7 @@ box.schema.user.grant('tester', 'read', 'universe')
 -- error: the privilege is not granted
 box.schema.user.revoke('tester', 'create', 'universe')
 ---
-- error: User 'tester' does not have create access on universe 'nil'
+- error: User 'tester' does not have create access on universe '*'
 ...
 -- no error: if_exists clause
 box.schema.user.revoke('tester', 'create', 'universe', nil, { if_exists = true })
diff --git a/test/box/alter.result b/test/box/alter.result
index eb7014d8b..36bdb5fd3 100644
--- a/test/box/alter.result
+++ b/test/box/alter.result
@@ -214,13 +214,13 @@ _index:select{}
   - [305, 1, 'owner', 'tree', {'unique': false}, [[1, 'unsigned']]]
   - [305, 2, 'name', 'tree', {'unique': true}, [[2, 'string']]]
   - [312, 0, 'primary', 'tree', {'unique': true}, [[1, 'unsigned'], [2, 'string'],
-      [3, 'unsigned']]]
+      [3, 'scalar']]]
   - [312, 1, 'owner', 'tree', {'unique': false}, [[0, 'unsigned']]]
-  - [312, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'unsigned']]]
+  - [312, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'scalar']]]
   - [313, 0, 'primary', 'tree', {'unique': true}, [[1, 'unsigned'], [2, 'string'],
-      [3, 'unsigned']]]
+      [3, 'scalar']]]
   - [313, 1, 'owner', 'tree', {'unique': false}, [[0, 'unsigned']]]
-  - [313, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'unsigned']]]
+  - [313, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'scalar']]]
   - [320, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
   - [320, 1, 'uuid', 'tree', {'unique': true}, [[1, 'string']]]
   - [330, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
diff --git a/test/xlog/upgrade.result b/test/xlog/upgrade.result
index f02996bba..8f5b4ad26 100644
--- a/test/xlog/upgrade.result
+++ b/test/xlog/upgrade.result
@@ -36,7 +36,7 @@ box.space._schema:select()
 ---
 - - ['cluster', '<server_uuid>']
   - ['max_id', 513]
-  - ['version', 1, 10, 0]
+  - ['version', 1, 10, 1]
 ...
 box.space._space:select()
 ---
@@ -85,10 +85,10 @@ box.space._space:select()
         'type': 'string'}, {'name': 'auth', 'type': 'map'}]]
   - [312, 1, '_priv', 'memtx', 0, {}, [{'name': 'grantor', 'type': 'unsigned'}, {
         'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
-      {'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
+      {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
   - [313, 1, '_vpriv', 'sysview', 0, {}, [{'name': 'grantor', 'type': 'unsigned'},
       {'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
-      {'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
+      {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
   - [320, 1, '_cluster', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'uuid',
         'type': 'string'}]]
   - [330, 1, '_truncate', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'count',
@@ -134,13 +134,13 @@ box.space._index:select()
   - [305, 1, 'owner', 'tree', {'unique': false}, [[1, 'unsigned']]]
   - [305, 2, 'name', 'tree', {'unique': true}, [[2, 'string']]]
   - [312, 0, 'primary', 'tree', {'unique': true}, [[1, 'unsigned'], [2, 'string'],
-      [3, 'unsigned']]]
+      [3, 'scalar']]]
   - [312, 1, 'owner', 'tree', {'unique': false}, [[0, 'unsigned']]]
-  - [312, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'unsigned']]]
+  - [312, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'scalar']]]
   - [313, 0, 'primary', 'tree', {'unique': true}, [[1, 'unsigned'], [2, 'string'],
-      [3, 'unsigned']]]
+      [3, 'scalar']]]
   - [313, 1, 'owner', 'tree', {'unique': false}, [[0, 'unsigned']]]
-  - [313, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'unsigned']]]
+  - [313, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'scalar']]]
   - [320, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
   - [320, 1, 'uuid', 'tree', {'unique': true}, [[1, 'string']]]
   - [330, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
-- 
2.15.2 (Apple Git-101.1)



[-- Attachment #2: Type: text/html, Size: 102411 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [tarantool-patches] [PATCH v2] Introduce separate entity object types for entity privileges.
  2018-08-14 13:41   ` Serge Petrenko
@ 2018-08-17  9:19     ` Vladimir Davydov
  2018-08-17 12:19       ` [tarantool-patches] " Serge Petrenko
  0 siblings, 1 reply; 6+ messages in thread
From: Vladimir Davydov @ 2018-08-17  9:19 UTC (permalink / raw)
  To: Serge Petrenko; +Cc: tarantool-patches, kostja

On Tue, Aug 14, 2018 at 04:41:25PM +0300, Serge Petrenko wrote:
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 3007a131d..7d65cab3a 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -2537,10 +2537,41 @@ priv_def_create_from_tuple(struct priv_def *priv, struct tuple *tuple)
>  {
>  	priv->grantor_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_ID);
>  	priv->grantee_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_UID);
> +
>  	const char *object_type =
>  		tuple_field_cstr_xc(tuple, BOX_PRIV_FIELD_OBJECT_TYPE);
> -	priv->object_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_OBJECT_ID);
>  	priv->object_type = schema_object_type(object_type);
> +
> +	const char *object_id = tuple_field(tuple, BOX_PRIV_FIELD_OBJECT_ID);

This isn't object_id. It's msgpack. Please rename this variable to
emphasize that. For instance, 'object_id_field', 'object_id_raw', or
simply 'data'.

> +	if (object_id == NULL) {
> +	    tnt_raise(ClientError, ER_NO_SUCH_FIELD,
> +		      BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE);
> +	}
> +	/*
> +	 * When granting or revoking privileges on a whole entity
> +	 * we pass an asterisk ('*') to object_id to indicate
> +	 * grant on every object of that entity.
> +	 * So check for that first.
> +	 */
> +	if (mp_typeof(*object_id) == MP_STR) {
> +		object_id = tuple_field_cstr_xc(tuple, BOX_PRIV_FIELD_OBJECT_ID);
> +		if(*object_id == '*' && strlen(object_id) == 1) {

Nit: in a case like this, better use strcmp.

> +			priv->object_id = 0;
> +			priv->object_type = schema_entity_type(priv->object_type);
> +		} else {
> +			tnt_raise(ClientError, ER_FIELD_TYPE,
> +				  BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE,
> +				  field_type_strs[FIELD_TYPE_UNSIGNED]);
> +		}
> +	} else if (mp_typeof(*object_id) == MP_UINT) {
> +		priv->object_id = tuple_field_u32_xc(tuple,
> +						     BOX_PRIV_FIELD_OBJECT_ID);
> +	} else {
> +		tnt_raise(ClientError, ER_FIELD_TYPE,
> +			  BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE,
> +			  field_type_strs[FIELD_TYPE_UNSIGNED]);
> +	}
> +

Somehow, I don't like this piece of code. For instance, you raise
the same error (FIELD_TYPE_UNSIGNED) in two places. Also, you use
tuple_field_*_xc after you explicitly check the type, which is
pointless, because they can't fail.

May be, something like this?

	switch (mp_typeof(*data)) {
	case MP_STR:
		if (mp_decode_strl(&data) == 0) {
			/* Entity-wide privilege. */
			priv->object_id = 0;
			break;
		}
		FALLTHROUGH;
	default:
		priv->object_id = tuple_field_u32_xc(...);
	}

>  	if (priv->object_type == SC_UNKNOWN) {
>  		tnt_raise(ClientError, ER_UNKNOWN_SCHEMA_OBJECT,
>  			  object_type);
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index b9b8c9004..361810c79 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -1801,17 +1801,19 @@ local function privilege_name(privilege)
>  end
>  
>  local function object_resolve(object_type, object_name)
> +    if object_name ~= nil and type(object_name) ~= 'string'
> +            and type(object_name) ~= 'number' then
> +        box.error(box.error.ILLEGAL_PARAMS, "wrong object name type")
> +    end
>      if object_type == 'universe' then
> -        if object_name ~= nil and type(object_name) ~= 'string'
> -                and type(object_name) ~= 'number' then
> -            box.error(box.error.ILLEGAL_PARAMS, "wrong object name type")
> -        end
>          return 0
>      end
> +    if (object_type == 'space' or object_type == 'function' or
> +       object_type == 'sequence' or object_type == 'role') and
> +       object_name == nil then
> +        return '*'
> +    end

No, I don't like this: now, if you want to add a new object type,
you have to patch two places: here and below, where the object name
is resolved. Let's leave it as before.

>      if object_type == 'space' then
> -        if object_name == nil or object_name == 0 then
> -            return 0
> -        end
>          local space = box.space[object_name]
>          if  space == nil then
>              box.error(box.error.NO_SUCH_SPACE, object_name)
> @@ -1819,9 +1821,6 @@ local function object_resolve(object_type, object_name)
>          return space.id
>      end
>      if object_type == 'function' then
> -        if object_name == nil or object_name == 0 then
> -            return 0
> -        end
>          local _vfunc = box.space[box.schema.VFUNC_ID]
>          local func
>          if type(object_name) == 'string' then
> @@ -1836,9 +1835,6 @@ local function object_resolve(object_type, object_name)
>          end
>      end
>      if object_type == 'sequence' then
> -        if object_name == nil or object_name == 0 then
> -            return 0
> -        end
>          local seq = sequence_resolve(object_name)
>          if seq == nil then
>              box.error(box.error.NO_SUCH_SEQUENCE, object_name)
> @@ -1867,6 +1863,9 @@ local function object_name(object_type, object_id)
>      if object_type == 'universe' then
>          return ""
>      end
> +    if object_id == '*' then
> +        return '*'
> +    end
>      local space
>      if object_type == 'space' then
>          space = box.space._vspace
> @@ -2106,10 +2105,10 @@ local function grant(uid, name, privilege, object_type,
>          _priv:replace{options.grantor, uid, object_type, oid, privilege_hex}
>      elseif not options.if_not_exists then
>              if object_type == 'role' then
> -                box.error(box.error.ROLE_GRANTED, name, object_name)
> +                box.error(box.error.ROLE_GRANTED, name, object_name or '*')
>              else
>                  box.error(box.error.PRIV_GRANTED, name, privilege,
> -                          object_type, object_name)
> +                          object_type, object_name or '*')
>              end
>      end
>  end
> @@ -2134,10 +2133,10 @@ local function revoke(uid, name, privilege, object_type, object_name, options)
>              return
>          end
>          if object_type == 'role' then
> -            box.error(box.error.ROLE_NOT_GRANTED, name, object_name)
> +            box.error(box.error.ROLE_NOT_GRANTED, name, object_name or '*')
>          else
>              box.error(box.error.PRIV_NOT_GRANTED, name, privilege,
> -                      object_type, object_name)
> +                      object_type, object_name or '*')
>          end
>      end
>      local old_privilege = tuple[5]
> @@ -2153,13 +2152,14 @@ local function revoke(uid, name, privilege, object_type, object_name, options)
>              return
>          end
>          box.error(box.error.PRIV_NOT_GRANTED, name, privilege,
> -                  object_type, object_name)
> +                  object_type, object_name or '*')
>      end
>      if privilege_hex ~= 0 then
>          _priv:replace{grantor, uid, object_type, oid, privilege_hex}
>      else
>          _priv:delete{uid, object_type, oid}
>      end
> +

Extra new line, please remove.

>  end
>  
>  local function drop(uid, opts)
> @@ -2192,9 +2192,10 @@ local function drop(uid, opts)
>      local privs = _vpriv.index.primary:select{uid}
>  
>      for k, tuple in pairs(privs) do
> -	-- we need an additional box.session.su() here, because of
> -	-- unnecessary check for privilege PRIV_REVOKE in priv_def_check()
> -        box.session.su("admin", revoke, uid, uid, tuple[5], tuple[3], tuple[4])
> +        -- we need an additional box.session.su() here, because of
> +        -- unnecessary check for privilege PRIV_REVOKE in priv_def_check()
> +        local oid = tuple[4] ~= '*' and tuple[4] or nil
> +        box.session.su("admin", revoke, uid, uid, tuple[5], tuple[3], oid)
>      end
>      box.space[box.schema.USER_ID]:delete{uid}
>  end
> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index 0293f6ef8..5dbc09bbb 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -964,6 +964,28 @@ local function upgrade_to_1_10_0()
>      create_vsequence_space()
>  end
>  
> +--------------------------------------------------------------------------------
> +--- Tarantool 1.10.1
> +--------------------------------------------------------------------------------
> +local function upgrade_space_priv_to_1_10_1()
> +    local _priv = box.space._priv
> +    local _vpriv = box.space._vpriv
> +    local f = _priv:format()
> +
> +    f[4].type = 'scalar'
> +    _priv:format(f)
> +    f = _vpriv:format()
> +    f[4].type = 'scalar'
> +    _vpriv:format(f)
> +    _priv.index.primary:alter{parts={2, 'unsigned', 3, 'string', 4, 'scalar'}}
> +    _vpriv.index.primary:alter{parts={2, 'unsigned', 3, 'string', 4, 'scalar'}}
> +    _priv.index.object:alter{parts={3, 'string', 4, 'scalar'}}
> +    _vpriv.index.object:alter{parts={3, 'string', 4, 'scalar'}}
> +end
> +
> +local function upgrade_to_1_10_1()
> +    upgrade_space_priv_to_1_10_1()
> +end
>  
>  local function get_version()
>      local version = box.space._schema:get{'version'}
> @@ -991,6 +1013,7 @@ local function upgrade(options)
>          {version = mkversion(1, 7, 6), func = upgrade_to_1_7_6, auto = false},
>          {version = mkversion(1, 7, 7), func = upgrade_to_1_7_7, auto = true},
>          {version = mkversion(1, 10, 0), func = upgrade_to_1_10_0, auto = true},
> +        {version = mkversion(1, 10, 1), func = upgrade_to_1_10_1, auto = true},

Should be 1.10.2

1.10.1 has already been released.

>      }
>  
>      for _, handler in ipairs(handlers) do
> diff --git a/src/box/schema.cc b/src/box/schema.cc
> index 433f52c08..9958e9016 100644
> --- a/src/box/schema.cc
> +++ b/src/box/schema.cc
> @@ -536,10 +536,18 @@ schema_find_name(enum schema_object_type type, uint32_t object_id)
>  	switch (type) {
>  	case SC_UNIVERSE:
>  		return "";
> +	case SC_ENTITY_SPACE:
> +		return "";
> +	case SC_ENTITY_FUNCTION:
> +		return "";
> +	case SC_ENTITY_SEQUENCE:
> +		return "";
> +	case SC_ENTITY_ROLE:
> +		return "";
> +	case SC_ENTITY_USER:
> +		return "";

Please rewrite it as

	case SC_ENTITY_SPACE:
	case SC_ENTITY_FUNCTION:
	case SC_ENTITY_SEQUENCE:
	...
		return "";

Also, I don't like that sometimes we use '*' and sometimes '' when
reporting errors. What about using '' for entity-wide privileges
everywhere, including the _priv space? This would save us from
possible conflicts with objects named '*' (yeah, no sane user would
call a space like that, but still). This would also allow us to
simplify object_resolve() as I suggested in the previous review
round.

>  	case SC_SPACE:
>  		{
> -			if (object_id == 0)
> -				return "SPACE";
>  			struct space *space = space_by_id(object_id);
>  			if (space == NULL)
>  				break;
> @@ -547,8 +555,6 @@ schema_find_name(enum schema_object_type type, uint32_t object_id)
>  		}
>  	case SC_FUNCTION:
>  		{
> -			if (object_id == 0)
> -				return "FUNCTION";
>  			struct func *func = func_by_id(object_id);
>  			if (func == NULL)
>  				break;
> @@ -556,8 +562,6 @@ schema_find_name(enum schema_object_type type, uint32_t object_id)
>  		}
>  	case SC_SEQUENCE:
>  		{
> -			if (object_id == 0)
> -				return "SEQUENCE";
>  			struct sequence *seq = sequence_by_id(object_id);
>  			if (seq == NULL)
>  				break;
> diff --git a/src/box/schema.h b/src/box/schema.h
> index 0822262d0..f1735ff34 100644
> --- a/src/box/schema.h
> +++ b/src/box/schema.h
> @@ -250,16 +250,19 @@ static inline
>  struct access *
>  entity_access_get(enum schema_object_type type)
>  {
> -       switch (type) {
> -       case SC_SPACE:
> -               return entity_access.space;
> -       case SC_FUNCTION:
> -               return entity_access.function;
> -       case SC_SEQUENCE:
> -               return entity_access.sequence;
> -       default:
> -               return NULL;
> -       }
> +	switch (type) {
> +	case SC_SPACE:
> +	case SC_ENTITY_SPACE:
> +		return entity_access.space;
> +	case SC_FUNCTION:
> +	case SC_ENTITY_FUNCTION:
> +		return entity_access.function;
> +	case SC_SEQUENCE:
> +	case SC_ENTITY_SEQUENCE:
> +		return entity_access.sequence;
> +	default:
> +		return NULL;
> +	}
>  }
>  
>  #endif /* INCLUDES_TARANTOOL_BOX_SCHEMA_H */
> diff --git a/src/box/schema_def.c b/src/box/schema_def.c
> index 97c074ab2..18ec6c8d2 100644
> --- a/src/box/schema_def.c
> +++ b/src/box/schema_def.c
> @@ -31,16 +31,39 @@
>  #include "schema_def.h"
>  
>  static const char *object_type_strs[] = {
> -	/* [SC_UKNNOWN]         = */ "unknown",
> -	/* [SC_UNIVERSE]        = */ "universe",
> -	/* [SC_SPACE]           = */ "space",
> -	/* [SC_FUNCTION]        = */ "function",
> -	/* [SC_USER]            = */ "user",
> -	/* [SC_ROLE]            = */ "role",
> -	/* [SC_SEQUENCE]        = */ "sequence",
> -	/* [SC_COLLATION]       = */ "collation",
> +	/* [SC_UKNNOWN]		    = */ "unknown",
> +	/* [SC_UNIVERSE]	    = */ "universe",
> +	/* [SC_SPACE]		    = */ "space",
> +	/* [SC_ENTITY_SPACE]	    = */ "space",
> +	/* [SC_FUNCTION]	    = */ "function",
> +	/* [SC_ENTITY_FUNCTION]	    = */ "function",
> +	/* [SC_USER]		    = */ "user",
> +	/* [SC_ENTITY_USER]	    = */ "user",
> +	/* [SC_ROLE]		    = */ "role",
> +	/* [SC_ENTITY_ROLE]	    = */ "role",
> +	/* [SC_SEQUENCE]	    = */ "sequence",
> +	/* [SC_ENTITY_SEQUENCE]	    = */ "sequence",
> +	/* [SC_COLLATION]	    = */ "collation",
> +	/* [SC_ENTITY_COLLATION]    = */ "collation",

I don't like this code duplication. Actually, I don't think that you
need to have separate names for entity-wide privileges at all: AFAICS,
schema_object_name() is never called for SC_ENTITY_*. Let's remove
SC_ENTITY_* from this array.

If you do that, you won't be able to increment object type to get entity
type. Well, OK, it's not that scary, taking into account the fact that
you always use a helper function for that (see right below). I see it
that way

enum schema_object_type {
	SC_UNKNOWN = 0,
	SC_UNIVERSE = 1,
	SC_SPACE = 2,
	SC_FUNCTION = 3,
	SC_USER = 4,
	SC_ROLE = 5,
	SC_SEQUENCE = 6,
	SC_COLLATION = 7,
	schema_object_type_MAX = 8

	/* Entity types. */
	SC_ENTITY_SPACE,
	SC_ENTITY_FUNCTION,
	...
}

enum schema_object_type
schema_entity_type(enum schema_object_type type)
{
	switch (type) {
	case SC_SPACE:
		return SC_ENTITY_SPACE;
	case SC_FUNCTION:
		return SC_ENTITY_FUNCTION;
	...
	}
}

>  };
>  
> +enum schema_object_type
> +schema_entity_type(enum schema_object_type type)
> +{
> +	switch(type) {

             ^^
         space missing

> +	case SC_SPACE:
> +	case SC_FUNCTION:
> +	case SC_USER:
> +	case SC_ROLE:
> +	case SC_SEQUENCE:
> +	case SC_COLLATION:
> +		return type + 1;
> +		break;
> +	default:
> +		unreachable();
> +	}
> +}
> +
>  enum schema_object_type
>  schema_object_type(const char *name)
>  {
> diff --git a/src/box/schema_def.h b/src/box/schema_def.h
> index 2edb8d37f..9b5bd6864 100644
> --- a/src/box/schema_def.h
> +++ b/src/box/schema_def.h
> @@ -218,19 +218,35 @@ enum {
>   *
>   * Use 0 for unknown to use the same index consistently
>   * even when there are more object types in the future.
> + *
> + * When adding new types please follow this rule:
> + * SC_ENTITY_NEW_OBJECT = SC_NEW_OBJECT + 1
> + * schema_entity_type() relies on this convention.
>   */
>  enum schema_object_type {
>  	SC_UNKNOWN = 0,
>  	SC_UNIVERSE = 1,
>  	SC_SPACE = 2,
> -	SC_FUNCTION = 3,
> -	SC_USER = 4,
> -	SC_ROLE = 5,
> -	SC_SEQUENCE = 6,
> -	SC_COLLATION = 7,
> -	schema_object_type_MAX = 8
> +	SC_ENTITY_SPACE = 3,
> +	SC_FUNCTION = 4,
> +	SC_ENTITY_FUNCTION = 5,
> +	SC_USER = 6,
> +	SC_ENTITY_USER = 7,
> +	SC_ROLE = 8,
> +	SC_ENTITY_ROLE = 9,
> +	SC_SEQUENCE = 10,
> +	SC_ENTITY_SEQUENCE = 11,
> +	SC_COLLATION = 12,
> +	SC_ENTITY_COLLATION = 13,
> +	schema_object_type_MAX = 13
>  };

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [tarantool-patches] Re: [PATCH v2] Introduce separate entity object types for entity privileges.
  2018-08-17  9:19     ` Vladimir Davydov
@ 2018-08-17 12:19       ` Serge Petrenko
  2018-08-17 15:57         ` Vladimir Davydov
  0 siblings, 1 reply; 6+ messages in thread
From: Serge Petrenko @ 2018-08-17 12:19 UTC (permalink / raw)
  To: Vladimir Davydov; +Cc: Konstantin Osipov, tarantool-patches

Hi! Thank you for review. I fixed your comments, the new diff is below.

> 17 авг. 2018 г., в 12:19, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а):
> 
> On Tue, Aug 14, 2018 at 04:41:25PM +0300, Serge Petrenko wrote:
>> diff --git a/src/box/alter.cc b/src/box/alter.cc
>> index 3007a131d..7d65cab3a 100644
>> --- a/src/box/alter.cc
>> +++ b/src/box/alter.cc
>> @@ -2537,10 +2537,41 @@ priv_def_create_from_tuple(struct priv_def *priv, struct tuple *tuple)
>> {
>> 	priv->grantor_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_ID);
>> 	priv->grantee_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_UID);
>> +
>> 	const char *object_type =
>> 		tuple_field_cstr_xc(tuple, BOX_PRIV_FIELD_OBJECT_TYPE);
>> -	priv->object_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_OBJECT_ID);
>> 	priv->object_type = schema_object_type(object_type);
>> +
>> +	const char *object_id = tuple_field(tuple, BOX_PRIV_FIELD_OBJECT_ID);
> 
> This isn't object_id. It's msgpack. Please rename this variable to
> emphasize that. For instance, 'object_id_field', 'object_id_raw', or
> simply 'data’.

Ok, let it be ‘data’.

> 
>> +	if (object_id == NULL) {
>> +	    tnt_raise(ClientError, ER_NO_SUCH_FIELD,
>> +		      BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE);
>> +	}
>> +	/*
>> +	 * When granting or revoking privileges on a whole entity
>> +	 * we pass an asterisk ('*') to object_id to indicate
>> +	 * grant on every object of that entity.
>> +	 * So check for that first.
>> +	 */
>> +	if (mp_typeof(*object_id) == MP_STR) {
>> +		object_id = tuple_field_cstr_xc(tuple, BOX_PRIV_FIELD_OBJECT_ID);
>> +		if(*object_id == '*' && strlen(object_id) == 1) {
> 
> Nit: in a case like this, better use strcmp.

We decided to switch to «» (empty string), so use mp_decode_strl(&data) == 0 here

> 
>> +			priv->object_id = 0;
>> +			priv->object_type = schema_entity_type(priv->object_type);
>> +		} else {
>> +			tnt_raise(ClientError, ER_FIELD_TYPE,
>> +				  BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE,
>> +				  field_type_strs[FIELD_TYPE_UNSIGNED]);
>> +		}
>> +	} else if (mp_typeof(*object_id) == MP_UINT) {
>> +		priv->object_id = tuple_field_u32_xc(tuple,
>> +						     BOX_PRIV_FIELD_OBJECT_ID);
>> +	} else {
>> +		tnt_raise(ClientError, ER_FIELD_TYPE,
>> +			  BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE,
>> +			  field_type_strs[FIELD_TYPE_UNSIGNED]);
>> +	}
>> +
> 
> Somehow, I don't like this piece of code. For instance, you raise
> the same error (FIELD_TYPE_UNSIGNED) in two places. Also, you use
> tuple_field_*_xc after you explicitly check the type, which is
> pointless, because they can't fail.
> 
> May be, something like this?
> 
> 	switch (mp_typeof(*data)) {
> 	case MP_STR:
> 		if (mp_decode_strl(&data) == 0) {
> 			/* Entity-wide privilege. */
> 			priv->object_id = 0;
> 			break;
> 		}
> 		FALLTHROUGH;
> 	default:
> 		priv->object_id = tuple_field_u32_xc(...);
> 	}

Ok, I use your variant.

> 
>> 	if (priv->object_type == SC_UNKNOWN) {
>> 		tnt_raise(ClientError, ER_UNKNOWN_SCHEMA_OBJECT,
>> 			  object_type);
>> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
>> index b9b8c9004..361810c79 100644
>> --- a/src/box/lua/schema.lua
>> +++ b/src/box/lua/schema.lua
>> @@ -1801,17 +1801,19 @@ local function privilege_name(privilege)
>> end
>> 
>> local function object_resolve(object_type, object_name)
>> +    if object_name ~= nil and type(object_name) ~= 'string'
>> +            and type(object_name) ~= 'number' then
>> +        box.error(box.error.ILLEGAL_PARAMS, "wrong object name type")
>> +    end
>>     if object_type == 'universe' then
>> -        if object_name ~= nil and type(object_name) ~= 'string'
>> -                and type(object_name) ~= 'number' then
>> -            box.error(box.error.ILLEGAL_PARAMS, "wrong object name type")
>> -        end
>>         return 0
>>     end
>> +    if (object_type == 'space' or object_type == 'function' or
>> +       object_type == 'sequence' or object_type == 'role') and
>> +       object_name == nil then
>> +        return '*'
>> +    end
> 
> No, I don't like this: now, if you want to add a new object type,
> you have to patch two places: here and below, where the object name
> is resolved. Let's leave it as before.

Ok.

> 
>>     if object_type == 'space' then
>> -        if object_name == nil or object_name == 0 then
>> -            return 0
>> -        end
>>         local space = box.space[object_name]
>>         if  space == nil then
>>             box.error(box.error.NO_SUCH_SPACE, object_name)
>> @@ -1819,9 +1821,6 @@ local function object_resolve(object_type, object_name)
>>         return space.id
>>     end
>>     if object_type == 'function' then
>> -        if object_name == nil or object_name == 0 then
>> -            return 0
>> -        end
>>         local _vfunc = box.space[box.schema.VFUNC_ID]
>>         local func
>>         if type(object_name) == 'string' then
>> @@ -1836,9 +1835,6 @@ local function object_resolve(object_type, object_name)
>>         end
>>     end
>>     if object_type == 'sequence' then
>> -        if object_name == nil or object_name == 0 then
>> -            return 0
>> -        end
>>         local seq = sequence_resolve(object_name)
>>         if seq == nil then
>>             box.error(box.error.NO_SUCH_SEQUENCE, object_name)
>> @@ -1867,6 +1863,9 @@ local function object_name(object_type, object_id)
>>     if object_type == 'universe' then
>>         return ""
>>     end
>> +    if object_id == '*' then
>> +        return '*'
>> +    end
>>     local space
>>     if object_type == 'space' then
>>         space = box.space._vspace
>> @@ -2106,10 +2105,10 @@ local function grant(uid, name, privilege, object_type,
>>         _priv:replace{options.grantor, uid, object_type, oid, privilege_hex}
>>     elseif not options.if_not_exists then
>>             if object_type == 'role' then
>> -                box.error(box.error.ROLE_GRANTED, name, object_name)
>> +                box.error(box.error.ROLE_GRANTED, name, object_name or '*')
>>             else
>>                 box.error(box.error.PRIV_GRANTED, name, privilege,
>> -                          object_type, object_name)
>> +                          object_type, object_name or '*')
>>             end
>>     end
>> end
>> @@ -2134,10 +2133,10 @@ local function revoke(uid, name, privilege, object_type, object_name, options)
>>             return
>>         end
>>         if object_type == 'role' then
>> -            box.error(box.error.ROLE_NOT_GRANTED, name, object_name)
>> +            box.error(box.error.ROLE_NOT_GRANTED, name, object_name or '*')
>>         else
>>             box.error(box.error.PRIV_NOT_GRANTED, name, privilege,
>> -                      object_type, object_name)
>> +                      object_type, object_name or '*')
>>         end
>>     end
>>     local old_privilege = tuple[5]
>> @@ -2153,13 +2152,14 @@ local function revoke(uid, name, privilege, object_type, object_name, options)
>>             return
>>         end
>>         box.error(box.error.PRIV_NOT_GRANTED, name, privilege,
>> -                  object_type, object_name)
>> +                  object_type, object_name or '*')
>>     end
>>     if privilege_hex ~= 0 then
>>         _priv:replace{grantor, uid, object_type, oid, privilege_hex}
>>     else
>>         _priv:delete{uid, object_type, oid}
>>     end
>> +
> 
> Extra new line, please remove.

Done

> 
>> end
>> 
>> local function drop(uid, opts)
>> @@ -2192,9 +2192,10 @@ local function drop(uid, opts)
>>     local privs = _vpriv.index.primary:select{uid}
>> 
>>     for k, tuple in pairs(privs) do
>> -	-- we need an additional box.session.su() here, because of
>> -	-- unnecessary check for privilege PRIV_REVOKE in priv_def_check()
>> -        box.session.su("admin", revoke, uid, uid, tuple[5], tuple[3], tuple[4])
>> +        -- we need an additional box.session.su() here, because of
>> +        -- unnecessary check for privilege PRIV_REVOKE in priv_def_check()
>> +        local oid = tuple[4] ~= '*' and tuple[4] or nil
>> +        box.session.su("admin", revoke, uid, uid, tuple[5], tuple[3], oid)
>>     end
>>     box.space[box.schema.USER_ID]:delete{uid}
>> end
>> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
>> index 0293f6ef8..5dbc09bbb 100644
>> --- a/src/box/lua/upgrade.lua
>> +++ b/src/box/lua/upgrade.lua
>> @@ -964,6 +964,28 @@ local function upgrade_to_1_10_0()
>>     create_vsequence_space()
>> end
>> 
>> +--------------------------------------------------------------------------------
>> +--- Tarantool 1.10.1
>> +--------------------------------------------------------------------------------
>> +local function upgrade_space_priv_to_1_10_1()
>> +    local _priv = box.space._priv
>> +    local _vpriv = box.space._vpriv
>> +    local f = _priv:format()
>> +
>> +    f[4].type = 'scalar'
>> +    _priv:format(f)
>> +    f = _vpriv:format()
>> +    f[4].type = 'scalar'
>> +    _vpriv:format(f)
>> +    _priv.index.primary:alter{parts={2, 'unsigned', 3, 'string', 4, 'scalar'}}
>> +    _vpriv.index.primary:alter{parts={2, 'unsigned', 3, 'string', 4, 'scalar'}}
>> +    _priv.index.object:alter{parts={3, 'string', 4, 'scalar'}}
>> +    _vpriv.index.object:alter{parts={3, 'string', 4, 'scalar'}}
>> +end
>> +
>> +local function upgrade_to_1_10_1()
>> +    upgrade_space_priv_to_1_10_1()
>> +end
>> 
>> local function get_version()
>>     local version = box.space._schema:get{'version'}
>> @@ -991,6 +1013,7 @@ local function upgrade(options)
>>         {version = mkversion(1, 7, 6), func = upgrade_to_1_7_6, auto = false},
>>         {version = mkversion(1, 7, 7), func = upgrade_to_1_7_7, auto = true},
>>         {version = mkversion(1, 10, 0), func = upgrade_to_1_10_0, auto = true},
>> +        {version = mkversion(1, 10, 1), func = upgrade_to_1_10_1, auto = true},
> 
> Should be 1.10.2
> 
> 1.10.1 has already been released.

Changed to 1.10.2 and updated bootstrap.snap

> 
>>     }
>> 
>>     for _, handler in ipairs(handlers) do
>> diff --git a/src/box/schema.cc b/src/box/schema.cc
>> index 433f52c08..9958e9016 100644
>> --- a/src/box/schema.cc
>> +++ b/src/box/schema.cc
>> @@ -536,10 +536,18 @@ schema_find_name(enum schema_object_type type, uint32_t object_id)
>> 	switch (type) {
>> 	case SC_UNIVERSE:
>> 		return "";
>> +	case SC_ENTITY_SPACE:
>> +		return "";
>> +	case SC_ENTITY_FUNCTION:
>> +		return "";
>> +	case SC_ENTITY_SEQUENCE:
>> +		return "";
>> +	case SC_ENTITY_ROLE:
>> +		return "";
>> +	case SC_ENTITY_USER:
>> +		return "";
> 
> Please rewrite it as
> 
> 	case SC_ENTITY_SPACE:
> 	case SC_ENTITY_FUNCTION:
> 	case SC_ENTITY_SEQUENCE:
> 	...
> 		return "";
> 
> Also, I don't like that sometimes we use '*' and sometimes '' when
> reporting errors. What about using '' for entity-wide privileges
> everywhere, including the _priv space? This would save us from
> possible conflicts with objects named '*' (yeah, no sane user would
> call a space like that, but still). This would also allow us to
> simplify object_resolve() as I suggested in the previous review
> round.

Done. I like the idea with using «» instead of «*», switched to that.
> 
>> 	case SC_SPACE:
>> 		{
>> -			if (object_id == 0)
>> -				return "SPACE";
>> 			struct space *space = space_by_id(object_id);
>> 			if (space == NULL)
>> 				break;
>> @@ -547,8 +555,6 @@ schema_find_name(enum schema_object_type type, uint32_t object_id)
>> 		}
>> 	case SC_FUNCTION:
>> 		{
>> -			if (object_id == 0)
>> -				return "FUNCTION";
>> 			struct func *func = func_by_id(object_id);
>> 			if (func == NULL)
>> 				break;
>> @@ -556,8 +562,6 @@ schema_find_name(enum schema_object_type type, uint32_t object_id)
>> 		}
>> 	case SC_SEQUENCE:
>> 		{
>> -			if (object_id == 0)
>> -				return "SEQUENCE";
>> 			struct sequence *seq = sequence_by_id(object_id);
>> 			if (seq == NULL)
>> 				break;
>> diff --git a/src/box/schema.h b/src/box/schema.h
>> index 0822262d0..f1735ff34 100644
>> --- a/src/box/schema.h
>> +++ b/src/box/schema.h
>> @@ -250,16 +250,19 @@ static inline
>> struct access *
>> entity_access_get(enum schema_object_type type)
>> {
>> -       switch (type) {
>> -       case SC_SPACE:
>> -               return entity_access.space;
>> -       case SC_FUNCTION:
>> -               return entity_access.function;
>> -       case SC_SEQUENCE:
>> -               return entity_access.sequence;
>> -       default:
>> -               return NULL;
>> -       }
>> +	switch (type) {
>> +	case SC_SPACE:
>> +	case SC_ENTITY_SPACE:
>> +		return entity_access.space;
>> +	case SC_FUNCTION:
>> +	case SC_ENTITY_FUNCTION:
>> +		return entity_access.function;
>> +	case SC_SEQUENCE:
>> +	case SC_ENTITY_SEQUENCE:
>> +		return entity_access.sequence;
>> +	default:
>> +		return NULL;
>> +	}
>> }
>> 
>> #endif /* INCLUDES_TARANTOOL_BOX_SCHEMA_H */
>> diff --git a/src/box/schema_def.c b/src/box/schema_def.c
>> index 97c074ab2..18ec6c8d2 100644
>> --- a/src/box/schema_def.c
>> +++ b/src/box/schema_def.c
>> @@ -31,16 +31,39 @@
>> #include "schema_def.h"
>> 
>> static const char *object_type_strs[] = {
>> -	/* [SC_UKNNOWN]         = */ "unknown",
>> -	/* [SC_UNIVERSE]        = */ "universe",
>> -	/* [SC_SPACE]           = */ "space",
>> -	/* [SC_FUNCTION]        = */ "function",
>> -	/* [SC_USER]            = */ "user",
>> -	/* [SC_ROLE]            = */ "role",
>> -	/* [SC_SEQUENCE]        = */ "sequence",
>> -	/* [SC_COLLATION]       = */ "collation",
>> +	/* [SC_UKNNOWN]		    = */ "unknown",
>> +	/* [SC_UNIVERSE]	    = */ "universe",
>> +	/* [SC_SPACE]		    = */ "space",
>> +	/* [SC_ENTITY_SPACE]	    = */ "space",
>> +	/* [SC_FUNCTION]	    = */ "function",
>> +	/* [SC_ENTITY_FUNCTION]	    = */ "function",
>> +	/* [SC_USER]		    = */ "user",
>> +	/* [SC_ENTITY_USER]	    = */ "user",
>> +	/* [SC_ROLE]		    = */ "role",
>> +	/* [SC_ENTITY_ROLE]	    = */ "role",
>> +	/* [SC_SEQUENCE]	    = */ "sequence",
>> +	/* [SC_ENTITY_SEQUENCE]	    = */ "sequence",
>> +	/* [SC_COLLATION]	    = */ "collation",
>> +	/* [SC_ENTITY_COLLATION]    = */ "collation",
> 
> I don't like this code duplication. Actually, I don't think that you
> need to have separate names for entity-wide privileges at all: AFAICS,
> schema_object_name() is never called for SC_ENTITY_*. Let's remove
> SC_ENTITY_* from this array.
> 
> If you do that, you won't be able to increment object type to get entity
> type. Well, OK, it's not that scary, taking into account the fact that
> you always use a helper function for that (see right below). I see it
> that way
> 
> enum schema_object_type {
> 	SC_UNKNOWN = 0,
> 	SC_UNIVERSE = 1,
> 	SC_SPACE = 2,
> 	SC_FUNCTION = 3,
> 	SC_USER = 4,
> 	SC_ROLE = 5,
> 	SC_SEQUENCE = 6,
> 	SC_COLLATION = 7,
> 	schema_object_type_MAX = 8
> 
> 	/* Entity types. */
> 	SC_ENTITY_SPACE,
> 	SC_ENTITY_FUNCTION,
> 	…
> }
> 

> enum schema_object_type
> schema_entity_type(enum schema_object_type type)
> {
> 	switch (type) {
> 	case SC_SPACE:
> 		return SC_ENTITY_SPACE;
> 	case SC_FUNCTION:
> 		return SC_ENTITY_FUNCTION;
> 	...
> 	}
> }
> 

Ok, switched to your variant with slight differences.

>> };
>> 
>> +enum schema_object_type
>> +schema_entity_type(enum schema_object_type type)
>> +{
>> +	switch(type) {
> 
>             ^^
>         space missing

Fixed.

> 
>> +	case SC_SPACE:
>> +	case SC_FUNCTION:
>> +	case SC_USER:
>> +	case SC_ROLE:
>> +	case SC_SEQUENCE:
>> +	case SC_COLLATION:
>> +		return type + 1;
>> +		break;
>> +	default:
>> +		unreachable();
>> +	}
>> +}
>> +
>> enum schema_object_type
>> schema_object_type(const char *name)
>> {
>> diff --git a/src/box/schema_def.h b/src/box/schema_def.h
>> index 2edb8d37f..9b5bd6864 100644
>> --- a/src/box/schema_def.h
>> +++ b/src/box/schema_def.h
>> @@ -218,19 +218,35 @@ enum {
>>  *
>>  * Use 0 for unknown to use the same index consistently
>>  * even when there are more object types in the future.
>> + *
>> + * When adding new types please follow this rule:
>> + * SC_ENTITY_NEW_OBJECT = SC_NEW_OBJECT + 1
>> + * schema_entity_type() relies on this convention.
>>  */
>> enum schema_object_type {
>> 	SC_UNKNOWN = 0,
>> 	SC_UNIVERSE = 1,
>> 	SC_SPACE = 2,
>> -	SC_FUNCTION = 3,
>> -	SC_USER = 4,
>> -	SC_ROLE = 5,
>> -	SC_SEQUENCE = 6,
>> -	SC_COLLATION = 7,
>> -	schema_object_type_MAX = 8
>> +	SC_ENTITY_SPACE = 3,
>> +	SC_FUNCTION = 4,
>> +	SC_ENTITY_FUNCTION = 5,
>> +	SC_USER = 6,
>> +	SC_ENTITY_USER = 7,
>> +	SC_ROLE = 8,
>> +	SC_ENTITY_ROLE = 9,
>> +	SC_SEQUENCE = 10,
>> +	SC_ENTITY_SEQUENCE = 11,
>> +	SC_COLLATION = 12,
>> +	SC_ENTITY_COLLATION = 13,
>> +	schema_object_type_MAX = 13
>> };
> 

---
 src/box/alter.cc             |  27 +++++++++++++++++++-
 src/box/bootstrap.snap       | Bin 1540 -> 1556 bytes
 src/box/lua/schema.lua       |  58 ++++++++++++++++++++++++++-----------------
 src/box/lua/upgrade.lua      |  23 +++++++++++++++++
 src/box/schema.cc            |  11 ++++----
 src/box/schema.h             |  23 +++++++++--------
 src/box/schema_def.c         |   7 ++++++
 src/box/schema_def.h         |  18 +++++++++++++-
 src/box/user.cc              |  27 +++++++++++---------
 test/box-py/bootstrap.result |  14 +++++------
 test/box/access.result       |   6 ++---
 test/box/access_misc.result  |   8 +++---
 test/box/alter.result        |   8 +++---
 test/xlog/upgrade.result     |  14 +++++------
 14 files changed, 166 insertions(+), 78 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 3007a131d..f586a2695 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2537,10 +2537,35 @@ priv_def_create_from_tuple(struct priv_def *priv, struct tuple *tuple)
 {
 	priv->grantor_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_ID);
 	priv->grantee_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_UID);
+
 	const char *object_type =
 		tuple_field_cstr_xc(tuple, BOX_PRIV_FIELD_OBJECT_TYPE);
-	priv->object_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_OBJECT_ID);
 	priv->object_type = schema_object_type(object_type);
+
+	const char *data = tuple_field(tuple, BOX_PRIV_FIELD_OBJECT_ID);
+	if (data == NULL) {
+	    tnt_raise(ClientError, ER_NO_SUCH_FIELD,
+		      BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE);
+	}
+	/*
+	 * When granting or revoking privileges on a whole entity
+	 * we pass empty string ('') to object_id to indicate
+	 * grant on every object of that entity.
+	 * So check for that first.
+	 */
+	switch (mp_typeof(*data)) {
+	case MP_STR:
+		if (mp_decode_strl(&data) == 0) {
+			/* Entity-wide privilege. */
+			priv->object_id = 0;
+			priv->object_type = schema_entity_type(priv->object_type);
+			break;
+		}
+		FALLTHROUGH;
+	default:
+		priv->object_id = tuple_field_u32_xc(tuple,
+						     BOX_PRIV_FIELD_OBJECT_ID);
+	}
 	if (priv->object_type == SC_UNKNOWN) {
 		tnt_raise(ClientError, ER_UNKNOWN_SCHEMA_OBJECT,
 			  object_type);
diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
index b610828c9c9ae9a22acdd8c150c16c6838b7a273..44992b050e9d73d69608fb92782d1e965f6cdc51 100644
GIT binary patch
delta 1551
zcmV+q2JrcW43rFz7=JM>GB!9ZXE|m$WMeloGhzx!ZgX^DZewLSAT%>EG&o@}VJ$c{
zGcheRV>UP~VPZ03EipATVq!NoIX7cCWeQe9Y;R+0Iv{&}3JTS_3%bn(nE=k-$Bkd5
z0000004TLD{QyvfEC41w*h<hAaRLBeJb=_J{}F0FAgJ*mVt>V;#?6&k&og$Aul<dv
zv3Dv(N-{sS_S_R$=If$P=IFJ-Qc5dSF3)wMv;i7#PyNg>UnEmX0mA^f0L=io5sT!g
zD{Fnf#vxPKYgy*n?1AMVIy{=p9gia^hC=L0fWLuz8rC_P&bxEnm$jbL+0jvRI_+sG
zru|Ih;m)d9Ab%fU2!MAhTHBbGce$H&nf~2*&yFmHLT-w8E2;yCj1eht_F<%$wYjrA
z--_qmirxZNlbP>7cRjpY(I5*1%V7?t>8y7vDuOB;TVebwm5d3mRG{g<l`_3ftd)rc
zn_;zHClt(+{hgbmxU+pKw$>&pI!j<g$w35)6B;Hdw}0vc|76#A%XVkpWtdu!!EHY`
zQ473V(OEa~Z9g}|)Sgs&h}w*uq0W{Rc(<Zvf8ImXW-LrCDFM%iVB<2)IG&t`=^X0p
z%Yh$fD8!Boc(<bFh|kp4F)S=%TMBr$q8k55lCM#${Y=DS+Bfbz&A~d;aQkK7b>5ZV
zdG3dP6Mx6^(8sb6!1;Xg8sDAa&WZ-$y>W_Zn~dpXc#Nf8o1_@R#?P@Ion`uUK3>En
z4|O&qz`GSKF%8FiNe275855I3zK&RX`$!^s><!J0t0shXfpC3jd1!TLaj@ww*s37t
z-HLiP9v};~La9)x)67Zcgesj%8+J-DBoj%4cYiCoL(1@BuzjT$lI`Wf$ik??2xi9d
ztc3#01eTBGMdTtHiCXRru~^$q5*v~bieRnCyA@ptRojk|H*sWb>UR9^o~tSri06BO
z0Q-*j>z9Q*4+(#G7_m1GyjxLQ$D%fO4}ENn1MgPUw1qv^wQ}ZNwi<Z1qOu?JnRn@f
zM}I7Goln@sxA7u>0uPF;){6CdrBo=>OXh`5;7UxHP%|pfgijbup=MGrPps{8(Quhz
zWw^YXTN9%N1NEZh;%hNtVMQxeEKqt?SWV_GYBSbz?d=0?f`WpY*c$IvG*qAOKeIM_
zQh2wbDYx3&2cp<tnmn~GC&WCKzkjS};(teS=fpg|%u;9X?K_pwnFhnDwIsY-(GRlN
ztgP{o#>Rx0;YE$dsj=v%(gx?x*@*;&XWUfcndq})KR0uozbQ*?y?v(<-mNH&3>7#_
z#&nJefg`BVSY%8M%m4s@04M+v21hAoRSyz?&^U^NFbZH021E!rh)xs$fZ#w_L4OfM
zEf@qq49vQeA))6t$8JsxhygJm2AP2;6jR1Ytzq>yuq)+j_YBuXSm^>oOc44oRR2+r
zfKn$8@9xdDJsnXgj8fvJX-OOEnOz2N82f7Vr6^tAcX*WFIPM{0Ntj!pLMS~2mv!oE
zRrM)J%oMN{r_N{V(s7_ah4$kjihs$6<OnC-Xy|vNdloebHTnit@EUCcD|n6OjV)em
zPC$*u2HNnREZ$rv)082si4@Jx$$z3_Ifq*d0kp^Vv;;5qa1a|JkDvz!Y7ZzgZLtTG
znKqel_G}Nk_kmTD8UY2Zw|;97AY4LmR2{?0eUDLb#Sq+kdJS#-4;^$U>3_%%=)mhn
z2W|IQc+vMG<^-d?>WOsJ?wF5DwFGHVZ;5{eV6!P9un202v<JSzI-4XFWp2pfJ=}&9
z|B3!R7$^i;Ur3=7lANu}q6eAjK|>w!Z8R4d19B91M{)$#!E>qp7jG6HO>MyhU^@4T
z^f(MLmIX$FanP|HqB(=3bAPgI29$n(?koqvkkh_+Jdt&9kemV%zz>2&MdX*2g^(0B
zb3fr-G#O-Ux`l`P)LF58aj(U8%fMRlqE2MOof#G*F;Vmq(Um!gfT;c#$##_N2vs0b
zGn`~?u8)RY;W>4D58f)EMzh}yokR<1H8AM^(7KO=Ygngs1QB1+=m<%o!R6Hut?jEW
B)aL*I

delta 1534
zcmV<a1p)e$41^4j7=JJ=G%zh^V>dT4F*z~{Np5p=VQyn(Iv_b=V_`LAF*YqTV`gS8
zG-hTvEnzrjFfCy?W;Qf4VP!dGGGq!?Lu_wjYdRo%F*+bOeF_TIx(m9^1&9F7A|rpb
zr2qf`001bpFZ}>e{VM<lJ=RLl7I6XqUp$Ch%_7lYJ_Lj_@PFoXP~+we(A76EO7~<&
z)F3;RA|;uhw(`4WgTRw>I&<{eU?~Ai$VOI~v629dx2JaIm@kx4N&&zCxB$xlBK}6v
zM#}nreKVfmYggvF>;bcooE<&pjzw|&ydgCd;B)Xi4C|Up=iRyONm;Mu=<J|49rmyj
z!~XSg?^aVP(0>n31mId~$~K1OP2OJJrGIzc!y|v*5Q}mxHFpG|K|%#jeQ^A;E^l>*
z3*Bp}NefgxX1+gm^KmUTgDMc1Jsb?P)oZC4!mL!?OgGcvU!7uNP_0u4V%4c@$$Tde
z<JJuBKo=+bJ1@oWHsew%rA$(CR4!bhCq$aiEJ3j}Cx2ttc)M<~yh-n@9Ru2aUZxgY
zOU+j=F>OCDy|bcHe0;hLW1g*=6kJQqvOo9n=`!@rT1v3PA=sEaGtUmrgF1V*+HwF$
z%o|ck2Ck)MDdN~U>lpNk)Rh9)Qd8p>$FU`TvVVQ}JNw3+pEy`&7|(v!cb#wKcU}Xc
zztr*E^M9!-1a3aZxQyRo-mQ)X;JxvQVS5a1E-Z#;*X8(mp!hlb(b=Wn_TxWHa?e&p
z0<NVdiD5jvlVdHR&M*6U`SOA@wvN<F<4PlXstt_|t42g+g<yeLeOP%|b(q;~x1wEl
zEj90~M>ms_NyVg8C=MtXRLX=hZO}2jlujiL*MCw|r<8%iFyl-urCG}bk_A!)lFN(Z
z*9rxd2`ZdQ%ZOz}8m(9>QmHeYBvm9q6rtA9wbTrW=uAh+mpZa8Z9M*WuhEnWw8OPP
z0Ken?`duE+M1Fc0sWuO;rRJ>TPnWxUKDEZdwbU$UUQcC(VtJFR2G>&aT`%TX-lUJ$
zzkkQk8h`!+Ak42Gb5O~I9X_GL4xhR(RGq@`<8YEmeGLBa%Ed{YaV#4wH7X6(wXrob
zRx(mCOfa|?Bo}qm;#z9jPz5?Ii>k-mKV61Bw$iw6MNm)>lUj2vHAVFK{#e#!kBV!l
ziDGG`ab1f2WXaFUg5t{}`TM8d6EBXt_J3uuRg$)9X&lOg&M*+4wU)S+njl=^i<EJZ
zX2ppwLyQ}LBXiMF<cgb*y<$jG_{C2oetkYX_VcpV`IoM<QW}RMaV<4zXt3a^V}T&3
z(OhIq3CsWhfB+}}AqGb&W<?JYfWSD4qc93!7zRWLI0_9I0RX{)ke~>l77Rj$ynhqj
zxeG%W18Ui|uoin^FOJH&6BE><l!R4e54%@5i^3tJ2=RjuhoRy}$qP!PWxP;CvBe2+
zB`AQ$m7GU4)NS@llAFhZ>1-;h#Us!zr2@yPWOP}$xk=>TS*Ej6Q8ZgBx&}sXh}Wym
z;Yvs=Xm6s&oTIkS5VXc28)t^2+kc4qR4r1#;*gdpU~^0s$pG8pm?G)gVGer7p5*j@
zrQSOro_cGm&fR}<@b8h^=iXW!%4reE=RqJFW<g443Z&4$8`^`Azzc12c=p(Su(w)T
z1)jmCmtK6UOn@l~=HxoFllhK1;>sqt`1BcS0HC@As3ba#z^Ko9Pi8avYk$Oa!!S#&
zSx3_!A8xyrpfT#3Y0zk_Hi}Y>p%$pI61F+5M3N&%2xKJAC;$CDD}<6?=$#5uoTZ5Z
zCNj}EgB|fKv=$L@ag>MEJ_76DnN*W1Z`O`MZ9ptwx|K1yxMV1V1(Ltx%(4y7obk4E
zW?=^8GIQ=kv4A0`wS7N1*>`Z!opukPx!_g@c{8|RJi<`!QS!z#<&+X#nnV8V6uo^%
zFL^|hfwSZ-+|LAZ&d5i?Zt>-$tMH_1UnT!Yb{@SussOv@WfJ50JyfLCQd9Bw_N-dh
k2=V>Mm|$S01}gnuSl5<E4okR>OyKJnJxL~`9Mur5?adb1g8%>k

diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index b9b8c9004..d347fd1e6 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -1801,16 +1801,16 @@ local function privilege_name(privilege)
 end
 
 local function object_resolve(object_type, object_name)
+    if object_name ~= nil and type(object_name) ~= 'string'
+            and type(object_name) ~= 'number' then
+        box.error(box.error.ILLEGAL_PARAMS, "wrong object name type")
+    end
     if object_type == 'universe' then
-        if object_name ~= nil and type(object_name) ~= 'string'
-                and type(object_name) ~= 'number' then
-            box.error(box.error.ILLEGAL_PARAMS, "wrong object name type")
-        end
         return 0
     end
     if object_type == 'space' then
-        if object_name == nil or object_name == 0 then
-            return 0
+        if object_name == '' then
+            return ''
         end
         local space = box.space[object_name]
         if  space == nil then
@@ -1819,8 +1819,8 @@ local function object_resolve(object_type, object_name)
         return space.id
     end
     if object_type == 'function' then
-        if object_name == nil or object_name == 0 then
-            return 0
+        if object_name == '' then
+            return ''
         end
         local _vfunc = box.space[box.schema.VFUNC_ID]
         local func
@@ -1836,8 +1836,8 @@ local function object_resolve(object_type, object_name)
         end
     end
     if object_type == 'sequence' then
-        if object_name == nil or object_name == 0 then
-            return 0
+        if object_name == '' then
+            return ''
         end
         local seq = sequence_resolve(object_name)
         if seq == nil then
@@ -1864,7 +1864,7 @@ local function object_resolve(object_type, object_name)
 end
 
 local function object_name(object_type, object_id)
-    if object_type == 'universe' then
+    if object_type == 'universe' or object_id == '' then
         return ""
     end
     local space
@@ -2072,12 +2072,18 @@ local function grant(uid, name, privilege, object_type,
                      object_name, options)
     -- From user point of view, role is the same thing
     -- as a privilege. Allow syntax grant(user, role).
-    if object_name == nil and object_type == nil then
-        -- sic: avoid recursion, to not bother with roles
-        -- named 'execute'
-        object_type = 'role'
-        object_name = privilege
-        privilege = 'execute'
+    if object_name == nil then
+        if object_type == nil then
+            -- sic: avoid recursion, to not bother with roles
+            -- named 'execute'
+            object_type = 'role'
+            object_name = privilege
+            privilege = 'execute'
+        else
+            -- Allow syntax grant(user, priv, entity)
+            -- for entity grants.
+            object_name = ''
+        end
     end
     local privilege_hex = privilege_check(privilege, object_type)
 
@@ -2117,10 +2123,16 @@ end
 local function revoke(uid, name, privilege, object_type, object_name, options)
     -- From user point of view, role is the same thing
     -- as a privilege. Allow syntax revoke(user, role).
-    if object_name == nil and object_type == nil then
-        object_type = 'role'
-        object_name = privilege
-        privilege = 'execute'
+    if object_name == nil then
+        if object_type == nil then
+            object_type = 'role'
+            object_name = privilege
+            privilege = 'execute'
+        else
+            -- Allow syntax revoke(user, privilege, entity)
+            -- to revoke entity privileges.
+            object_name = ''
+        end
     end
     local privilege_hex = privilege_check(privilege, object_type)
     options = options or {}
@@ -2192,8 +2204,8 @@ local function drop(uid, opts)
     local privs = _vpriv.index.primary:select{uid}
 
     for k, tuple in pairs(privs) do
-	-- we need an additional box.session.su() here, because of
-	-- unnecessary check for privilege PRIV_REVOKE in priv_def_check()
+        -- we need an additional box.session.su() here, because of
+        -- unnecessary check for privilege PRIV_REVOKE in priv_def_check()
         box.session.su("admin", revoke, uid, uid, tuple[5], tuple[3], tuple[4])
     end
     box.space[box.schema.USER_ID]:delete{uid}
diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index 0293f6ef8..091da2dc4 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -964,6 +964,28 @@ local function upgrade_to_1_10_0()
     create_vsequence_space()
 end
 
+--------------------------------------------------------------------------------
+--- Tarantool 1.10.2
+--------------------------------------------------------------------------------
+local function upgrade_space_priv_to_1_10_2()
+    local _priv = box.space._priv
+    local _vpriv = box.space._vpriv
+    local f = _priv:format()
+
+    f[4].type = 'scalar'
+    _priv:format(f)
+    f = _vpriv:format()
+    f[4].type = 'scalar'
+    _vpriv:format(f)
+    _priv.index.primary:alter{parts={2, 'unsigned', 3, 'string', 4, 'scalar'}}
+    _vpriv.index.primary:alter{parts={2, 'unsigned', 3, 'string', 4, 'scalar'}}
+    _priv.index.object:alter{parts={3, 'string', 4, 'scalar'}}
+    _vpriv.index.object:alter{parts={3, 'string', 4, 'scalar'}}
+end
+
+local function upgrade_to_1_10_2()
+    upgrade_space_priv_to_1_10_2()
+end
 
 local function get_version()
     local version = box.space._schema:get{'version'}
@@ -991,6 +1013,7 @@ local function upgrade(options)
         {version = mkversion(1, 7, 6), func = upgrade_to_1_7_6, auto = false},
         {version = mkversion(1, 7, 7), func = upgrade_to_1_7_7, auto = true},
         {version = mkversion(1, 10, 0), func = upgrade_to_1_10_0, auto = true},
+        {version = mkversion(1, 10, 2), func = upgrade_to_1_10_2, auto = true},
     }
 
     for _, handler in ipairs(handlers) do
diff --git a/src/box/schema.cc b/src/box/schema.cc
index 433f52c08..4502ca6dc 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -535,11 +535,14 @@ schema_find_name(enum schema_object_type type, uint32_t object_id)
 {
 	switch (type) {
 	case SC_UNIVERSE:
+	case SC_ENTITY_SPACE:
+	case SC_ENTITY_FUNCTION:
+	case SC_ENTITY_SEQUENCE:
+	case SC_ENTITY_ROLE:
+	case SC_ENTITY_USER:
 		return "";
 	case SC_SPACE:
 		{
-			if (object_id == 0)
-				return "SPACE";
 			struct space *space = space_by_id(object_id);
 			if (space == NULL)
 				break;
@@ -547,8 +550,6 @@ schema_find_name(enum schema_object_type type, uint32_t object_id)
 		}
 	case SC_FUNCTION:
 		{
-			if (object_id == 0)
-				return "FUNCTION";
 			struct func *func = func_by_id(object_id);
 			if (func == NULL)
 				break;
@@ -556,8 +557,6 @@ schema_find_name(enum schema_object_type type, uint32_t object_id)
 		}
 	case SC_SEQUENCE:
 		{
-			if (object_id == 0)
-				return "SEQUENCE";
 			struct sequence *seq = sequence_by_id(object_id);
 			if (seq == NULL)
 				break;
diff --git a/src/box/schema.h b/src/box/schema.h
index 0822262d0..f1735ff34 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -250,16 +250,19 @@ static inline
 struct access *
 entity_access_get(enum schema_object_type type)
 {
-       switch (type) {
-       case SC_SPACE:
-               return entity_access.space;
-       case SC_FUNCTION:
-               return entity_access.function;
-       case SC_SEQUENCE:
-               return entity_access.sequence;
-       default:
-               return NULL;
-       }
+	switch (type) {
+	case SC_SPACE:
+	case SC_ENTITY_SPACE:
+		return entity_access.space;
+	case SC_FUNCTION:
+	case SC_ENTITY_FUNCTION:
+		return entity_access.function;
+	case SC_SEQUENCE:
+	case SC_ENTITY_SEQUENCE:
+		return entity_access.sequence;
+	default:
+		return NULL;
+	}
 }
 
 #endif /* INCLUDES_TARANTOOL_BOX_SCHEMA_H */
diff --git a/src/box/schema_def.c b/src/box/schema_def.c
index 97c074ab2..bbae046c3 100644
--- a/src/box/schema_def.c
+++ b/src/box/schema_def.c
@@ -41,6 +41,13 @@ static const char *object_type_strs[] = {
 	/* [SC_COLLATION]       = */ "collation",
 };
 
+enum schema_object_type
+schema_entity_type(enum schema_object_type type)
+{
+	assert((int) type < (int) schema_object_type_MAX);
+	return type + schema_object_type_MAX - 1;
+}
+
 enum schema_object_type
 schema_object_type(const char *name)
 {
diff --git a/src/box/schema_def.h b/src/box/schema_def.h
index 2edb8d37f..5ece809f1 100644
--- a/src/box/schema_def.h
+++ b/src/box/schema_def.h
@@ -218,6 +218,10 @@ enum {
  *
  * Use 0 for unknown to use the same index consistently
  * even when there are more object types in the future.
+ *
+ * When adding new types please keep the
+ * same order between objects and corresponding entity types.
+ * schema_entity_type() relies on this convention.
  */
 enum schema_object_type {
 	SC_UNKNOWN = 0,
@@ -228,9 +232,21 @@ enum schema_object_type {
 	SC_ROLE = 5,
 	SC_SEQUENCE = 6,
 	SC_COLLATION = 7,
-	schema_object_type_MAX = 8
+	schema_object_type_MAX = 8,
+	SC_ENTITY_SPACE,
+	SC_ENTITY_FUNCTION,
+	SC_ENTITY_USER,
+	SC_ENTITY_ROLE,
+	SC_ENTITY_SEQUENCE,
+	SC_ENTITY_COLLATION
 };
 
+/**
+ * Given a object type, return an entity type it belongs to.
+ */
+enum schema_object_type
+schema_entity_type(enum schema_object_type type);
+
 enum schema_object_type
 schema_object_type(const char *name);
 
diff --git a/src/box/user.cc b/src/box/user.cc
index fbf06566a..eec785652 100644
--- a/src/box/user.cc
+++ b/src/box/user.cc
@@ -207,12 +207,23 @@ access_find(struct priv_def *priv)
 		access = universe.access;
 		break;
 	}
+	case SC_ENTITY_SPACE:
+	{
+		access = entity_access.space;
+		break;
+	}
+	case SC_ENTITY_FUNCTION:
+	{
+		access = entity_access.function;
+		break;
+	}
+	case SC_ENTITY_SEQUENCE:
+	{
+		access = entity_access.sequence;
+		break;
+	}
 	case SC_SPACE:
 	{
-		if (priv->object_id == 0) {
-			access = entity_access.space;
-			break;
-		}
 		struct space *space = space_by_id(priv->object_id);
 		if (space)
 			access = space->access;
@@ -220,10 +231,6 @@ access_find(struct priv_def *priv)
 	}
 	case SC_FUNCTION:
 	{
-		if (priv->object_id == 0) {
-			access = entity_access.function;
-			break;
-		}
 		struct func *func = func_by_id(priv->object_id);
 		if (func)
 			access = func->access;
@@ -231,10 +238,6 @@ access_find(struct priv_def *priv)
 	}
 	case SC_SEQUENCE:
 	{
-		if (priv->object_id == 0) {
-			access = entity_access.sequence;
-			break;
-		}
 		struct sequence *seq = sequence_by_id(priv->object_id);
 		if (seq)
 			access = seq->access;
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index 16c2027cf..cf8242de5 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -5,7 +5,7 @@ box.space._schema:select{}
 ---
 - - ['cluster', '<cluster uuid>']
   - ['max_id', 511]
-  - ['version', 1, 10, 0]
+  - ['version', 1, 10, 2]
 ...
 box.space._cluster:select{}
 ---
@@ -58,10 +58,10 @@ box.space._space:select{}
         'type': 'string'}, {'name': 'auth', 'type': 'map'}]]
   - [312, 1, '_priv', 'memtx', 0, {}, [{'name': 'grantor', 'type': 'unsigned'}, {
         'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
-      {'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
+      {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
   - [313, 1, '_vpriv', 'sysview', 0, {}, [{'name': 'grantor', 'type': 'unsigned'},
       {'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
-      {'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
+      {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
   - [320, 1, '_cluster', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'uuid',
         'type': 'string'}]]
   - [330, 1, '_truncate', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'count',
@@ -104,13 +104,13 @@ box.space._index:select{}
   - [305, 1, 'owner', 'tree', {'unique': false}, [[1, 'unsigned']]]
   - [305, 2, 'name', 'tree', {'unique': true}, [[2, 'string']]]
   - [312, 0, 'primary', 'tree', {'unique': true}, [[1, 'unsigned'], [2, 'string'],
-      [3, 'unsigned']]]
+      [3, 'scalar']]]
   - [312, 1, 'owner', 'tree', {'unique': false}, [[0, 'unsigned']]]
-  - [312, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'unsigned']]]
+  - [312, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'scalar']]]
   - [313, 0, 'primary', 'tree', {'unique': true}, [[1, 'unsigned'], [2, 'string'],
-      [3, 'unsigned']]]
+      [3, 'scalar']]]
   - [313, 1, 'owner', 'tree', {'unique': false}, [[0, 'unsigned']]]
-  - [313, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'unsigned']]]
+  - [313, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'scalar']]]
   - [320, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
   - [320, 1, 'uuid', 'tree', {'unique': true}, [[1, 'string']]]
   - [330, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
diff --git a/test/box/access.result b/test/box/access.result
index f4669a4a3..599500633 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -504,7 +504,7 @@ box.space._priv:select{id}
 ...
 box.schema.user.grant('user', 'read', 'universe')
 ---
-- error: User 'user' already has read access on universe 'nil'
+- error: User 'user' already has read access on universe ''
 ...
 box.space._priv:select{id}
 ---
@@ -690,7 +690,7 @@ box.schema.user.grant('guest', 'read,write,execute', 'universe')
 ...
 box.schema.user.grant('guest', 'read,write,execute', 'universe')
 ---
-- error: User 'guest' already has read,write,execute access on universe 'nil'
+- error: User 'guest' already has read,write,execute access on universe ''
 ...
 box.schema.user.grant('guest', 'read,write,execute', 'universe', '', { if_not_exists = true })
 ---
@@ -703,7 +703,7 @@ box.schema.user.revoke('guest', 'usage,session', 'universe')
 ...
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 ---
-- error: User 'guest' does not have read,write,execute access on universe 'nil'
+- error: User 'guest' does not have read,write,execute access on universe ''
 ...
 box.schema.user.revoke('guest', 'read,write,execute', 'universe', '', { if_exists = true })
 ---
diff --git a/test/box/access_misc.result b/test/box/access_misc.result
index 2d87fa2d5..3061f1181 100644
--- a/test/box/access_misc.result
+++ b/test/box/access_misc.result
@@ -436,7 +436,7 @@ box.schema.user.revoke('testuser', 'usage,session', 'universe')
 ...
 box.schema.user.revoke('testuser', 'read, write, execute', 'universe')
 ---
-- error: User 'testuser' does not have read, write, execute access on universe 'nil'
+- error: User 'testuser' does not have read, write, execute access on universe ''
 ...
 box.schema.user.revoke('testuser', 'create', 'universe')
 ---
@@ -797,10 +797,10 @@ box.space._space:select()
         'type': 'string'}, {'name': 'auth', 'type': 'map'}]]
   - [312, 1, '_priv', 'memtx', 0, {}, [{'name': 'grantor', 'type': 'unsigned'}, {
         'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
-      {'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
+      {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
   - [313, 1, '_vpriv', 'sysview', 0, {}, [{'name': 'grantor', 'type': 'unsigned'},
       {'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
-      {'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
+      {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
   - [320, 1, '_cluster', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'uuid',
         'type': 'string'}]]
   - [330, 1, '_truncate', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'count',
@@ -853,7 +853,7 @@ box.schema.user.grant('tester', 'read', 'universe')
 -- error: the privilege is not granted
 box.schema.user.revoke('tester', 'create', 'universe')
 ---
-- error: User 'tester' does not have create access on universe 'nil'
+- error: User 'tester' does not have create access on universe ''
 ...
 -- no error: if_exists clause
 box.schema.user.revoke('tester', 'create', 'universe', nil, { if_exists = true })
diff --git a/test/box/alter.result b/test/box/alter.result
index eb7014d8b..36bdb5fd3 100644
--- a/test/box/alter.result
+++ b/test/box/alter.result
@@ -214,13 +214,13 @@ _index:select{}
   - [305, 1, 'owner', 'tree', {'unique': false}, [[1, 'unsigned']]]
   - [305, 2, 'name', 'tree', {'unique': true}, [[2, 'string']]]
   - [312, 0, 'primary', 'tree', {'unique': true}, [[1, 'unsigned'], [2, 'string'],
-      [3, 'unsigned']]]
+      [3, 'scalar']]]
   - [312, 1, 'owner', 'tree', {'unique': false}, [[0, 'unsigned']]]
-  - [312, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'unsigned']]]
+  - [312, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'scalar']]]
   - [313, 0, 'primary', 'tree', {'unique': true}, [[1, 'unsigned'], [2, 'string'],
-      [3, 'unsigned']]]
+      [3, 'scalar']]]
   - [313, 1, 'owner', 'tree', {'unique': false}, [[0, 'unsigned']]]
-  - [313, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'unsigned']]]
+  - [313, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'scalar']]]
   - [320, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
   - [320, 1, 'uuid', 'tree', {'unique': true}, [[1, 'string']]]
   - [330, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
diff --git a/test/xlog/upgrade.result b/test/xlog/upgrade.result
index f02996bba..76467baf1 100644
--- a/test/xlog/upgrade.result
+++ b/test/xlog/upgrade.result
@@ -36,7 +36,7 @@ box.space._schema:select()
 ---
 - - ['cluster', '<server_uuid>']
   - ['max_id', 513]
-  - ['version', 1, 10, 0]
+  - ['version', 1, 10, 2]
 ...
 box.space._space:select()
 ---
@@ -85,10 +85,10 @@ box.space._space:select()
         'type': 'string'}, {'name': 'auth', 'type': 'map'}]]
   - [312, 1, '_priv', 'memtx', 0, {}, [{'name': 'grantor', 'type': 'unsigned'}, {
         'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
-      {'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
+      {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
   - [313, 1, '_vpriv', 'sysview', 0, {}, [{'name': 'grantor', 'type': 'unsigned'},
       {'name': 'grantee', 'type': 'unsigned'}, {'name': 'object_type', 'type': 'string'},
-      {'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
+      {'name': 'object_id', 'type': 'scalar'}, {'name': 'privilege', 'type': 'unsigned'}]]
   - [320, 1, '_cluster', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'uuid',
         'type': 'string'}]]
   - [330, 1, '_truncate', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'count',
@@ -134,13 +134,13 @@ box.space._index:select()
   - [305, 1, 'owner', 'tree', {'unique': false}, [[1, 'unsigned']]]
   - [305, 2, 'name', 'tree', {'unique': true}, [[2, 'string']]]
   - [312, 0, 'primary', 'tree', {'unique': true}, [[1, 'unsigned'], [2, 'string'],
-      [3, 'unsigned']]]
+      [3, 'scalar']]]
   - [312, 1, 'owner', 'tree', {'unique': false}, [[0, 'unsigned']]]
-  - [312, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'unsigned']]]
+  - [312, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'scalar']]]
   - [313, 0, 'primary', 'tree', {'unique': true}, [[1, 'unsigned'], [2, 'string'],
-      [3, 'unsigned']]]
+      [3, 'scalar']]]
   - [313, 1, 'owner', 'tree', {'unique': false}, [[0, 'unsigned']]]
-  - [313, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'unsigned']]]
+  - [313, 2, 'object', 'tree', {'unique': false}, [[2, 'string'], [3, 'scalar']]]
   - [320, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
   - [320, 1, 'uuid', 'tree', {'unique': true}, [[1, 'string']]]
   - [330, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
-- 
2.15.2 (Apple Git-101.1)

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [tarantool-patches] Re: [PATCH v2] Introduce separate entity object types for entity privileges.
  2018-08-17 12:19       ` [tarantool-patches] " Serge Petrenko
@ 2018-08-17 15:57         ` Vladimir Davydov
  0 siblings, 0 replies; 6+ messages in thread
From: Vladimir Davydov @ 2018-08-17 15:57 UTC (permalink / raw)
  To: Serge Petrenko; +Cc: Konstantin Osipov, tarantool-patches

On Fri, Aug 17, 2018 at 03:19:29PM +0300, Serge Petrenko wrote:
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 3007a131d..f586a2695 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -2537,10 +2537,35 @@ priv_def_create_from_tuple(struct priv_def *priv, struct tuple *tuple)
>  {
>  	priv->grantor_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_ID);
>  	priv->grantee_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_UID);
> +
>  	const char *object_type =
>  		tuple_field_cstr_xc(tuple, BOX_PRIV_FIELD_OBJECT_TYPE);
> -	priv->object_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_OBJECT_ID);
>  	priv->object_type = schema_object_type(object_type);
> +
> +	const char *data = tuple_field(tuple, BOX_PRIV_FIELD_OBJECT_ID);
> +	if (data == NULL) {
> +	    tnt_raise(ClientError, ER_NO_SUCH_FIELD,

Bad indentation - should be a tab here. Please fix.

> + *
> + * When adding new types please keep the
> + * same order between objects and corresponding entity types.
> + * schema_entity_type() relies on this convention.
>   */
>  enum schema_object_type {
>  	SC_UNKNOWN = 0,
> @@ -228,9 +232,21 @@ enum schema_object_type {
>  	SC_ROLE = 5,
>  	SC_SEQUENCE = 6,
>  	SC_COLLATION = 7,
> -	schema_object_type_MAX = 8
> +	schema_object_type_MAX = 8,

Please add a comment here that says that below this point only entity
types are supposed to be defined.

> +	SC_ENTITY_SPACE,
> +	SC_ENTITY_FUNCTION,
> +	SC_ENTITY_USER,
> +	SC_ENTITY_ROLE,
> +	SC_ENTITY_SEQUENCE,
> +	SC_ENTITY_COLLATION
>  };

> +enum schema_object_type
> +schema_entity_type(enum schema_object_type type)
> +{
> +	assert((int) type < (int) schema_object_type_MAX);
> +	return type + schema_object_type_MAX - 1;
> +}

No. Too fragile - easy to make a mistake when adding a new type.
And this -1 looks suspicious. Kostja isn't going to like it.

I vote for rewriting this code with a simple switch-case.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-08-17 15:57 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-02 10:55 [tarantool-patches] [PATCH v2] Introduce separate entity object types for entity privileges Serge Petrenko
2018-08-07 16:38 ` Vladimir Davydov
2018-08-14 13:41   ` Serge Petrenko
2018-08-17  9:19     ` Vladimir Davydov
2018-08-17 12:19       ` [tarantool-patches] " Serge Petrenko
2018-08-17 15:57         ` Vladimir Davydov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox