Re: [PATCH] Reworks for Access Control facilities (r2311)

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> Could you post any review comments, even if it is not comprehensive yet?

In general, you don't need to preface your comments with 'MEMO:'. I
would encourage removing that. You might use 'FIXME:' instead, if it is
something which needs to be corrected in the future. Additionally,
please think about PG as a whole project. Talking about 'native
privilege mechanism' implies there are 'native' and 'foreign' ones. I
would recommend using the term 'default' instead. Also, rather than
saying it is "not a correct way" when referring to decisions made about
the permissions checks for the 'default' mechanism, just say it's the
default way and that other modules might not implement it the same.
Saying it's "not a correct way" implies a problem with the existing
code. If that's true, then it should be addressed separatly from this
patch.

Specifically, in LookupExplicitNamespace, you added:

---------------------
MEMO: The native privilege mechanism always allows everyone to apply
ACL_USAGE permission on the temporary namespaces implicitly, so it was
omitted to check the obvious one here. But it is a heuristic, not a
correct way.
---------------------

My recommendation for how to reword this, if it's accurate, is:
---------------------
By default, everyone is permitted ACL_USAGE on temporary namespaces
implicitly, so a check for it was omitted here. Other security models
may wish to implement a check, so call ac_schema_search() to check.
---------------------

You might also provide a specific example of where and why this check
matters. I'm not entirely convinced it's necessary or makes sense, to
be honest..

Also, does it make sense to still have LookupCreationNamespace? Given
its charter and the changes, is it still different from
LookupExplicitNamespace?

I would recommend adding back the comments above the calls to
ac_*_grant() which explicitly say:

---------------------
If we found no grant options, consider whether to issue a hard error.
Per spec, having any privilege at all on the object will get you by
here.
---------------------

The issue here is to make it clear to any callers that ac_*_grant() does
not check if everything being attempted will succeed but rather if
any one thing will. Same thing with the comments above the ac_*_grant()
functions themselves.

I'm not sure that the comment in restrict_grant() regarding this is
really necessary, considering that post-change there shouldn't be a
pg_aclmask() anymore, so referring to it in a comment seems odd.

In general, I like what you've done with splitting
restrict_and_check_grant() up.

Regarding your comment in dependency.c about objects which are dropped
due to dependencies:

Have you already developed the code to resolve this issue? Is there
additional information required to check if the object can be dropped?
Should the ac_object_drop() just call the regular drop routines? But
they'd have to have a flag added which indicates if it's a dependency
drop or not, right? If the regular drop routine isn't called, then what
would ac_object_drop() use to determine if the drop is permitted or not?

My concern here is that we don't want to develop a new API and then
immediately discover that it doesn't cover the cases we need it for.
If you have already developed an ac_object_drop() which would work for
existing PG and would be easily changed to support SE, I would recommend
you include it in this patch.

I don't find the comment regarding what happened with FindConversion to
be nearly descriptive enough. Can you elaborate on why the check wasn't
necessary and has now been removed? If it really isn't needed, why have
that function at all?

Regarding OperatorCreate, it looks like there may be some functional
changes. Specifically, get_other_operator() could now be called even if
a given user doesn't own the operator shell he's trying to fill out. It
could then error-out with the "operator cannot be its own negator or
sort operator" error. I'm not sure that's a big deal at all, but it's a
difference which we might want to document as expected. This assumes,
of course, that such a situation could really happen. If I'm missing
something and that's not possible, let me know.

The new 'permission' argument to ProcedureCreate should be documented in
the function definition, or at least where it's used, as to what it's
for and why it's needed and why ac_proc_create is conditional on it.

Again, regarding shdepend, if you have the code already which works with
the default permissions model for PG, you should include it in this
patch as part of the API. I realize this contradicts a bit what I said
earlier, but the main concern here is making sure that it will actually
work for SEPG. If you vouch that it will, then perhaps we don't need to
add them now, but I would probably also remove the comments then. One
or the other.. Let's not add comments which will immediately be
removed, assuming everything goes well.

There is a similar change in CreateConversionCommand. Again, I don't
think it's a big deal, but I wonder if we should make a decision about
if permission checks should be first or last and then be consistant
about it. My gut feeling is that we should be doing them first and
doing all of them, if at all possible.. There are a couple of other
places like this.

I'm also concerned about the inconsistancy regarding if the roleOid is
passed into the function of if GetUserId() is used. I would recommend
being consistant with this. Either GetUserId() is always 'good enough',
or it's not, and we should require the roleOid to be passed into all of
the ac_* functions. I'm tending towards the latter, since it appears to
be necessary in some cases. If there is some division of the ac_*
functions where it's consistant within a division, that might be
alright, but it should be discussed somewhere.

I've glanced through the rest and in general I feel like it's starting
to look good. Thanks for your efforts towards this. I'd really like to
see this get committed eventually.

Thanks!

Stephen