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

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

[Maria Khaydich]

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


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(-)
 
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..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..'\''
+                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
 
--
Maria Khaydich

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

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

[Igor Munkin]

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

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

[Maria Khaydich]

[-- 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 --]

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

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

On 19/02/2020 17:35, Maria Khaydich wrote:
> Error message on granted privileges was not flexible and
> did not distinguish between universal or any other priviliges

priviliges -> privileges

> leaving either 'nil' or simply '' at the end.
>  
> Closes #714
> > 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

It would be good to have a dot in the end of this sentence and of the
previous comment. And to start the sentence with a capital letter.
Although I can't block the patch because of this nit, so LGTM and up
to you.

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

[Vladislav Shpilevoy]

Almost forgot - I don't know whether this should be
included into the change log. Please, ask Kirill.

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

[Kirill Yukhin]

Hello,

On 20 фев 23:45, Vladislav Shpilevoy wrote:
> Almost forgot - I don't know whether this should be
> included into the change log. Please, ask Kirill.

I think yes.

--
Regards, Kirill Yukhin

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

[Maria Khaydich]

[-- 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 --]

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

[Kirill Yukhin]

Hello,

On 19 фев 19:35, 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  

I've checked your patch into master.

--
Regards, Kirill Yukhin

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

[Maria Khaydich]

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


Looks like this patch caused some tests to fail according to Sasha (avtikhon).
I made another commit fixing the issue.
 
----------------------------------------------------------------------
Previous commit that was merged into master did not do
proper clean-up. That caused occasional failures of other
tests, all with the same error saying user 'guest' already
had access on universe.
 
Closes #714
---
Branch:
https://github.com/tarantool/tarantool/compare/eljashm/gh-714-box-schema-user-grant-invalid-error  

 test/box/access.result   | 18 ++++++++++++------
 test/box/access.test.lua | 10 ++++++----
 2 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/test/box/access.result b/test/box/access.result
index b454d0eaa..e351eaf2c 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -2113,23 +2113,29 @@ box.space._priv:delete{1, 'universe', 0}
 -- 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.create('grantee')
 ---
 ...
-box.schema.user.grant('guest', 'read,write,execute', 'universe')
+box.schema.user.grant('grantee', 'read,write,execute', 'universe')
 ---
-- error: User 'guest' already has read,write,execute access on universe
+...
+box.schema.user.grant('grantee', 'read,write,execute', 'universe')
+---
+- error: User 'grantee' 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('grantee', 'read,write,execute', 'space', 'not_universe')
+---
+...
+box.schema.user.grant('grantee', 'read,write,execute', 'space', 'not_universe')
 ---
+- error: User 'grantee' already has read,write,execute access on space 'not_universe'
 ...
-box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
+box.schema.user.drop('grantee')
 ---
-- 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 387c8b06b..6be558247 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -812,11 +812,13 @@ box.space._priv:delete{1, 'universe', 0}
 -- 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')
+box.schema.user.create('grantee')
+box.schema.user.grant('grantee', 'read,write,execute', 'universe')
+box.schema.user.grant('grantee', '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')
+box.schema.user.grant('grantee', 'read,write,execute', 'space', 'not_universe')
+box.schema.user.grant('grantee', 'read,write,execute', 'space', 'not_universe')
+box.schema.user.drop('grantee')
 sp:drop()
-- 
2.24.0 
>Среда, 26 февраля 2020, 1:20 +03:00 от Kirill Yukhin < kyukhin@tarantool.org >:
> 
>Hello,
>
>On 19 фев 19:35, 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  
>I've checked your patch into master.
>
>--
>Regards, Kirill Yukhin 
 
 
--
Maria Khaydich
 
 

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

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

[Igor Munkin]

Masha,

As Sasha Tu. noticed it looks to be related to the test-run issue[1].

Cced Sasha Ti. to take a look on the changes.

On 26.02.20, Maria Khaydich wrote:
> 
> Looks like this patch caused some tests to fail according to Sasha (avtikhon).
> I made another commit fixing the issue.
>  
> ----------------------------------------------------------------------
> Previous commit that was merged into master did not do
> proper clean-up. That caused occasional failures of other
> tests, all with the same error saying user 'guest' already
> had access on universe.
>  
> Closes #714
> ---
> Branch:
> https://github.com/tarantool/tarantool/compare/eljashm/gh-714-box-schema-user-grant-invalid-error  
> 
>  test/box/access.result   | 18 ++++++++++++------
>  test/box/access.test.lua | 10 ++++++----
>  2 files changed, 18 insertions(+), 10 deletions(-)
> diff --git a/test/box/access.result b/test/box/access.result
> index b454d0eaa..e351eaf2c 100644
> --- a/test/box/access.result
> +++ b/test/box/access.result
> @@ -2113,23 +2113,29 @@ box.space._priv:delete{1, 'universe', 0}
>  -- 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.create('grantee')
>  ---
>  ...
> -box.schema.user.grant('guest', 'read,write,execute', 'universe')
> +box.schema.user.grant('grantee', 'read,write,execute', 'universe')
>  ---
> -- error: User 'guest' already has read,write,execute access on universe
> +...
> +box.schema.user.grant('grantee', 'read,write,execute', 'universe')
> +---
> +- error: User 'grantee' 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('grantee', 'read,write,execute', 'space', 'not_universe')
> +---
> +...
> +box.schema.user.grant('grantee', 'read,write,execute', 'space', 'not_universe')
>  ---
> +- error: User 'grantee' already has read,write,execute access on space 'not_universe'
>  ...
> -box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
> +box.schema.user.drop('grantee')
>  ---
> -- 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 387c8b06b..6be558247 100644
> --- a/test/box/access.test.lua
> +++ b/test/box/access.test.lua
> @@ -812,11 +812,13 @@ box.space._priv:delete{1, 'universe', 0}
>  -- 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')
> +box.schema.user.create('grantee')
> +box.schema.user.grant('grantee', 'read,write,execute', 'universe')
> +box.schema.user.grant('grantee', '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')
> +box.schema.user.grant('grantee', 'read,write,execute', 'space', 'not_universe')
> +box.schema.user.grant('grantee', 'read,write,execute', 'space', 'not_universe')
> +box.schema.user.drop('grantee')
>  sp:drop()
> -- 
> 2.24.0 
>  

<snipped>

>  
> --
> Maria Khaydich
>  
>  

[1]: https://github.com/tarantool/test-run/issues/156

-- 
Best regards,
IM

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

[Alexander Tikhonov]

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

Masha,

The fix LGTM, because the test that uses some resources should always be sure
that it is free. Anyway harness should try to help the tests to avoid of such issues,
but it will never reach absolute confidence that all situations are under control, so
the test will be ahead of it in preparing such resources. As about harness then it
can be improved by the QA for the same issues too. As for now, this fix is completely
correct and needed.


>Среда, 26 февраля 2020, 18:00 +03:00 от Igor Munkin <imun@tarantool.org>:
>
>Masha,
>
>As Sasha Tu. noticed it looks to be related to the test-run issue[1].
>
>Cced Sasha Ti. to take a look on the changes.
>
>On 26.02.20, Maria Khaydich wrote:
>> 
>> Looks like this patch caused some tests to fail according to Sasha (avtikhon).
>> I made another commit fixing the issue.
>>  
>> ----------------------------------------------------------------------
>> Previous commit that was merged into master did not do
>> proper clean-up. That caused occasional failures of other
>> tests, all with the same error saying user 'guest' already
>> had access on universe.
>>  
>> Closes #714
>> ---
>> Branch:
>>  https://github.com/tarantool/tarantool/compare/eljashm/gh-714-box-schema-user-grant-invalid-error  
>> 
>>  test/box/access.result   | 18 ++++++++++++------
>>  test/box/access.test.lua | 10 ++++++----
>>  2 files changed, 18 insertions(+), 10 deletions(-)
>> diff --git a/test/box/access.result b/test/box/access.result
>> index b454d0eaa..e351eaf2c 100644
>> --- a/test/box/access.result
>> +++ b/test/box/access.result
>> @@ -2113,23 +2113,29 @@ box.space._priv:delete{1, 'universe', 0}
>>  -- 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.create('grantee')
>>  ---
>>  ...
>> -box.schema.user.grant('guest', 'read,write,execute', 'universe')
>> +box.schema.user.grant('grantee', 'read,write,execute', 'universe')
>>  ---
>> -- error: User 'guest' already has read,write,execute access on universe
>> +...
>> +box.schema.user.grant('grantee', 'read,write,execute', 'universe')
>> +---
>> +- error: User 'grantee' 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('grantee', 'read,write,execute', 'space', 'not_universe')
>> +---
>> +...
>> +box.schema.user.grant('grantee', 'read,write,execute', 'space', 'not_universe')
>>  ---
>> +- error: User 'grantee' already has read,write,execute access on space 'not_universe'
>>  ...
>> -box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
>> +box.schema.user.drop('grantee')
>>  ---
>> -- 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 387c8b06b..6be558247 100644
>> --- a/test/box/access.test.lua
>> +++ b/test/box/access.test.lua
>> @@ -812,11 +812,13 @@ box.space._priv:delete{1, 'universe', 0}
>>  -- 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')
>> +box.schema.user.create('grantee')
>> +box.schema.user.grant('grantee', 'read,write,execute', 'universe')
>> +box.schema.user.grant('grantee', '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')
>> +box.schema.user.grant('grantee', 'read,write,execute', 'space', 'not_universe')
>> +box.schema.user.grant('grantee', 'read,write,execute', 'space', 'not_universe')
>> +box.schema.user.drop('grantee')
>>  sp:drop()
>> -- 
>> 2.24.0 
>>  
>
><snipped>
>
>>  
>> --
>> Maria Khaydich
>>  
>>  
>
>[1]:  https://github.com/tarantool/test-run/issues/156
>
>-- 
>Best regards,
>IM


-- 
Alexander Tikhonov

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

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

[Igor Munkin]

Masha,

Thanks, the patch LGTM, but I left a few nits below.

On 26.02.20, Maria Khaydich wrote:
> 
> Looks like this patch caused some tests to fail according to Sasha (avtikhon).
> I made another commit fixing the issue.
>  
> ----------------------------------------------------------------------
> Previous commit that was merged into master did not do
> proper clean-up. That caused occasional failures of other
> tests, all with the same error saying user 'guest' already
> had access on universe.

I propose the following rewording for the commit message:
| box: use non-default user for tests
|
| The commit e8009f4158fc2d6efa2824cd634564b3c7a2f899
| ('box: user.grant error should be versatile') introduced a flaky test,
| since it provides a non-default grants for the default 'guest' user
| and doesn't drop the changes at the end.
| To avoid flaky failures a new user is created for the test and dropped
| when the related tests are finished.
Feel free to change it on your own.

>  
> Closes #714

I guess Follow-up #714 is more appropriate here.

> ---
> Branch:
> https://github.com/tarantool/tarantool/compare/eljashm/gh-714-box-schema-user-grant-invalid-error  
> 
>  test/box/access.result   | 18 ++++++++++++------
>  test/box/access.test.lua | 10 ++++++----
>  2 files changed, 18 insertions(+), 10 deletions(-)

<snipped>

>  
> --
> Maria Khaydich
>  
>  

-- 
Best regards,
IM

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

[Alexander Turenko]

We can just revoke privileges granted for 'guest' and the patch will be
smaller.

Changed the patch and the description (in the spirit of Igor's wording).

The result:

 | commit e33216af9889a2387f331fd30c157d60292bdc5b
 | Author: Maria <maria.khaydich@tarantool.org>
 | Date:   Wed Feb 26 16:55:36 2020 +0300
 | 
 |     test: add clean up for box/access test
 |     
 |     The commit e8009f4158fc2d6efa2824cd634564b3c7a2f899 ('box: user.grant
 |     error should be versatile') did not do proper clean-up: it grants
 |     non-default privileges for user 'guest' and does not revoke them at the
 |     end. That caused occasional failures of other tests, all with the same
 |     error saying user 'guest' already had access on universe.
 |     
 |     This case should be handled by test-run in a future, see [1].
 |     
 |     [1]: https://github.com/tarantool/test-run/issues/156
 |     
 |     Follows up #714
 | 
 | diff --git a/test/box/access.result b/test/box/access.result
 | index b454d0eaa..20b1b8b35 100644
 | --- a/test/box/access.result
 | +++ b/test/box/access.result
 | @@ -2131,6 +2131,13 @@ box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
 |  ---
 |  - error: User 'guest' already has read,write,execute access on space 'not_universe'
 |  ...
 | +-- Clean up.
 | +box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 | +---
 | +...
 | +box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
 | +---
 | +...
 |  sp:drop()
 |  ---
 |  ...
 | diff --git a/test/box/access.test.lua b/test/box/access.test.lua
 | index 387c8b06b..3e083a383 100644
 | --- a/test/box/access.test.lua
 | +++ b/test/box/access.test.lua
 | @@ -819,4 +819,8 @@ box.schema.user.grant('guest', 'read,write,execute', 'universe')
 |  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')
 | +
 | +-- Clean up.
 | +box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 | +box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
 |  sp:drop()

Pushed to master. CCed Kirill.

WBR, Alexander Turenko.