pgsql: Refer to a TOKEN_USER payload as a "token user, " not as a "user
Refer to a TOKEN_USER payload as a "token user," not as a "user token".
This corrects messages for can't-happen errors. The corresponding "user
token" appears in the HANDLE argument of GetTokenInformation().
Branch
------
master
Details
-------
http://git.postgresql.org/pg/commitdiff/c22650cd6450854e1a75064b698d7dcbb4a8821a
Modified Files
--------------
src/backend/libpq/auth.c | 2 +-
src/common/exec.c | 7 ++-----
2 files changed, 3 insertions(+), 6 deletions(-)
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
* Noah Misch (noah@leadboat.com) wrote:
Refer to a TOKEN_USER payload as a "token user," not as a "user token".
This corrects messages for can't-happen errors. The corresponding "user
token" appears in the HANDLE argument of GetTokenInformation().
I'm not at all convinced that this is an improvement. I understand that
it's a "can't happen" case, but we're calling out to a OS function and
as much as things "can't happen" they do, in fact, occationally happen,
and there's no such thing as a "token user" concept. There's an enum,
one value of which is "TokenUser" and that's what we're asking the OS to
provide us info about, but I'd argue that if we're going to refer to the
textual enum representation then we should spell it just exactly as the
enum has it.
If we don't want to use "TokenUser" then I'd suggest that "user token"
is a more accurate term to use, as we had before this change. There is
no such thing as a "token user", as far as I'm aware, in GSSAPI, SSPI,
or general access token lingo.
Thanks!
Stephen
On Fri, Apr 01, 2016 at 10:12:12PM -0400, Stephen Frost wrote:
* Noah Misch (noah@leadboat.com) wrote:
Refer to a TOKEN_USER payload as a "token user," not as a "user token".
This corrects messages for can't-happen errors. The corresponding "user
token" appears in the HANDLE argument of GetTokenInformation().I'm not at all convinced that this is an improvement. I understand that
it's a "can't happen" case, but we're calling out to a OS function and
as much as things "can't happen" they do, in fact, occationally happen,
They do, yes. I mentioned that for the purpose of hinting that this commit
does not warrant release notes coverage.
and there's no such thing as a "token user" concept. There's an enum,
one value of which is "TokenUser" and that's what we're asking the OS to
provide us info about, but I'd argue that if we're going to refer to the
textual enum representation then we should spell it just exactly as the
enum has it.If we don't want to use "TokenUser" then I'd suggest that "user token"
is a more accurate term to use, as we had before this change. There is
no such thing as a "token user", as far as I'm aware, in GSSAPI, SSPI,
or general access token lingo.
"User token" has definitely been wrong. We already possess the user token at
the moments of these error messages, because we pass the user token as the
first GetTokenInformation() argument. We're retrieving information about the
"user" that pertains to a particular "token", hence "token user." A verbose
description is "could not get user associated with access token."
I see some advantages of writing "TokenUser", as you propose. However, our
error style guide says "Avoid mentioning called function names, either;
instead say what the code was trying to do." Mentioning an enumerator name is
morally similar to mentioning a function name.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Noah Misch (noah@leadboat.com) wrote:
On Fri, Apr 01, 2016 at 10:12:12PM -0400, Stephen Frost wrote:
and there's no such thing as a "token user" concept. There's an enum,
one value of which is "TokenUser" and that's what we're asking the OS to
provide us info about, but I'd argue that if we're going to refer to the
textual enum representation then we should spell it just exactly as the
enum has it.If we don't want to use "TokenUser" then I'd suggest that "user token"
is a more accurate term to use, as we had before this change. There is
no such thing as a "token user", as far as I'm aware, in GSSAPI, SSPI,
or general access token lingo."User token" has definitely been wrong. We already possess the user token at
the moments of these error messages, because we pass the user token as the
first GetTokenInformation() argument. We're retrieving information about the
"user" that pertains to a particular "token", hence "token user." A verbose
description is "could not get user associated with access token."
Ok, "user token information" would still be better than "token user"
which has a completely different connotation, as I see it.
I see some advantages of writing "TokenUser", as you propose. However, our
error style guide says "Avoid mentioning called function names, either;
instead say what the code was trying to do." Mentioning an enumerator name is
morally similar to mentioning a function name.
That's a fair point, but it doesn't mean we should use a different
spelling for the enumerator name to avoid that piece of the policy. I
certianly don't see "token user" as saying "what the code was trying to
do" in this case.
Thanks!
Stephen
Stephen Frost <sfrost@snowman.net> writes:
* Noah Misch (noah@leadboat.com) wrote:
I see some advantages of writing "TokenUser", as you propose. However, our
error style guide says "Avoid mentioning called function names, either;
instead say what the code was trying to do." Mentioning an enumerator name is
morally similar to mentioning a function name.
That's a fair point, but it doesn't mean we should use a different
spelling for the enumerator name to avoid that piece of the policy. I
certianly don't see "token user" as saying "what the code was trying to
do" in this case.
FWIW, "token user" conveys entirely inappropriate, politically incorrect
connotations to me ;-). I don't have any great suggestions on what to use
instead, but I share Stephen's unhappiness with the wording as-committed.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Apr 01, 2016 at 11:07:01PM -0400, Tom Lane wrote:
Stephen Frost <sfrost@snowman.net> writes:
* Noah Misch (noah@leadboat.com) wrote:
I see some advantages of writing "TokenUser", as you propose. However, our
error style guide says "Avoid mentioning called function names, either;
instead say what the code was trying to do." Mentioning an enumerator name is
morally similar to mentioning a function name.That's a fair point, but it doesn't mean we should use a different
spelling for the enumerator name to avoid that piece of the policy. I
certianly don't see "token user" as saying "what the code was trying to
do" in this case.FWIW, "token user" conveys entirely inappropriate, politically incorrect
connotations to me ;-). I don't have any great suggestions on what to use
instead, but I share Stephen's unhappiness with the wording as-committed.
The wording in GetTokenUser() and AddUserToTokenDacl() seems fine; let's
standardize on that. Also, every GetTokenUser() failure has been yielding two
messages, the second contributing no further detail. I'll reduce that to the
usual one message per failure.
nm
Attachments:
token-msg-dedup-v1.patchtext/plain; charset=us-asciiDownload
diff --git a/src/common/exec.c b/src/common/exec.c
index ec8c655..d736b02 100644
--- a/src/common/exec.c
+++ b/src/common/exec.c
@@ -674,10 +674,7 @@ AddUserToTokenDacl(HANDLE hToken)
/* Get the current user SID */
if (!GetTokenUser(hToken, &pTokenUser))
- {
- log_error("could not get token user: error code %lu", GetLastError());
- goto cleanup;
- }
+ goto cleanup; /* callee printed a message */
/* Figure out the size of the new ACL */
dwNewAclSize = asi.AclBytesInUse + sizeof(ACCESS_ALLOWED_ACE) +
token-user-v1.patchtext/plain; charset=us-asciiDownload
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 2751183..f21056e 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -1242,8 +1242,8 @@ pg_SSPI_recvauth(Port *port)
if (!GetTokenInformation(token, TokenUser, NULL, 0, &retlen) && GetLastError() != 122)
ereport(ERROR,
- (errmsg_internal("could not get token user size: error code %lu",
- GetLastError())));
+ (errmsg_internal("could not get token information buffer size: error code %lu",
+ GetLastError())));
tokenuser = malloc(retlen);
if (tokenuser == NULL)
@@ -1252,8 +1252,8 @@ pg_SSPI_recvauth(Port *port)
if (!GetTokenInformation(token, TokenUser, tokenuser, retlen, &retlen))
ereport(ERROR,
- (errmsg_internal("could not get token user: error code %lu",
- GetLastError())));
+ (errmsg_internal("could not get token information: error code %lu",
+ GetLastError())));
CloseHandle(token);
diff --git a/src/port/win32security.c b/src/port/win32security.c
index d3eba9a..ab9cd67 100644
--- a/src/port/win32security.c
+++ b/src/port/win32security.c
@@ -248,14 +248,14 @@ pgwin32_get_dynamic_tokeninfo(HANDLE token, TOKEN_INFORMATION_CLASS class,
if (GetTokenInformation(token, class, NULL, 0, &InfoBufferSize))
{
snprintf(errbuf, errsize,
- "could not get token information: got zero size\n");
+ "could not get token information buffer size: got zero size\n");
return FALSE;
}
if (GetLastError() != ERROR_INSUFFICIENT_BUFFER)
{
snprintf(errbuf, errsize,
- "could not get token information: error code %lu\n",
+ "could not get token information buffer size: error code %lu\n",
GetLastError());
return FALSE;
}
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 416829d..dcafcf1 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -904,7 +904,7 @@ current_windows_user(const char **acct, const char **dom)
if (!GetTokenInformation(token, TokenUser, NULL, 0, &retlen) && GetLastError() != 122)
{
fprintf(stderr,
- _("%s: could not get token user size: error code %lu\n"),
+ _("%s: could not get token information buffer size: error code %lu\n"),
progname, GetLastError());
exit(2);
}
@@ -912,7 +912,7 @@ current_windows_user(const char **acct, const char **dom)
if (!GetTokenInformation(token, TokenUser, tokenuser, retlen, &retlen))
{
fprintf(stderr,
- _("%s: could not get token user: error code %lu\n"),
+ _("%s: could not get token information: error code %lu\n"),
progname, GetLastError());
exit(2);
}
* Noah Misch (noah@leadboat.com) wrote:
On Fri, Apr 01, 2016 at 11:07:01PM -0400, Tom Lane wrote:
Stephen Frost <sfrost@snowman.net> writes:
* Noah Misch (noah@leadboat.com) wrote:
I see some advantages of writing "TokenUser", as you propose. However, our
error style guide says "Avoid mentioning called function names, either;
instead say what the code was trying to do." Mentioning an enumerator name is
morally similar to mentioning a function name.That's a fair point, but it doesn't mean we should use a different
spelling for the enumerator name to avoid that piece of the policy. I
certianly don't see "token user" as saying "what the code was trying to
do" in this case.FWIW, "token user" conveys entirely inappropriate, politically incorrect
connotations to me ;-). I don't have any great suggestions on what to use
instead, but I share Stephen's unhappiness with the wording as-committed.The wording in GetTokenUser() and AddUserToTokenDacl() seems fine; let's
standardize on that. Also, every GetTokenUser() failure has been yielding two
messages, the second contributing no further detail. I'll reduce that to the
usual one message per failure.
This approach works for me.
Thanks!
Stephen
Stephen Frost <sfrost@snowman.net> writes:
* Noah Misch (noah@leadboat.com) wrote:
The wording in GetTokenUser() and AddUserToTokenDacl() seems fine; let's
standardize on that. Also, every GetTokenUser() failure has been yielding two
messages, the second contributing no further detail. I'll reduce that to the
usual one message per failure.
This approach works for me.
OK by me, too.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers