Re: [PATCH] DefaultACLs

From: Petr Jelinek <pjmodos(at)pjmodos(dot)net>
To: Jan Urbański <wulczer(at)wulczer(dot)org>
Cc: Josh Berkus <josh(at)agliodbs(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Stephen Frost <sfrost(at)snowman(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PATCH] DefaultACLs
Date: 2009-09-22 10:58:41
Message-ID: 4AB8ADE1.6040206@pjmodos.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Jan Urbański napsal(a):
> Hi,
>
> here's a (late, sorry about that) review:
>
Thanks for the comprehensive review!

> It's unified, not context, but that's trivial.
>
It's not, I have git configured to produce context diffs (see
http://wiki.postgresql.org/wiki/Working_with_Git#Context_diffs_with_Git ).

> == Code ==
>
> There's a few places where the following pattern is used:
> if (!stmt->grantees)
> whereas I think the project prefers:
> stmt->grantees != NIL
>

Ok changed that.

> I'm not sure if this pattern in src/backend/catalog/aclchk.c is the best
> option:
> if (rolenames == NIL)
> rolenames = lappend(rolenames, makeString(pstrdup("")));
> if (nspnames == NIL)
> nspnames = lappend(nspnames, makeString(pstrdup("")));
> Appending an empty string and then checking in strlen of the option
> value is 0
> is ugly.
>

I changed that, I had to change the way SetDefaultACLs is called a bit
anyway (to fix the problem with dependencies you mentioned below).

> In SetDefaultACLs the OidIsValid(roleId) is not necessary, maybe better
> put in
> assert(oidIsValid). The caller always puts a valid rolename in there. Or
> maybe
> even better: make the caller pass an InvalidOid for the role if no is
> specified. This way the handling arguments is more uniform between
> SetDefaultACLs and ExecDefaultACLsStmt.
>

Same as above.

> The logic in get_default_acl and pg_namespace_object_default_acl could be
> improved, for instance get_default_acl checks if the result of the scan
> is null
> and if is, returns NULL. At the same time, the only calling function,
> pg_namespace_object_default_acl checks the isNull flag instead of just
> checking
> if the result is NULL.
>
> Also, pg_namespace_object_default_acl could just do without the isNull out
> parameter and the same goes for get_default_acl. Just return NULL to
> indicate
> an invalid result and declare a isNull in get_default_acl locally to use
> it in
> heap_getattr. This also saves some lines in InsertPgClassTuple.
>
I agree, btw it actually does not save anything in InsertPgClassTuple,
but it does save few lines in ProcedureCreate.

> Also, in InsertPgClassTuple change the order of the branches:
> + if (isNull)
> + nulls[Anum_pg_class_relacl - 1] = true;
> + else
> + values[Anum_pg_class_relacl - 1] = PointerGetDatum(relacl);
> to have the same pattern as the next if statement.
>
Done.

> In ExecDefaultACLsStmt this fragment:
> else if (strcmp(defel->defname, "roles") == 0)
> {
> if (rolenames)
> ereport(ERROR,
> (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("conflicting or redundant options")));
> drolenames = defel;
> }
> Should test if (drolenames), because currently it's possible to do:
>
Typo, fixed.

> A comment in dependency.c function getDefaultACLDescription with
> something like
> "shouldn't get here" in the default: switch branch could be useful,
> cf. getRelationDescription.
>
Ok.

> In ExecGrantDefaults_Relation there's a hunk:
> + if (isNull)
> + elog(ERROR, "no DEFAULT PRIVILEGES for relation");
> Maybe you could move it higher, no need to do other stuff if it's going
> to fail
> afterwards anyway.
>
Ok, I moved it just after owner check.

> ExecGrantDefaults_Function and ExecGrantDefaults_Relation could maybe share
> code? They look quite similar, although it might not be so easy to
> factor out
> common functionality.
>
They don't really, they operate on different tables and the only partly
shared part is the indexes and acl dependency update.

> The "unrecognized GrantStmt.objtype: %d" error message needs better
> wording I
> think.
>
Well, that exact same message is in other two places in original code, I
just copy pasted it.

> No code patch removes rows from pg_default_acls, so it might accumulate
> cruft. Maybe a drop default privileges? Or maybe revoking all would delete
> the row instead of setting it? It has the same meaning, I guess...
>
Now that I fixed DefaultACLs removal on role drop this is no longer true
(previously it was only dropped with schema a leftover from original
design).
Also revoking everything is not same as having no DefaultACLs, with no
DefaultACLs current behavior is used (owner gets all privs and public
might get something depending on object type).

> == Compiling and installing ==
>
> My gcc complains about
>
> gram.y: In function ‘base_yyparse’:
> gram.y:1128: warning: assignment from incompatible pointer type
> gram.y:1135: warning: passing argument 1 of ‘lappend’ from incompatible
> pointer type
> gram.y:1135: warning: assignment from incompatible pointer type
> gram.y:1136: warning: assignment from incompatible pointer type
>
Fixed.

> Regression tests fail because of the username mismatch
>
> ! DETAIL: drop cascades to default acls for role postgres on new
> relation in namespace regressns
> --- 951,957 ----
> ! DETAIL: drop cascades to default acls for role wulczer on new
> relation in namespace regressns
>
Removed that part from regression.

> == Testing ==
>
> The functionality worked as advertised, although I was able to do the
> following:
>
> postgres=# create role test login;
> CREATE ROLE
> postgres=# \c - test
> psql (8.5devel)
> You are now connected to database "postgres" as user "test".
> postgres=> alter default privileges grant select on table to test;
> ALTER DEFAULT PRIVILEGES
> postgres=> \c - wulczer
> psql (8.5devel)
> You are now connected to database "postgres" as user "wulczer".
> postgres=# drop role test;
> DROP ROLE
> postgres=# select * from pg_default_acls ;
> defaclrole | defaclnamespace | defaclobjtype | defacllist
> ------------+-----------------+---------------+-----------------------
> 16384 | 0 | r | {16384=arwdDxt/16384}
>
> The numeric defaclrole and defacllist don't look good.
>
Fixed, DefaultACLs for role are now dropped in DropRole.

> While testing I also got a "unexpected object type: 1248" from
> src/backend/catalog/pg_shdepend.c, but was unable to reproduce it.
> This happened after I did a combination of DROP OWNED BY and REASSIGN
> OWNED for
> a role that has membership in other roles and that all had default
> privileges
> in a schema.
>

Yeah that was because I was adding ACL dependencies and wasn't removing
them in DROP OWNED BY. Fixed. This is why I had to change the way
SetDefaultACLs is called, so it could be called from DROP OWNED BY.

> == Docs ==
>
> Need to document that FOR USER also works (as a synonym for FOR ROLE) in the
> synopsis of ALTER DEFAULT PRIVILEGES.
>
Added.

> Add examples of usage of the FOR USER/FOR ROLE syntax and explain what
> they do.
>

Added simple example.

> In the ALTER DEFAULT PRIVILEGES synopsis there's a copy-paste-o because
> it says
> you can do:
> ALTER DEFAULT PRIVILEGES IN SCHEMA s GRANT DEFAULT PRIVILEGES ON TABLE
> TO test;
>
Eh, right, fixed.

> The wording of "with grant options parameter is not applicable" in the
> sql-grant page should mentiont that you also can't do:
> GRANT DEFAULT PRIVILEGES ON s.test2 TO test;
> so it should be indicated the "TO rolename" is also not applicable.
>
Done.

> == Other ==
>
> I'm not really sure what's the use of GRANT DEFAULT PRIVILEGES... Is it for
> backward-sanitizing existing databases?
>
Yes (especially in combination with GRANT ON ALL if it gets in).

> Not tested on Windows, probably not important.
>

I did test it on Windows ;)

> Having said all that, I'm moving the patch to "Waiting on author".
>
I'll changed it back to "needs review" since I made quite a few changes
(per your review).

--
Regards
Petr Jelinek (PJMODOS)

Attachment Content-Type Size
defacl-2009-09-22.diff.gz application/x-tar 18.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message KaiGai Kohei 2009-09-22 12:23:12 Re: [PATCH] Largeobject access controls
Previous Message Simon Riggs 2009-09-22 10:29:34 Re: Hot Standby 0.2.1