Refactor user management #1052
Refactor user management #1052
Conversation
As described in pgbouncer#484, if a user is defined in the [users] section of the config file (perhaps to take advantage of per-user overrides) but then that user is *not* defined in auth_file, pgBouncer currently gets confused when trying to check their password. It sees the user exists, but fails to notice a password was never defined, resulting in no running of auth_query.
As described in pgbouncer#484, if a user is defined in the [users] section of the config file (perhaps to take advantage of per-user overrides) but then that user is *not* defined in auth_file, pgBouncer currently gets confused when trying to check their password. It sees the user exists, but fails to notice a password was never defined, resulting in no running of auth_query. Add a test case to catch this. The case successfully fails before this patchset and succeeds after it.
As described in pgbouncer#484, if a user is defined in the [users] section of the config file (perhaps to take advantage of per-user overrides) but then that user is *not* defined in auth_file, pgBouncer currently gets confused when trying to check their password. It sees the user exists, but fails to notice a password was never defined, resulting in no running of auth_query. Add a test case to catch this. The case successfully fails before this patchset and succeeds after it.
As described in pgbouncer#484, if a user is defined in the [users] section of the config file (perhaps to take advantage of per-user overrides) but then that user is *not* defined in auth_file, pgBouncer currently gets confused when trying to check their password. It sees the user exists, but fails to notice a password was never defined, resulting in no running of auth_query.t Add a test case to catch this. The case successfully fails before this patchset and succeeds after it.
This reduces checks that must be made throughout the code, though it does not reduce the need to know which record is the proper one (user or user->cf_user) to use. Also, pre-emptively make uncrustify happy *before* github has a go at it.
The comments are more accurate, and the unit test now runs against freebsd as well as other OSs.
There was a problem hiding this comment.
I think it's a fine refactor. I do think there's more work to do if this is the direction - a lot of field names that used to be of type PgUser are still named some permutation of "user", even though they are now storing credential info instead of the new GlobalUser type.
include/bouncer.h
Outdated
| @@ -326,7 +327,7 @@ struct PgPool { | |||
| struct List map_head; /* entry in user->pool_list */ | |||
|
|
|||
| PgDatabase *db; /* corresponding database */ | |||
| PgUser *user; /* user logged in as, this field is NULL for peer pools */ | |||
| PgAuthInfo *user; /* user logged in as, this field is NULL for peer pools */ | |||
There was a problem hiding this comment.
Given GlobalUser as a type, I am tempted to suggest this field should be called auth_info or credentials instead of user, to reduce confusion. (And yes, this implies a fair amount of substitutions elsewhere.)
In general, how do you feel about never using "user" for pgAuthInfo data, and keeping that reserved for GlobalUser types?
There was a problem hiding this comment.
Yeah, it's definitely something I think we should be consistent about and was originally trying to do what you are suggesting. But when I got to the auth_user field, I didn't really like the resulting auth_user_auth_info name.
So I held off on changing everything for now, because maybe it fits better to rename PgAuthInfo to PgUser
There was a problem hiding this comment.
What about s/PgAuthInfo/PgCredentials/, so that you end up with auth_user_credentials or the like?
There was a problem hiding this comment.
I like that suggestion. I'll try that out tomorrow.
There was a problem hiding this comment.
Okay, I finally found the time to do this. Could you take another look? I'm pretty happy with the result.
doc/config.md
Outdated
| @@ -1255,6 +1256,12 @@ Example: | |||
|
|
|||
| Only a few settings are available here. | |||
|
|
|||
| Note that when `auth_file` configured, if a user is defined in this section but | |||
There was a problem hiding this comment.
...auth_file is configured...
include/bouncer.h
Outdated
| bool dynamic_passwd; /* does the password need to be refreshed every use */ | ||
|
|
||
| /* | ||
| * global_user points at the global user that is used for configuration |
There was a problem hiding this comment.
...global user which is used...
include/bouncer.h
Outdated
|
|
||
| /* | ||
| * The global user is used for configuration settings and connection count. It | ||
| * includes credentials, but this are empty if the user is not configured in |
Based upon #1039, but changed a lot by splitting the
PgUsertype into two different types. First we introduce aPgGlobalUsertype which stores user settings from the ini file, credentials inauth_fileand connection count tracking. And secondly we introducePgCredentialswhich only stores user credentials, these could either come from theauth_fileorauth_query. This type is embedded in thePgGlobalUserto store any global credentials fromauth_file, and it also links back to the matchingPgGlobalUser.This makes user configuration work for dynamically created users and will also list dynamically created users in the output of
SHOW USERS.Fixes #484
Fixes #706
Resolves #1039