Обсуждение: REFERENCES privilege should not be symmetric (was Re: [GENERAL] Postgres Permissions Article)

Paul Jungwirth <pj@illuminatedcomputing.com> writes:
>> Also I don't understand why you wrote “You need the permission on both
>> tables”: Only the owner of a table can add constraints to it

> Ah, this piece was really helpful for me in making it click. Thanks so
> much! I added a couple new paragraphs to my post with a link back to
> this thread. I feel like it all makes sense now! :-)

> FYI "You need this permission on both tables" is what the docs say
> (https://www.postgresql.org/docs/9.6/static/sql-grant.html):

>>> To create a foreign key constraint, it is necessary to have this
>>> privilege on both the referencing and referenced columns.

> Maybe it would be worth clarifying there that you need to *own* the
> referencing table, and you need REFERENCES on the referenced table?

Hmm ... interesting.  A bit of excavating in tablecmds.c shows that
in order to do ADD FOREIGN KEY, you need to be owner of the table
the constraint is being attached to (checked by ATSimplePermissions,
which is used for AT_AddConstraint by ATPrepCmd), *and* you need
REFERENCES on both tables, or at least on the columns involved in
the proposed FK constraint (checked by checkFkeyPermissions, which
is invoked against each of the tables by ATAddForeignKeyConstraint).

So yeah, this seems a little bit redundant.  In principle, a table owner
could revoke her own REFERENCES permissions on the table and thereby
disable creation of FKs leading out of it, but it'd be pretty unusual
to want to do so.

Moreover, this definition seems neither intuitive (REFERENCES doesn't
seem like it should be symmetric) nor compliant with the SQL standard.
In SQL:2011 section 11.8 <referential constraint definition> I read

Access Rules
1) The applicable privileges for the owner of T shall include REFERENCES
for each referenced column.

(T is the referenced table.)  I see nothing suggesting that the command
requires REFERENCES privilege on the referencing table.  Now this is a
little garbled, because surely they meant the owner of the referencing
table (who is issuing the command) not the owner of the referenced table,
but I think the intent is clear enough.

In short, it seems like this statement in the docs is correctly describing
our code's behavior, but said behavior is wrong and should be changed.
I'd propose fixing it like that in HEAD; I'm not sure if the back branches
should also be changed.

            regards, tom lane

On Thu, Mar 30, 2017 at 4:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> In short, it seems like this statement in the docs is correctly describing
> our code's behavior, but said behavior is wrong and should be changed.
> I'd propose fixing it like that in HEAD; I'm not sure if the back branches
> should also be changed.

Sounds reasonable, but I don't see much advantage to changing it in
the back-branches.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Mar 30, 2017 at 4:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> In short, it seems like this statement in the docs is correctly describing
>> our code's behavior, but said behavior is wrong and should be changed.
>> I'd propose fixing it like that in HEAD; I'm not sure if the back branches
>> should also be changed.

> Sounds reasonable, but I don't see much advantage to changing it in
> the back-branches.

Well, it's a SQL-compliance bug, and we often back-patch bug fixes.

The argument for not back-patching a bug fix usually boils down to
fear of breaking existing applications, but it's hard to see how
removal of a permission check could break a working application ---
especially when the permission check is as hard to trigger as this one.
How many table owners ever revoke their own REFERENCES permission?

            regards, tom lane

On Fri, Mar 31, 2017 at 11:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Thu, Mar 30, 2017 at 4:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> In short, it seems like this statement in the docs is correctly describing
>>> our code's behavior, but said behavior is wrong and should be changed.
>>> I'd propose fixing it like that in HEAD; I'm not sure if the back branches
>>> should also be changed.
>
>> Sounds reasonable, but I don't see much advantage to changing it in
>> the back-branches.
>
> Well, it's a SQL-compliance bug, and we often back-patch bug fixes.

Personally, I'd be more inclined to view it as a definitional change.
Maybe we picked the wrong definition before, but it's well-established
behavior at this point.  The SQL standard also says that identifiers
are supposed to be folded to upper case, so I don't think the theory
that every deviation from the standard should be fixed and
back-patched holds a lot of water.

> The argument for not back-patching a bug fix usually boils down to
> fear of breaking existing applications, but it's hard to see how
> removal of a permission check could break a working application ---
> especially when the permission check is as hard to trigger as this one.
> How many table owners ever revoke their own REFERENCES permission?

Sure, but that argument cuts both ways.  If nobody ever does that, who
will be helped by back-patching this?

I certainly agree that back-patching this change is pretty low risk.
I just don't think it has any real benefits.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Mar 31, 2017 at 11:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The argument for not back-patching a bug fix usually boils down to
>> fear of breaking existing applications, but it's hard to see how
>> removal of a permission check could break a working application ---
>> especially when the permission check is as hard to trigger as this one.
>> How many table owners ever revoke their own REFERENCES permission?

> Sure, but that argument cuts both ways.  If nobody ever does that, who
> will be helped by back-patching this?
> I certainly agree that back-patching this change is pretty low risk.
> I just don't think it has any real benefits.

I think the benefit is reduction of user confusion.  Admittedly, since
Paul is the first person I can remember ever having complained about it,
maybe nobody else is confused.

            regards, tom lane

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Fri, Mar 31, 2017 at 10:40 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think the benefit is reduction of user confusion.  Admittedly, since
>> Paul is the first person I can remember ever having complained about it,
>> maybe nobody else is confused.

> ​After going back-and-forth on this (and not being able to independently
> come to the conclusion that what we are adhering to is actually a typo) I'm
> going to toss my +1 in with Robert's.

OK, the consensus seems clearly against changing this in the back branches,
so I'll just fix it in HEAD.

            regards, tom lane