Re: Misleading error "permission denied for table"

On Wed, Oct 16, 2024 at 10:11 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Nathan Bossart <nathandbossart(at)gmail(dot)com> writes:
> > On Wed, Oct 16, 2024 at 07:36:29PM +0530, Ashutosh Bapat wrote:
> >> Shouldn't we report "permission defined for column atest5.three?
>
> > We do have "permission denied for column" messages in aclchk.c (e.g.,
> > aclcheck_error_col()), but I don't see them actually used anywhere (or at
> > least they don't show up in any expected regression test output).

Attached 0001 adds a test, which triggers column permission error
message, to privileges.out. That test file has tests for GRANTing role
to role and ALTER user etc. but no test for GRANT on an object. But I
didn't find any other testfile which does that. So I think
privileges.sql is the right place for the test. I think this patch can
be committed independently.

> I'm
> > inclined to agree that a more specific error would be nice, but I worry
> > there's some hidden complexity that's prevented it thus far...
>
> See ExecCheckOneRelPerms, which seems to regard this as a TODO.
> I think the hard part is how to report the cases that involve
> pg_attribute_aclcheck_all(..., ACLMASK_ANY), which means
>
> * If 'how' is ACLMASK_ANY, then returns ACLCHECK_OK if user has any of the
> * privileges identified by 'mode' on any non-dropped column in the relation;
>
> so that you can't really finger a specific column as being in
> violation. Maybe we could leave those cases as just mentioning the
> rel; not sure if that leads to failing to move the goalposts much.

I think, it might just suffice to say "permission denied for relation
\"%s\" and all of its columns" in the error with a hint ""require
permission on the relation or any of its columns" as done in patch
0002. What do you think?

> Otherwise, we could extend ExecCheckOneRelPerms and
> pg_attribute_aclcheck_all to pass back a column number, and
> then modify the error reporting in ExecCheckPermissions.
>

The attached patch does this. To select or modify a column, a user
needs to have relevant permission on the table and/or the column. So
the error message should mention both. That's what the patch does, it
reports the first column without the required privileges. With that
all the errors resulting from SELECT/INSERT/UPDATE report both the
name of the table and the first column without permission. E.g.
following change in the attached patch
INSERT INTO base_tbl VALUES (3, 'Row 3', 3.0); -- not allowed
- ERROR: permission denied for table base_tbl
+ ERROR: permission denied for relation "base_tbl" as well as its column "a"

That's technically correct but may not be as useful since the user may
not intend to add column privileges at all OR if they add column
privilege on "a", the statement will still error out mentioning column
"b" next. Let's call it approach 1.

To fix that we could take advice from TODO in ExecCheckOneRelPerms()
and see if there are other columns with column privileges. Existence
of such columns indicates that the user's intention is to add column
privileges and they are missing privileges on one or more of them. The
result would be much better, however
1. we can't stop checking permissions on the first failure as we do today
2. the columns for which column privileges are set may not be
referenced in the query and thus we have to look at all the column
privileges irrespective of whether those are referenced or not. That
looks like a lot of unnecessary work.
Let's call this approach 2.

Approach 3 is a heuristic. We stop at the first failed column
permission check but while doing so note if we found a column with
required permissions. If such a column exists, we mention both the
table and the first column for which permission check failed in the
error message. Otherwise we just mention the table in the error
message. In case the user is missing column permissions, it's quite
likely that they are missing it on a later column instead of very
first one. So this heuristic will mostly work. Downside is the error
message depends upon the columns mentioned in the query - thus may
change from query to query and has potential to cause confusion.

Approach 4 is to leave the error message as is but provide a hint
"require permissions on the relation or all the columns referenced in
the query". That's simplest and enough to set the user on the right
path, though not strictly accurate.

I am leaning towards 3 or 4 but open to suggestions (changes to above
approaches or new options).

--
Best Wishes,
Ashutosh Bapat