Name type used for Authentication TTL #1990
Conversation
| @@ -25,6 +25,8 @@ typedef std::vector<Auth::SchemeConfig *> ConfigVector; | |||
|
|
|||
| class UserRequest; | |||
|
|
|||
| using Ttl = int32_t; | |||
There was a problem hiding this comment.
The new name is missing a description. The description must document what time unit this integer is using (e.g., seconds or time_t unit) and that negative values reflect "expired" entries.
| Auth::Basic::User::ttl() const | ||
| { | ||
| if (credentials() != Auth::Ok && credentials() != Auth::Pending) | ||
| return -1; // TTL is obsolete NOW. | ||
| return Auth::User::ttl(); // TTL is obsolete NOW. |
There was a problem hiding this comment.
Here and and in similar contexts, please either continue to return -1 or introduce a named Auth::Ttl constant and return that. Either would be more readable and safe than the proposed "use TTL computed by Auth::User::ttl() while assuming that that value is a negative TTL" change.
| @@ -25,6 +25,8 @@ typedef std::vector<Auth::SchemeConfig *> ConfigVector; | |||
|
|
|||
| class UserRequest; | |||
|
|
|||
| using Ttl = int32_t; | |||
There was a problem hiding this comment.
I am worried that the suggested name -- "Auth::Ttl" -- is too general because authentication code may need several different types for TTL. For example, authenticate_ttl uses a different type today. However, I assume you consider such worries unfounded and am not requesting any related changes.
| @@ -25,6 +25,8 @@ typedef std::vector<Auth::SchemeConfig *> ConfigVector; | |||
|
|
|||
| class UserRequest; | |||
|
|
|||
| using Ttl = int32_t; | |||
There was a problem hiding this comment.
The underlying type -- int32_t -- is wrong on several (probably obvious?) levels. However, I assume that this PR scope is limited to naming existing int32_t uses and excludes fixing the underlying type. Thus, I am not requesting those fixes in this PR.
| * Credentials are not permitted to be re-used from a | ||
| * credentials cache unless the authentication scheme | ||
| * defines a way to determine a TTL with which to bound | ||
| * the scope of their valid re-use. |
There was a problem hiding this comment.
Expired credentials reuse is also not permitted, right? Please add that critical information to the method description. Without that restriction, the method API does not really work because the method caller cannot distinguish "scheme did not define ..." cases from "credentials expired" cases.
| @@ -66,10 +66,15 @@ class User : public RefCountable | |||
| const SBuf userKey() const {return userKey_;} | |||
|
|
|||
| /** | |||
| * How long these credentials are still valid for. | |||
| * Negative numbers means already expired. | |||
| * Credentials are not permitted to be re-used from a | |||
There was a problem hiding this comment.
Can we say that credentials are not permitted to be cached "unless the scheme defines..."? In other words, caching code should not cache credentials for schemes that do not override this method. That would be a stronger rule/claim than the proposed "not permitted to be re-used".
| @@ -256,7 +256,7 @@ Auth::User::CredentialsCacheStats(StoreEntry *output) | |||
| Auth::Type_str[auth_user->auth_type], | |||
| CredentialState_str[auth_user->credentials()], | |||
| auth_user->ttl(), | |||
There was a problem hiding this comment.
This storeAppendPrintf() function call secretly assumes that ttl() returns something that can be "printed "using %d. I recommend refactoring CredentialsCacheStats() internals to use std::ostream instead, preferably in a separate PR. If that is not done, let's at least add an XXX:
| auth_user->ttl(), | |
| auth_user->ttl(), // XXX: Here and below, we assume that Ttl can be printed using %d |
| @@ -256,7 +256,7 @@ Auth::User::CredentialsCacheStats(StoreEntry *output) | |||
| Auth::Type_str[auth_user->auth_type], | |||
| CredentialState_str[auth_user->credentials()], | |||
| auth_user->ttl(), | |||
| static_cast<int32_t>(auth_user->expiretime - squid_curtime + Auth::TheConfig.credentialsTtl), | |||
| static_cast<Auth::Ttl>(auth_user->expiretime - squid_curtime + Auth::TheConfig.credentialsTtl), | |||
There was a problem hiding this comment.
Where possible, please avoid Foo:: inside Foo:
| static_cast<Auth::Ttl>(auth_user->expiretime - squid_curtime + Auth::TheConfig.credentialsTtl), | |
| static_cast<Ttl>(auth_user->expiretime - squid_curtime + Auth::TheConfig.credentialsTtl), |
or even
| static_cast<Auth::Ttl>(auth_user->expiretime - squid_curtime + Auth::TheConfig.credentialsTtl), | |
| static_cast<Ttl>(auth_user->expiretime - squid_curtime + TheConfig.credentialsTtl), |
rousskov
changed the title
- Add named type for Authentication TTL
+ Name type used for Authentication TTLI adjusted the title to clarify that we are naming something rather than adding something, but both variants have problems (e.g., the existing type does have a name already), and I do not insist on this title change. Please feel free to revert. |
rousskov
added
the
S-waiting-for-author
No description provided.