Обсуждение: Reserved roles and user mapping
Hi, Currently in CreateUserMapping(): /* Additional check to protect reserved role names */ check_rolespec_name(stmt->user, "Cannotspecify 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" 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, "Cannotalter reserved role mapping user."); /* Additional check to protect reserved role names */ check_rolespec_name(stmt->user, "Cannotremove reserved role mapping user."); 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 Thanks, Amit
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