Обсуждение: Extra functionality to createuser
Sitting on my todo list for a while has been to consider the idea of adding a bit of additional functionality to createuser. One of the functions of CREATE ROLE is to associate the role with other roles, thus... create role my_new_user nosuperuser nocreatedb login IN ROLE app_readonly_role, app2_writer_role; That isn't something that I can do using createuser; to do that, I would need to submit two requests separately: PGUSER=postgres createuser -D -S -l my_new_user PGUSER=postgres psql -c "grant app_readonly_role, app2_writer_role to my_new_user;" I could certainly change over to using psql to do all the work, but it would be rather nice if createuser had (say) a "-g" option which allowed specifying the set of roles that should be assigned. Thus, the above commands might be replaced by: PGUSER=postgres createuser -D -S -l -g app_readonly_role,app2_writer_role my_new_user Would this be worth adding to the ToDo list? -- When confronted by a difficult problem, solve it by reducing it to the question, "How would the Lone Ranger handle this?"
On Thu, Sep 26, 2013 at 1:04 PM, Christopher Browne <cbbrowne@gmail.com> wrote: > Sitting on my todo list for a while has been to consider the idea of > adding a bit of additional functionality to createuser. > > One of the functions of CREATE ROLE is to associate the role with > other roles, thus... > > create role my_new_user nosuperuser nocreatedb login > IN ROLE app_readonly_role, app2_writer_role; > > That isn't something that I can do using createuser; to do that, I > would need to submit two requests separately: > > PGUSER=postgres createuser -D -S -l my_new_user > PGUSER=postgres psql -c "grant app_readonly_role, app2_writer_role > to my_new_user;" > > I could certainly change over to using psql to do all the work, but it > would be rather nice if createuser had (say) a "-g" option which > allowed specifying the set of roles that should be assigned. > > Thus, the above commands might be replaced by: > PGUSER=postgres createuser -D -S -l -g > app_readonly_role,app2_writer_role my_new_user > > Would this be worth adding to the ToDo list? I'd be inclined to favor a patch implementing this. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 9/27/13 6:01 AM, Robert Haas wrote: >> Thus, the above commands might be replaced by: >> > PGUSER=postgres createuser -D -S -l -g >> >app_readonly_role,app2_writer_role my_new_user >> > >> >Would this be worth adding to the ToDo list? > I'd be inclined to favor a patch implementing this. +1 -- Jim C. Nasby, Data Architect jim@nasby.net 512.569.9461 (cell) http://jim.nasby.net
Attached is a patch implementing the "-g / --roles" option for createuser. I'll be attaching it to the open CommitFest shortly.
Вложения
Hello, Tried to test this patch. Did the following 1. cloned from https://github.com/samthakur74/postgres 2. Applied patch and make install 3. created rolesapp_readonly_role,app2_writer_role 4. Tried createuser -D -S -l -g app_readonly_role,app2_writer_role test_user got error: createuser: invalid option -- 'g' 5. Tried createuser -D -S -l --roles app_readonly_role,app2_writer_role test_user. This does not give error. 6. Confirmed that test_user is created using \du and it has postgres=# \du List of roles Role name | Attributes | Member of -------------------+------------------------------------------------+----------- ---------------------------Sameer | Superuser, Create role, Create DB, Replication | {}app2_writer_role | Cannotlogin | {}app_readonly_role | Cannot login | {}my_new_user | | {app_reado nly_role,app2_writer_role}test_user | | {app_reado nly_role,app2_writer_role} 7. createuser --help does show -g, --roles roles to associate with this new role So i think -g option is failing regards Sameer
> 1. cloned from https://github.com/samthakur74/postgres Sorry. cloned from https://github.com/postgres/postgres regards Sameer
On Thu, Nov 14, 2013 at 5:41 AM, Sameer Thakur <samthakur74@gmail.com> wrote: > So i think -g option is failing Right you are. I was missing a "g:" in the getopt_long() call. Attached is a revised patch that handles that. And it behaves better: postgres@cbbrowne ~/p/s/b/scripts> ./createuser -g purge_role -U postgres newuser4 postgres@cbbrowne ~/p/s/b/scripts> pg_dumpall -g | grep newuser4 CREATE ROLE newuser4; ALTER ROLE newuser4 WITH NOSUPERUSER INHERIT NOCREATEROLE NOCREATEDB LOGIN NOREPLICATION; GRANT purge_role TO newuser4 GRANTED BY postgres; -- When confronted by a difficult problem, solve it by reducing it to the question, "How would the Lone Ranger handle this?"
Вложения
On 11/14/13, 4:35 PM, Christopher Browne wrote:> On Thu, Nov 14, 2013 at 5:41 AM, Sameer Thakur <samthakur74@gmail.com> wrote: >> So i think -g option is failing > > Right you are. > > I was missing a "g:" in the getopt_long() call. > > Attached is a revised patch that handles that. > src/bin/scripts/createuser.c:117: indent with spaces. + case 'g': src/bin/scripts/createuser.c:118: indent with spaces. + roles = pg_strdup(optarg);
On Fri, Nov 15, 2013 at 3:14 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > On 11/14/13, 4:35 PM, Christopher Browne wrote:> On Thu, Nov 14, 2013 at > 5:41 AM, Sameer Thakur <samthakur74@gmail.com> wrote: >>> So i think -g option is failing >> >> Right you are. >> >> I was missing a "g:" in the getopt_long() call. >> >> Attached is a revised patch that handles that. >> > > src/bin/scripts/createuser.c:117: indent with spaces. > + case 'g': > src/bin/scripts/createuser.c:118: indent with spaces. > + roles = pg_strdup(optarg); OK, I ran pgindent on createuser.c, which leads to the Next Patch... -- When confronted by a difficult problem, solve it by reducing it to the question, "How would the Lone Ranger handle this?"
Вложения
On Sat, Nov 16, 2013 at 4:57 AM, Christopher Browne <cbbrowne@gmail.com> wrote:
> On Fri, Nov 15, 2013 at 3:14 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
>> On 11/14/13, 4:35 PM, Christopher Browne wrote:> On Thu, Nov 14, 2013 at
>> 5:41 AM, Sameer Thakur <samthakur74@gmail.com> wrote:
>>>> So i think -g option is failing
>>>
>>> Right you are.
>>>
This patch adds useful option '-g' to createuser utility which will
allow user to make new roles as member of existing roles and the same
is already possible by Create Role/User syntax.
Few comments:
1.
+ <term><option>-g</></term>
+ <term><option>--roles</></term>
All other options which require argument are of form:<term><option>-c <replaceable
class="parameter">number</replaceable></></term> <term><option>--connection-limit=<replaceable
class="parameter">number</replaceable></></term>
So I think it is better to have this new option which require argument
in similar form.
2.
+ Indicates roles to associate with this role.
I think word associate is not very clear, wouldn't it be better to
mention that this new role will be member of roles specified.
For example:
Indicates roles to which the new role will be immediately added as a new member.
3.
+ case 'g':
+ roles = pg_strdup(optarg);
+ break;
If we see most of other options in case handling are ordered as per
their order in long_options array. For example
static struct option long_options[] = {
{"host", required_argument, NULL, 'h'},
{"port", required_argument, NULL, 'p'},
..
Now the order of handling for both is same in switch case or while get
opt_long() function call. I think this makes code easy to understand
and modify.
However there is no functionality issue here, so you can keep the code
as per your existing patch as well, this is just a suggestion.
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Mon, Nov 18, 2013 at 1:01 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Sat, Nov 16, 2013 at 4:57 AM, Christopher Browne <cbbrowne@gmail.com> wrote:
> Few comments:
>
> 1.
> + <term><option>-g</></term>
> + <term><option>--roles</></term>
>
> All other options which require argument are of form:
> <term><option>-c <replaceable class="parameter">number</replaceable></></term>
> <term><option>--connection-limit=<replaceable
> class="parameter">number</replaceable></></term>
>
> So I think it is better to have this new option which require argument
> in similar form.
Sounds good, done.
> 2.
> + Indicates roles to associate with this role.
>
> I think word associate is not very clear, wouldn't it be better to
> mention that this new role will be member of roles specified.
> For example:
> Indicates roles to which the new role will be immediately added as a new member.
With a switch of "immediately" and "added", done. That does better
describe the behaviour.
> 3.
> + case 'g':
> + roles = pg_strdup(optarg);
> + break;
>
> If we see most of other options in case handling are ordered as per
> their order in long_options array. For example
>
> static struct option long_options[] = {
> {"host", required_argument, NULL, 'h'},
> {"port", required_argument, NULL, 'p'},
> ..
>
> Now the order of handling for both is same in switch case or while get
> opt_long() function call. I think this makes code easy to understand
> and modify.
> However there is no functionality issue here, so you can keep the code
> as per your existing patch as well, this is just a suggestion.
That is easy enough to change, and yes, indeed, having the new code
look just like what it is near seems an improvement.
I picked the location of the 'g:' in the opt_long() call basically arbitrarily;
if there is any reason for it to go in a different spot, I'd be happy to
shift it.
--
When confronted by a difficult problem, solve it by reducing it to the
question, "How would the Lone Ranger handle this?"
Вложения
Patch needs to be rebased again.
On Wed, Nov 20, 2013 at 2:05 AM, Christopher Browne <cbbrowne@gmail.com> wrote:
> On Mon, Nov 18, 2013 at 1:01 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> On Sat, Nov 16, 2013 at 4:57 AM, Christopher Browne <cbbrowne@gmail.com> wrote:
>
> I picked the location of the 'g:' in the opt_long() call basically arbitrarily;
I think this is okay, the main point was to maintain consistency
with surrounding code.
> if there is any reason for it to go in a different spot, I'd be happy to
> shift it.
The only other option could be to add it at end which is generally
better unless we get any benefit by adding it in middle.
On further tests, I found inconsistency in behavior when some special
characters are used in role names.
1. Test for role name containing quotes
a. In psql, create a role containing quotes in role name. create role amitk in role "test_ro'le_3";
b. Now if we try to make a new role member of this role using
createuser utility, it gives error try-1 createuser.exe -g test_ro'le_3 -p 5446 amitk_2
createuser: creation of new role failed: ERROR: unterminated quoted
string at or near "'le_3;"
LINE 1: ... NOCREATEDB NOCREATEROLE INHERIT LOGIN IN ROLE test_ro'le_3; try-2 createuser.exe -g "test_ro'le_3" -p
5446amitk
createuser: creation of new role failed: ERROR: unterminated quoted
string at or near "'le_3;"
LINE 1: ... NOCREATEDB NOCREATEROLE INHERIT LOGIN IN ROLE test_ro'le_3;
c. If I try quoted string in new role to be created, it works fine. createuser.exe -p 5446 am'itk_2
As quoted strings work well for role names, I think it should work
with -g option as well.
2. Test for role name containing special character ';' (semicolon)
a. create role "test;_1";
b. Now if we try to make a new role member of this role using
createuser utility, it gives error try-1
createuser.exe -g test;_1 -p 5446 amitk_4
createuser: creation of new role failed: ERROR: syntax error at or near "_1"
LINE 1: ...RUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN IN ROLE test;_1; try-2
^
createuser.exe -g "test;_1" -p 5446 amitk_4
createuser: creation of new role failed: ERROR: syntax error at or near "_1"
LINE 1: ...RUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN IN ROLE test;_1;
^ try-3
createuser.exe -g 'test;_1' -p 5446 amitk_4
createuser: creation of new role failed: ERROR: syntax error at or
near "'test;_1'"
LINE 1: ...SER NOCREATEDB NOCREATEROLE INHERIT LOGIN IN ROLE 'test;_1';
c. If I try semicolon in new role to be created, it works fine.
createuser.exe -p 5446 amit;k_3
As semicolon work well for role names, I think it should work with -g
option as well.
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Tue, Nov 19, 2013 at 11:54 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On further tests, I found inconsistency in behavior when some special
> characters are used in role names.
>
> 1. Test for role name containing quotes
> a. In psql, create a role containing quotes in role name.
> create role amitk in role "test_ro'le_3";
>
> b. Now if we try to make a new role member of this role using
> createuser utility, it gives error
> try-1
> createuser.exe -g test_ro'le_3 -p 5446 amitk_2
> createuser: creation of new role failed: ERROR: unterminated quoted
> string at or near "'le_3;"
> LINE 1: ... NOCREATEDB NOCREATEROLE INHERIT LOGIN IN ROLE test_ro'le_3;
> try-2
> createuser.exe -g "test_ro'le_3" -p 5446 amitk
> createuser: creation of new role failed: ERROR: unterminated quoted
> string at or near "'le_3;"
> LINE 1: ... NOCREATEDB NOCREATEROLE INHERIT LOGIN IN ROLE test_ro'le_3;
>
> c. If I try quoted string in new role to be created, it works fine.
> createuser.exe -p 5446 am'itk_2
>
> As quoted strings work well for role names, I think it should work
> with -g option as well.
>
> 2. Test for role name containing special character ';' (semicolon)
> a. create role "test;_1";
>
> b. Now if we try to make a new role member of this role using
> createuser utility, it gives error
> try-1
> createuser.exe -g test;_1 -p 5446 amitk_4
> createuser: creation of new role failed: ERROR: syntax error at or near "_1"
> LINE 1: ...RUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN IN ROLE test;_1;
> try-2 ^
> createuser.exe -g "test;_1" -p 5446 amitk_4
> createuser: creation of new role failed: ERROR: syntax error at or near "_1"
> LINE 1: ...RUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN IN ROLE test;_1;
> ^
> try-3
> createuser.exe -g 'test;_1' -p 5446 amitk_4
> createuser: creation of new role failed: ERROR: syntax error at or
> near "'test;_1'"
> LINE 1: ...SER NOCREATEDB NOCREATEROLE INHERIT LOGIN IN ROLE 'test;_1';
>
> c. If I try semicolon in new role to be created, it works fine.
> createuser.exe -p 5446 amit;k_3
>
> As semicolon work well for role names, I think it should work with -g
> option as well.
I was not unconscious of there being the potential for issue here; there is an
easy answer of double quoting the string, thus:
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index 88b8f2a..04ec324 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -308,7 +308,7 @@ main(int argc, char *argv[])
if (conn_limit != NULL)
appendPQExpBuffer(&sql, " CONNECTION LIMIT %s", conn_limit);
if (roles != NULL)
- appendPQExpBuffer(&sql, " IN ROLE %s", roles);
+ appendPQExpBuffer(&sql, " IN ROLE \"%s\"", roles);
appendPQExpBufferStr(&sql, ";\n");
if (echo)
(END)
I was conscious of not quoting it. Note that other parameters are not quoted
either, so I imagined I was being consistent with that.
I have added the above change, as well as rebasing, per Peter's recommendation.
--
When confronted by a difficult problem, solve it by reducing it to the
question, "How would the Lone Ranger handle this?"
Вложения
Wait, that doesn't work if more than one role is added, as they get merged together by the quoting. A somewhat ugly amount of quoting can be done at the shell level to induce double quotes. $ createuser -g "\"test_rol'e_3\"" usequoted3 I note that similar (with not quite identical behaviour) issues apply to the user name. Perhaps the resolution to this is to leave quoting issues to the administrator. That simplifies the problem away. I suspect that the apparatus needed to do a thorough solution (e.g. - parse the string, and do something "smarter") may be larger than is worth getting into.
On Wed, Nov 20, 2013 at 9:53 PM, Christopher Browne <cbbrowne@gmail.com> wrote: > Wait, that doesn't work if more than one role is added, as they get > merged together by the quoting. > > A somewhat ugly amount of quoting can be done at the shell level to > induce double quotes. > > $ createuser -g "\"test_rol'e_3\"" usequoted3 > > I note that similar (with not quite identical behaviour) issues apply > to the user name. Perhaps the > resolution to this is to leave quoting issues to the administrator. > That simplifies the problem away. We are already doing something similar for username, refer below line: printfPQExpBuffer(&sql, "CREATE ROLE %s", fmtId(newuser)); Here fmtId() is doing handling for quotes. Now for new syntax IN ROLE, the difference is that it can have multiple strings which needs additional handling. I think some similar handling should be there in server, pg_dump as well. > I suspect that the apparatus needed to do a thorough solution (e.g. - > parse the string, and do something > "smarter") may be larger than is worth getting into. I think if it needs some bigger solution then we can leave it by having small note in documentation, but if it's just a matter of calling some existing functions or mimic some handling done at other place, then it is worth trying. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Wed, 2013-11-20 at 11:23 -0500, Christopher Browne wrote: > I note that similar (with not quite identical behaviour) issues apply > to the user name. Perhaps the > resolution to this is to leave quoting issues to the administrator. > That simplifies the problem away. How about only one role name per -g option, but allowing the -g option to be repeated?
On Fri, Dec 6, 2013 at 10:31 AM, Peter Eisentraut <peter_e@gmx.net> wrote: > On Wed, 2013-11-20 at 11:23 -0500, Christopher Browne wrote: >> I note that similar (with not quite identical behaviour) issues apply >> to the user name. Perhaps the >> resolution to this is to leave quoting issues to the administrator. >> That simplifies the problem away. > > How about only one role name per -g option, but allowing the -g option > to be repeated? I think that might simplify the problem and patch, but do you think it is okay to have inconsistency for usage of options between Create User statement and this utility? With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Sat, Dec 7, 2013 at 11:39 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Fri, Dec 6, 2013 at 10:31 AM, Peter Eisentraut <peter_e@gmx.net> wrote: >> On Wed, 2013-11-20 at 11:23 -0500, Christopher Browne wrote: >>> I note that similar (with not quite identical behaviour) issues apply >>> to the user name. Perhaps the >>> resolution to this is to leave quoting issues to the administrator. >>> That simplifies the problem away. >> >> How about only one role name per -g option, but allowing the -g option >> to be repeated? > > I think that might simplify the problem and patch, but do you think > it is okay to have inconsistency > for usage of options between Create User statement and this utility? Yes. In general, command-line utilities use a very different syntax for options-passing that SQL commands. Trying to make them consistent feels unnecessary or perhaps even counterproductive. And the proposed syntax is certainly a convention common to many other command-line utilities, so I think it's fine. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Dec 10, 2013 at 12:20 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Sat, Dec 7, 2013 at 11:39 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> On Fri, Dec 6, 2013 at 10:31 AM, Peter Eisentraut <peter_e@gmx.net> wrote: >>> >>> How about only one role name per -g option, but allowing the -g option >>> to be repeated? >> >> I think that might simplify the problem and patch, but do you think >> it is okay to have inconsistency >> for usage of options between Create User statement and this utility? > > Yes. In general, command-line utilities use a very different syntax > for options-passing that SQL commands. Trying to make them consistent > feels unnecessary or perhaps even counterproductive. And the proposed > syntax is certainly a convention common to many other command-line > utilities, so I think it's fine. Okay, the new way for syntax suggested by Peter has simplified the problem. Please find the updated patch and docs for multiple -g options. If there are no objections, then I will mark this patch as Ready For Committer. Christopher, please check once, if you have any comments/objections for modifications. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Вложения
On Tue, Dec 10, 2013 at 9:55 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Tue, Dec 10, 2013 at 12:20 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Sat, Dec 7, 2013 at 11:39 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: >>> On Fri, Dec 6, 2013 at 10:31 AM, Peter Eisentraut <peter_e@gmx.net> wrote: >>>> >>>> How about only one role name per -g option, but allowing the -g option >>>> to be repeated? >>> >>> I think that might simplify the problem and patch, but do you think >>> it is okay to have inconsistency >>> for usage of options between Create User statement and this utility? >> >> Yes. In general, command-line utilities use a very different syntax >> for options-passing that SQL commands. Trying to make them consistent >> feels unnecessary or perhaps even counterproductive. And the proposed >> syntax is certainly a convention common to many other command-line >> utilities, so I think it's fine. > > Okay, the new way for syntax suggested by Peter has simplified the problem. > Please find the updated patch and docs for multiple -g options. Committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Dec 11, 2013 at 6:23 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Dec 10, 2013 at 9:55 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> Okay, the new way for syntax suggested by Peter has simplified the problem. >> Please find the updated patch and docs for multiple -g options. > > Committed. Thank you. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Wed, Dec 11, 2013 at 7:53 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Dec 10, 2013 at 9:55 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> On Tue, Dec 10, 2013 at 12:20 AM, Robert Haas <robertmhaas@gmail.com> wrote: >>> On Sat, Dec 7, 2013 at 11:39 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: >>>> On Fri, Dec 6, 2013 at 10:31 AM, Peter Eisentraut <peter_e@gmx.net> wrote: >>>>> >>>>> How about only one role name per -g option, but allowing the -g option >>>>> to be repeated? >>>> >>>> I think that might simplify the problem and patch, but do you think >>>> it is okay to have inconsistency >>>> for usage of options between Create User statement and this utility? >>> >>> Yes. In general, command-line utilities use a very different syntax >>> for options-passing that SQL commands. Trying to make them consistent >>> feels unnecessary or perhaps even counterproductive. And the proposed >>> syntax is certainly a convention common to many other command-line >>> utilities, so I think it's fine. >> >> Okay, the new way for syntax suggested by Peter has simplified the problem. >> Please find the updated patch and docs for multiple -g options. > > Committed. Looks good, thanks! -- When confronted by a difficult problem, solve it by reducing it to the question, "How would the Lone Ranger handle this?"