(2010/01/24 11:27), Robert Haas wrote:
On Sat, Jan 23, 2010 at 8:33 PM, KaiGai Kohei<[email protected]> wrote:
(2010/01/24 9:08), Robert Haas wrote:
On Sat, Jan 23, 2010 at 2:17 AM, KaiGai Kohei<[email protected]> wrote:
However, it is unclear for me whether the revised ATSimplePermissions()
provide cleaner code than currently we have, because it also needs
a big switch ... case statement within.
Am I misunderstanding something?
Well, not everyone is going to agree on what "cleaner code" means in
every case, but the reason that I like my design better is because it
moves all of the decision making out of ATPrepCmd() into
ATSimplePermissions(). What you're proposing would mean that
ATPrepCmd() would basically continue to know everything about which
operations need which permissions checks, which I don't think is going
to scale very well to alternative security providers, if we eventually
decide to support such a thing.
Hmm. Indeed, the existing ATPrepCmd() closely combines the permission
checks and controls of code path (inheritance recursion and AT_PASS_*),
and it is worthwhile to divide these independent logic into two.
Yeah, that's what I thought, too.
In your plan, where the new ATSimplePermissions() should be called?
From ATPrepCmd(), just before the big switch statement.
If we still call it from the ATPrepCmd() stage, it needs to apply
special treatments some of operations with part-B logic.
Not sure if I understand what you mean. ATSimplePermissions() won't
be responsible for applying permissions checks related to other
objects upon which the command is operating (e.g. the other table, if
adding a foreign key). It will however be responsible for knowing
everything about which permission checks to apply to the main table
involved, which will require some special-case logic for certain
command types.
One other candidate of the entrypoint is the head of each ATExecXXXX()
functions. [...snip...]
I don't think this is a good idea. Calling it in just one place seems
less error-prone and easier to audit.
Yes, most of the ALTER TABLE options runs ATPrepCmd() except for RENAME
TO and SET SCHEMA, so it is a good candidate to apply less error-prone
permission checks.
The reason why I introduced this alternative idea is from the perspective
of simple basis/concept about where we should apply permission checks,
although it needs larger number of entrypoints compared to head of the
ATPrepCmd().
If we put the new ATSimplePermissions() with all the needed information
just after gathering them at the execution stage, we don't need to have
some of exceptions which takes additional checks except for ownership
on the relation to be altered.
Of course, code simpleness is important. Likewise, I think simpleness in
basis/concept (less number of special treatments) is also important.
I'd like to introduce it using a pseudo code.
:
It will allow to eliminate self recursion in ATAddCheckConstraint() and
to apply permission checks for new CHECK constraint in ATPrepCmd() phase.
Perhaps, it may be consolidated to ATPrepAddConstraint().
I don't really see that this gains us anything.
Hmm, indeed, here is no more benefit except for eliminating one self recursion.
Thanks,
--
KaiGai Kohei <[email protected]>
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
