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

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

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


Nits and change log are 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  

@ChangeLog
- Pretty error message for user.grant - no extra ' ' for universal privileges.
    gh-714

 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..b454d0eaa 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..387c8b06b 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 
>Пятница, 21 февраля 2020, 1:45 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:
> 
>Almost forgot - I don't know whether this should be
>included into the change log. Please, ask Kirill. 
 
 
--
Maria Khaydich
 

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