Re: [Tarantool-patches] [PATCH] box: user.grant error should be versatile

["Maria Khaydich" <maria.khaydich@tarantool.org>]

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


Thank you for the review. The fix is done.
 
----------------------------------------------------------------------
Error message on granted privileges was not flexible and
did not distinguish between universal or any other privileges
leaving either 'nil' or simply '' at the end.
 
Closes #714
---
 Issue:
  https://github.com/tarantool/tarantool/issues/714  
 Branch:
  https://github.com/tarantool/tarantool/tree/eljashm/gh-714-box-schema-user-grant-invalid-error  

 src/box/errcode.h        |  2 +-
 src/box/lua/schema.lua   |  3 +++
 test/box/access.result   | 30 ++++++++++++++++++++++++++++--
 test/box/access.test.lua | 14 ++++++++++++++
 4 files changed, 46 insertions(+), 3 deletions(-)
diff --git a/src/box/errcode.h b/src/box/errcode.h
index 6f6e28c6c..cac8447e2 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -141,7 +141,7 @@ struct errcode_record {
     /* 86 */_(ER_SESSION_CLOSED,        "Session is closed") \
     /* 87 */_(ER_ROLE_LOOP,            "Granting role '%s' to role '%s' would create a loop") \
     /* 88 */_(ER_GRANT,            "Incorrect grant arguments: %s") \
-    /* 89 */_(ER_PRIV_GRANTED,        "User '%s' already has %s access on %s '%s'") \
+    /* 89 */_(ER_PRIV_GRANTED,        "User '%s' already has %s access on %s%s") \
     /* 90 */_(ER_ROLE_GRANTED,        "User '%s' already has role '%s'") \
     /* 91 */_(ER_PRIV_NOT_GRANTED,        "User '%s' does not have %s access on %s '%s'") \
     /* 92 */_(ER_ROLE_NOT_GRANTED,        "User '%s' does not have role '%s'") \
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 50c96a335..f537c3cec 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -2408,6 +2408,9 @@ local function grant(uid, name, privilege, object_type,
                privilege == 'execute' then
                 box.error(box.error.ROLE_GRANTED, name, object_name)
             else
+                if object_type ~= 'universe' then
+                    object_name = string.format(" '%s'", object_name)
+                end
                 box.error(box.error.PRIV_GRANTED, name, privilege,
                           object_type, object_name)
             end
diff --git a/test/box/access.result b/test/box/access.result
index 9554991ad..be8b1c521 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -532,7 +532,7 @@ box.space._priv:select{id}
 ...
 box.schema.user.grant('user', 'read', 'universe')
 ---
-- error: User 'user' already has read access on universe ''
+- error: User 'user' already has read access on universe
 ...
 box.space._priv:select{id}
 ---
@@ -738,7 +738,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 ''
+- error: User 'guest' already has read,write,execute access on universe
 ...
 box.schema.user.grant('guest', 'read,write,execute', 'universe', '', { if_not_exists = true })
 ---
@@ -2108,3 +2108,29 @@ box.space._priv:delete{1, 'universe', 0}
 ---
 - error: 'Incorrect grant arguments: can''t revoke universe from the admin user'
 ...
+--
+-- gh-714: box.schema.user.grant error should be versatile,
+-- i.e. error on universally granted privileges shouldn't
+-- include any redundant details and/or symbols
+--
+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
+...
+-- expected behavior of grant() error shouldn't change otherwise
+sp = box.schema.create_space('not_universe')
+---
+...
+box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
+---
+...
+box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
+---
+- error: User 'guest' already has read,write,execute access on space 'not_universe'
+...
+sp:drop()
+---
+...
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index 759827721..46373b71a 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -806,3 +806,17 @@ box.schema.user.drop("user3")
 -- instance could not bootstrap nor recovery.
 --
 box.space._priv:delete{1, 'universe', 0}
+
+--
+-- gh-714: box.schema.user.grant error should be versatile,
+-- i.e. error on universally granted privileges shouldn't
+-- include any redundant details and/or symbols
+--
+box.schema.user.grant('guest', 'read,write,execute', 'universe')
+box.schema.user.grant('guest', 'read,write,execute', 'universe')
+
+-- expected behavior of grant() error shouldn't change otherwise
+sp = box.schema.create_space('not_universe')
+box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
+box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
+sp:drop()
-- 
2.24.0
  
>Четверг, 20 февраля 2020, 13:52 +03:00 от Igor Munkin <imun@tarantool.org>:
> 
>Masha,
>
>Thanks for the patch. It LGTM except the one minor comment below.
>
>On 19.02.20, Maria Khaydich wrote:
>>
>> Error message on granted privileges was not flexible and
>> did not distinguish between universal or any other priviliges
>> leaving either 'nil' or simply '' at the end.
>>  
>> Closes #714
>> ----------------------------------------------------------------------
>> Issue:
>>  https://github.com/tarantool/tarantool/issues/714  
>> Branch:
>>  https://github.com/tarantool/tarantool/tree/eljashm/gh-714-box-schema-user-grant-invalid-error  
>>  
>>  src/box/errcode.h        |  2 +-
>>  src/box/lua/schema.lua   |  3 +++
>>  test/box/access.result   | 30 ++++++++++++++++++++++++++++--
>>  test/box/access.test.lua | 14 ++++++++++++++
>>  4 files changed, 46 insertions(+), 3 deletions(-)
>>  
>
><snipped>
>
>> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
>> index 50c96a335..228f8798a 100644
>> --- a/src/box/lua/schema.lua
>> +++ b/src/box/lua/schema.lua
>> @@ -2408,6 +2408,9 @@ local function grant(uid, name, privilege, object_type,
>>                 privilege == 'execute' then
>>                  box.error(box.error.ROLE_GRANTED, name, object_name)
>>              else
>> +                if object_type ~= 'universe' then
>> +                    object_name = ' \''..object_name..'\''
>
>Minor: the way you build the resulting error string seems to be
>inconvenient to me. Please consider the following and choose the one
>you guess fits more:
>| object_name = " '" .. object_name .. "'"
>or
>| object name = string.format(" '%s'", object_name)
>
>> +                end
>>                  box.error(box.error.PRIV_GRANTED, name, privilege,
>>                            object_type, object_name)
>>              end
>>  
>
><snipped>
>
>> -- 
>> 2.24.0
>>  
>> --
>> Maria Khaydich
>
>--
>Best regards,
>IM
 
 
--
Maria Khaydich
 

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