Re: has_privs_of_role vs. is_member_of_role, redux

On Thu, Aug 25, 2022 at 4:41 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Yeah, I'd lean against back-patching. This is the sort of behavioral
> change that users tend not to like finding in minor releases.

Here's a small patch. Despite the small size of the patch, there are a
couple of debatable points here:

1. Should we have a code comment? I feel it isn't necessary, because
there's a comment just a few lines earlier saying "Look up the role
OIDs and do permissions checks" and that seems like sufficient
justification for what follows.

2. What about the error message? Personally, I'm not very excited
about "permission denied to whatever" as a way to phrase an error
message. It doesn't sound like particularly good grammar to me. But
it's the phrasing we use elsewhere, so I guess we should do the same
here.

3. Should we have a test case? We are extremely thin on test cases for
NOINHERIT behavior, it seems, and testing this one thing when we don't
test anything else seems relatively useless. Also, privileges.sql is a
giant mess. It's a 1700+ line test file that tests many fairly
unrelated things. I am inclined to think that this file badly needs to
be split up into a bunch of smaller files, because it's practically
unmaintainable as is. For instance, the stuff at the top of the file
is testing a bunch of things about role privileges, but then check
some stuff about leakproof functions before coming back to test stuff
about groups, which logically probably belongs with the role
privileges stuff. Perhaps a reasonable starting split would be
something like:

- Privileges on roles.
- Privileges on relations.
- Privileges on other kinds of objects.
- Default privileges.
- Security barriers and leakproof functions.
- Security-restricted operations.

Some of those files might be fairly small initially, but they might be
get bigger later, especially because it'd be a heck of a lot easier to
add new test cases if you didn't have to worry that some change you
make is going to break a test 1000 lines down in the file.

--
Robert Haas
EDB: http://www.enterprisedb.com