Re: [PATCH] ACE Framework - Database, Schema

I read through the database patch a little more. Here are some
further thoughts.

ace_database_create() calls have_createdb_privilege(), but of course
what ace_database_create() is supposed to be checking is whether you
have privileges to create the DB. This is extremely confusing. The
calling sequence for ace_database_create() seems to indicate a failure
of abstraction. The srcIsTemplate argument seems quite problematic.
It is one of many values that are returned by get_db_info(), and it's
passed to ace_database_create() because the current PG model happens
to care. It seems like a near-certainty that future security models
will care about something else. (Incidentally, the interface to the
existing get_db_info function seems to me to be rather poor. By the
time we get up to 10 out parameters, I think we ought to be using a
structure. This might be material for a standalone patch.)

I don't understand the modifications to restrict_and_check_grant().
It seems to me that this is going in the wrong direction, although
since I don't understand the motivation for the change, I might be
wrong. If we have a piece of code that centralizes permissions
checking now, splitting that up into a bunch of ace_*_grant()
functions and duplicating it doesn't seem like an improvement.

ace_database_alter() appears to never be called with more than one
argument that is non-NULL/InvalidOid, which is usually a sign that the
interface is wrong. There are two calls to this function where,
apparently, we're checking alter database permissions without
specifying any details whatsoever about exactly what's going to get
altered. That sounds like we don't really know what things we want to
pass across the interface layer, so we're just passing the ones that
we happen to care about at the moment. It certainly looks like we
need separate functions for alter, rename, and alter-set. In some
cases it might make sense to pass the parsenode as an argument rather
than trying to decide which bits of data contained within it are
sufficiently interesting to be worth sending over.

Tom objected to ace_database_calculate_size() before. It seems to me
that in order to keep this managable we have to settle on a finite set
of permissions and stick with it. For example, in this case, we might
decide that in EVERY security model, calculating the database size
requires the same permissions as connect. The individual security
models might want to make different decisions about how to check that
permission, but they all have to treat it the same way they would
treat an actual connection attempt. It seems to me that if every
security model is allowed to introduce not only new logic for making
checks but also new kinds of checks, we might as well give up on
designing an interface layer.

(Similarly, skipping back to ace_database_create() for a minute ...
since I asked for a patch that only deals with one object type, I
won't now complain about having gotten one. But I will note that I
think where ace_database_create() calls pg_tablespace_aclchk() it
probably eventually needs to call some ace_tablespace_...() function.
Although frankly I'm not sure which one. Would
ace_tablespace_create() mean permission to create a tablespace or
permission to create tables within that tablespace?)

security/ace.h, like a good number of other things in this patch, does
not conform to project indentation style. All of the parameter blocks
(parameter : description) don't either; unless I'm mistaken, pgindent
will do something awful to those. The comments generally need some
editing help from a native English speaker, and I spotted at least two
typos (defatult for default, provides for providers). They also make
reference to multiple security providers, which is not within the
purview of this patch.

I am not very happy with the ace_<object-type>_<operation> naming
convention. Most PG functions are named a little more descriptively
than that. Like I can pretty much guess that AlterDatabase() is going
to alter a database, and get_db_info() is going to get info about a
DB, and ExecHashJoin() is going to execute a hash join. It's not
possible to look at ace_database_create() and have any idea what the
function does, unless you have the preexisting knowledge that ACE
stands for "access control extensions", and then you still don't know
what the function does because "access control extensions database
create" is a sentence with no verb. Granted, we have some poorly
named functions in the system already, but I'd like to not increase
the number. Apart from all of the above, I still believe that for a
first phase of this we should just be looking to centralize access
control decisions without promising to ever do anything more, and
"access control extensions" doesn't send that message. I don't know
exactly what the best way to structure this is but I think putting the
word "check" somewhere in there would be a good start (or maybe the
already-used-elsewhere abbreviation "chk").

Again, I want to provide what help I can here, but this is a massive
project, so help is really needed.

...Robert