Thread: getting confused parsing ACLITEMS...
This is the situation, I create a user called "
test=# create user """";
CREATE USER
test=# drop user """";
DROP USER
test=# create user """";
CREATE USER
test=# create table temp(a int4);
CREATE TABLE
test=# grant select on temp to """";
GRANT
test=# \dp temp Access privileges for database "test"Schema | Table | Access privileges
--------+-------+-----------------------------------------------------public | temp |
{chriskl=a*r*w*d*R*x*t*/chriskl,"\"\"\"=r/chriskl"}
(1 row)
OK, so the second aclitem is:
"\"\"\"=r/chriskl"
So how on earth does that quoting work????
If I remove the first and last quote (assuming the whole thing is quoted),
unescape it, then I get this:
"""=r/chriskl
Which I cannot parse, as """ doens't mean anything! I think that the second
aclitem should appear like this;
"\"\\\"\"=r/chriskl"
Which will (after removing string quotes and unescaping), reduce to this:
"\""=r/chriskl
I notice that it doesn't confuse postgres itself though:
test=# revoke select on temp from """";
REVOKE
test=# \dp temp Access privileges for database "test"Schema | Table | Access privileges
--------+-------+----------------------------------public | temp | {chriskl=a*r*w*d*R*x*t*/chriskl}
(1 row)
So is there a bug here?
Chris
The situation seems to be a bug that this patch would address. It seems to
me that when a username is considered unsafe due to containing double
quotes, the double quotes should be escaped (and the backslashes)!
Does this look alright?
Chris
Index: src/backend/utils/adt/acl.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/backend/utils/adt/acl.c,v
retrieving revision 1.94
diff -c -r1.94 acl.c
*** src/backend/utils/adt/acl.c 4 Aug 2003 02:40:04 -0000 1.94
--- src/backend/utils/adt/acl.c 8 Aug 2003 09:03:19 -0000
***************
*** 124,131 **** } if (!safe) *p++ = '"';
! for (src = s; *src; src++) *p++ = *src; if (!safe) *p++ = '"'; *p =
'\0';
--- 124,134 ---- } if (!safe) *p++ = '"';
! for (src = s; *src; src++) {
! if (!safe && (*src == '"' || *src == '\\'))
! *p++ = '\\'; *p++ = *src;
+ } if (!safe) *p++ = '"'; *p = '\0';
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes:
> The situation seems to be a bug that this patch would address. It seems to
> me that when a username is considered unsafe due to containing double
> quotes, the double quotes should be escaped (and the backslashes)!
> Does this look alright?
I would personally say that the convention ought to match what is done
in quoted identifiers, namely: " becomes "", backslash is not special
and therefore doesn't need anything.
More to the point, this is highly incomplete... you did not teach the
adjacent getid routine about this, and there is code in (at least)
pg_dump.c that knows the quoting conventions used here.
regards, tom lane
Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes:
>> More to the point, this is highly incomplete... you did not teach the
>> adjacent getid routine about this, and there is code in (at least)
>> pg_dump.c that knows the quoting conventions used here.
> Hang on - those routines can parse the acls just fine? How? How do they
> handle usernames with equals signs in them (my major prob). How can it
> work at all?
IIRC, the present code assumes that usernames won't contain embedded
doublequotes. I did recently fix pg_dump to work in cases that it
wouldn't have handled before, including embedded equals. (BTW, my
mistake above: the pg_dump code is not in pg_dump.c, but in dumputils.c.
Look at copyAclUserName in particular.)
regards, tom lane
Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes:
> So if you agree that there is a quoting problem,and you don't mind
> breaking backwards compatibility for it, I'll do a complete patch...
I don't see any backwards-compatibility issue, because usernames
containing double quotes just plain don't work in past releases;
we've never before bothered to have a complete quoting solution
in ACLs.
regards, tom lane
> More to the point, this is highly incomplete... you did not teach the > adjacent getid routine about this, and there is code in (at least) > pg_dump.c that knows the quoting conventions used here. Hang on - those routines can parse the acls just fine? How? How do they handle usernames with equals signs in them (my major prob). How can it work at all? Chris
OK, So if you agree that there is a quoting problem,and you don't mind breaking backwards compatibility for it, I'll do a complete patch... Chris On Fri, 8 Aug 2003, Tom Lane wrote: > "Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes: > > The situation seems to be a bug that this patch would address. It seems to > > me that when a username is considered unsafe due to containing double > > quotes, the double quotes should be escaped (and the backslashes)! > > > Does this look alright? > > I would personally say that the convention ought to match what is done > in quoted identifiers, namely: " becomes "", backslash is not special > and therefore doesn't need anything. > > More to the point, this is highly incomplete... you did not teach the > adjacent getid routine about this, and there is code in (at least) > pg_dump.c that knows the quoting conventions used here. > > regards, tom lane >
Tom Lane wrote: >Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes: > > >>So if you agree that there is a quoting problem,and you don't mind >>breaking backwards compatibility for it, I'll do a complete patch... >> >> > >I don't see any backwards-compatibility issue, because usernames >containing double quotes just plain don't work in past releases; >we've never before bothered to have a complete quoting solution >in ACLs. > > Is it useful to allow these special chars at all? Seems this creates a lot of work, and most admins will probably stick to "normal" user names anyway. Regards, Andreas
Andreas Pflug <pgadmin@pse-consulting.de> writes:
> Is it useful to allow these special chars at all? Seems this creates a
> lot of work, and most admins will probably stick to "normal" user names
> anyway.
Well, the reason it's been left unfixed for so long is exactly that it
didn't seem pressing. But if Chris wants to do the work, I won't stand
in his way ...
regards, tom lane
Of course, now I've just gone to some trouble to accomodate funky characters in user and dbnames in logging I'd have to kill him ... :-) Seriously, I think there's a good case for banning a few characters in at least some names - like []<>'"~#*|\ , say andrew Tom Lane wrote: >Andreas Pflug <pgadmin@pse-consulting.de> writes: > > >>Is it useful to allow these special chars at all? Seems this creates a >>lot of work, and most admins will probably stick to "normal" user names >>anyway. >> >> > >Well, the reason it's been left unfixed for so long is exactly that it >didn't seem pressing. But if Chris wants to do the work, I won't stand >in his way ... > > > >
> >I don't see any backwards-compatibility issue, because usernames > >containing double quotes just plain don't work in past releases; > >we've never before bothered to have a complete quoting solution > >in ACLs. > > > > > > Is it useful to allow these special chars at all? Seems this creates a > lot of work, and most admins will probably stick to "normal" user names > anyway. It's the principle of the thing :) Also, I don't know how non-english speakers treat their double quote characers... Chris
> Seriously, I think there's a good case for banning a few characters in > at least some names - like []<>'"~#*|\ , say Why? They're allowed in all other identifiers. And what if someone already has a database full of usernames with those chars? They wouldn't be able to load their dump properly... Chris > andrew > > Tom Lane wrote: > > >Andreas Pflug <pgadmin@pse-consulting.de> writes: > > > > > >>Is it useful to allow these special chars at all? Seems this creates a > >>lot of work, and most admins will probably stick to "normal" user names > >>anyway. > >> > >> > > > >Well, the reason it's been left unfixed for so long is exactly that it > >didn't seem pressing. But if Chris wants to do the work, I won't stand > >in his way ... > > > > > > > > > > > ---------------------------(end of broadcast)--------------------------- > TIP 2: you can get off all lists at once with the unregister command > (send "unregister YourEmailAddressHere" to majordomo@postgresql.org) >
I know. It just makes a few things a pain if you can't say "I know this character can't be part of that". Nevermind. Just wishful thinking. I'll shut up now. andrew Christopher Kings-Lynne wrote: >>Seriously, I think there's a good case for banning a few characters in >>at least some names - like []<>'"~#*|\ , say >> >> > >Why? They're allowed in all other identifiers. And what if someone >already has a database full of usernames with those chars? They wouldn't >be able to load their dump properly... > >Chris > >
I believe Tom just applied this. Thanks.
---------------------------------------------------------------------------
Christopher Kings-Lynne wrote:
> The situation seems to be a bug that this patch would address. It seems to
> me that when a username is considered unsafe due to containing double
> quotes, the double quotes should be escaped (and the backslashes)!
>
> Does this look alright?
>
> Chris
>
> Index: src/backend/utils/adt/acl.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/backend/utils/adt/acl.c,v
> retrieving revision 1.94
> diff -c -r1.94 acl.c
> *** src/backend/utils/adt/acl.c 4 Aug 2003 02:40:04 -0000 1.94
> --- src/backend/utils/adt/acl.c 8 Aug 2003 09:03:19 -0000
> ***************
> *** 124,131 ****
> }
> if (!safe)
> *p++ = '"';
> ! for (src = s; *src; src++)
> *p++ = *src;
> if (!safe)
> *p++ = '"';
> *p = '\0';
> --- 124,134 ----
> }
> if (!safe)
> *p++ = '"';
> ! for (src = s; *src; src++) {
> ! if (!safe && (*src == '"' || *src == '\\'))
> ! *p++ = '\\';
> *p++ = *src;
> + }
> if (!safe)
> *p++ = '"';
> *p = '\0';
>
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org
>
-- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610)
359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square,
Pennsylvania19073