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 : >  >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(-) >>   > > > >> 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 >>   > > > >> --  >> 2.24.0 >>   >> -- >> Maria Khaydich > >-- >Best regards, >IM     -- Maria Khaydich