Re: RangeVarGetRelid()

On Mon, Dec 19, 2011 at 01:43:18PM -0500, Robert Haas wrote:
> On Mon, Dec 19, 2011 at 11:55 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > Here's the rule I used: the pre-lock checks must prove that the user could, by
> > some command working as-designed, have taken a lock at least as strong as the
> > one we're about to acquire. ?How does that work out in practice? ?Relation
> > owners can always take AccessExclusiveLock by way of a DROP command, so
> > ownership is always sufficient as a pre-lock test.
> >
> > The above namespace check is no exception; the object owner has authority to
> > take the AccessExclusiveLock independent of his authority to ultimately
> > complete the rename. ?(Changes arising from the recent discussions about
> > locking namespaces during DDL would complicate the situation in other ways.)
>
> Yes: and I have it mind to work on that for the current release cycle,
> time permitting. It seems to me that there's no real value in
> consolidating this callback if there's a change coming down the pipe
> that would require us to split it right back out again.
>
> Also, while I see the point that it wouldn't be a security issue to
> defer this particular check until after the lock is taken, I still
> don't think it's a good idea. I basically don't see any point in
> splitting up the security checks across multiple functions. Our
> permissions-checking logic is already rather diffuse; I think that
> finding more reasons for some of it to be done in one function and
> some of it to be done in some other function is going in the wrong
> direction. It makes it easier for future people editing the code to
> rearrange things in ways that subtly break security without realizing
> it, and it's also one more obstacle for things like sepgsql which
> would like to get control whenever we do a security check. (Dimitri's
> recent comments about command triggers suggest that MAC isn't the only
> application of an option to override the default access control
> policy.) It's also not terribly consistent from a user perspective:
> hmm, if I don't have permission to do operation X because of reason A,
> then my command fails immediately; if I don't have permission because
> of reason B, then it waits for a lock and then fails. Some amount of
> inconsistency there is probably inevitable, because, for example, we
> won't know whether you have permission on an entire inheritance
> hierarchy until we start walking it. But I don't really see the point
> in going out of our way to add more of it.
>
> I think it's important to keep in mind that most of the code that is
> going into those callbacks is just being moved. We're not really
> ending up with any more code overall, except to the extent that there
> are a couple of lines of net increase for each callback because, well,
> it has a comment, and maybe a declaration, and some curly braces at
> the beginning and the end. The ownership check is two lines of code;
> it doesn't matter whether we duplicate that or not. In many cases, I
> think the callbacks are actually increasing readability, by pulling a
> bunch of related checks into their own function instead of cluttering
> up the main code path with them. I don't want to end up with a lot of
> needless code duplication, but I also don't feel any powerful need to
> squeeze the number of callback functions down to an absolute minimum
> just because I can.

I'm satisfied that you and I have a common understanding of the options and
their respective merits. There's plenty of room for judgement in choosing
which merits to seize at the expense of others. Your judgements here seem
entirely reasonable.

nm