Reserved roles and user mapping

Amit,

* Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:

Currently in CreateUserMapping():

/* Additional check to protect reserved role names */
check_rolespec_name(stmt->user,
"Cannot specify reserved role as mapping user.");

User mapping terminology is not that clear to me really but how does the
following sound as detail message:

"Cannot create mapping for reserved roles" or "Cannot create reserved role
mapping"

I'm open to changing that, though I don't think the existing terminology
is all that bad either.

Also then, are checks for reserved role specification in
AlterUserMapping() and RemoveUserMapping() really necessary?

/* Additional check to protect reserved role names */
check_rolespec_name(stmt->user,
"Cannot alter reserved role mapping user.");

/* Additional check to protect reserved role names */
check_rolespec_name(stmt->user,
"Cannot remove reserved role mapping user.");

That was intentional as I was covering all cases where
get_rolespec_oid/tuple() are used to convert rolename->OID for any
purpose, as a method of trying to ensure coverage of all code paths.
Creation of user mappings are different from most objects in that they
are explicitly created, rather than created with the implicit assumption
that the creator owns them, which is why this is a bit different than
the other cases.

Messages output in those cases are:

ERROR: role "pg_signal_backend" is reserved
DETAIL: Cannot alter reserved role mapping user.

ERROR: role "pg_signal_backend" is reserved
DETAIL: Cannot remove reserved role mapping user.

Whereas, the following would seem more natural:

ERROR: user mapping "pg_signal_backend" does not exist for the server

Perhaps. I can almost imagine a case where we ship a default role which
has a pre-defined mapping to the "local" server for the purpose of
accessing another database through postgres_fdw (which has since become
included by default, similar to plpgsql). Yeah, that's a reallllly long
stretch, but I'm not really a fan of remove those checks from this code
path.

On the other hand, perhaps we could move those checks to after the
lookup for the user mapping, thus getting the above error message
(unless we do end up with such a mapping in PostgreSQL 16) and not
leaving the paths un-checked.

Thanks!

Stephen

Hi Stephen,

On 2016/04/14 9:24, Stephen Frost wrote:

Amit,

* Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:

Currently in CreateUserMapping():

/* Additional check to protect reserved role names */
check_rolespec_name(stmt->user,
"Cannot specify reserved role as mapping user.");

User mapping terminology is not that clear to me really but how does the
following sound as detail message:

"Cannot create mapping for reserved roles" or "Cannot create reserved role
mapping"

I'm open to changing that, though I don't think the existing terminology
is all that bad either.

Sure, no problem if you think so.

Also then, are checks for reserved role specification in
AlterUserMapping() and RemoveUserMapping() really necessary?

/* Additional check to protect reserved role names */
check_rolespec_name(stmt->user,
"Cannot alter reserved role mapping user.");

/* Additional check to protect reserved role names */
check_rolespec_name(stmt->user,
"Cannot remove reserved role mapping user.");

That was intentional as I was covering all cases where
get_rolespec_oid/tuple() are used to convert rolename->OID for any
purpose, as a method of trying to ensure coverage of all code paths.
Creation of user mappings are different from most objects in that they
are explicitly created, rather than created with the implicit assumption
that the creator owns them, which is why this is a bit different than
the other cases.

Messages output in those cases are:

ERROR: role "pg_signal_backend" is reserved
DETAIL: Cannot alter reserved role mapping user.

ERROR: role "pg_signal_backend" is reserved
DETAIL: Cannot remove reserved role mapping user.

Whereas, the following would seem more natural:

ERROR: user mapping "pg_signal_backend" does not exist for the server

Perhaps. I can almost imagine a case where we ship a default role which
has a pre-defined mapping to the "local" server for the purpose of
accessing another database through postgres_fdw (which has since become
included by default, similar to plpgsql). Yeah, that's a reallllly long
stretch, but I'm not really a fan of remove those checks from this code
path.

On the other hand, perhaps we could move those checks to after the
lookup for the user mapping, thus getting the above error message
(unless we do end up with such a mapping in PostgreSQL 16) and not
leaving the paths un-checked.

I see. I think your this comment brings home the point about user
mappings and default roles for me. So AIUI, user mappings of certain
default role for remote DB access would be created during initdb and
altering/dropping them would be prohibited which is the point of having
these checks. I don't however clearly understand what that implies for
other default roles (pg_*).

Thanks,
Amit

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers