-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
These patches must be applied on top of the "Do not check entries..." one.
[PATCH 1/2] Store lastLogin attribute when authenticating online This is needed for the second patch as we rely on lastLogin to decide whether to delete an entry or not.
[PATCH 2/2] Better cleanup task handling Implements a different mechanism for cleanup task. Instead of just deleting expired entries, this patch adds a new option login_cache_timeout for domains. If an entry is expired and the last login was more days in the past that login_cache_timeout, the entry is deleted.
Groups are deleted if they are expired and and no user references them (no user has memberof: attribute pointing at that group).
The parameter login_cache_timeout is not LDAP-specific, so that other future backends might use the same timeout setting.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/23/2010 07:50 AM, Jakub Hrozek wrote:
These patches must be applied on top of the "Do not check entries..." one.
[PATCH 1/2] Store lastLogin attribute when authenticating online This is needed for the second patch as we rely on lastLogin to decide whether to delete an entry or not.
[PATCH 2/2] Better cleanup task handling Implements a different mechanism for cleanup task. Instead of just deleting expired entries, this patch adds a new option login_cache_timeout for domains. If an entry is expired and the last login was more days in the past that login_cache_timeout, the entry is deleted.
Groups are deleted if they are expired and and no user references them (no user has memberof: attribute pointing at that group).
The parameter login_cache_timeout is not LDAP-specific, so that other future backends might use the same timeout setting.
Patch 0001: Ack.
Patch 0002: Nack.
+ 'login_cache_timeout' : _('How long to keep cached entries with after last successfull login'), Should read _('How long to keep cached entries after last successful login (days)')
Similarly, you misspelled "successful" in the manpage.
Also, you bumped the value of IPA_OPTS_BASIC_TEST without actually adding the option to ipa_common.c. That defeats the purpose of this test. It's meant to remind you that ipa_common.c needs to be updated when ldap_common.c is.
The approach here looks good, but I've also asked Simo to take a look as well, since he originally proposed the design.
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
On Tue, 23 Feb 2010 13:50:42 +0100 Jakub Hrozek jhrozek@redhat.com wrote:
[PATCH 1/2] Store lastLogin attribute when authenticating online This is needed for the second patch as we rely on lastLogin to decide whether to delete an entry or not.
This one seem to save the last login _only_ when the authentication was made online.
Looks like the wrong way to record the last login. IT seem that the function is misnamed and should be renames to set_last_online_auth(), and a new funciotn names set_last_login() should be created and called when any auth (online or offline) succeeds.
Simo.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/23/2010 11:58 AM, Simo Sorce wrote:
On Tue, 23 Feb 2010 13:50:42 +0100 Jakub Hrozek jhrozek@redhat.com wrote:
[PATCH 1/2] Store lastLogin attribute when authenticating online This is needed for the second patch as we rely on lastLogin to decide whether to delete an entry or not.
This one seem to save the last login _only_ when the authentication was made online.
Looks like the wrong way to record the last login. IT seem that the function is misnamed and should be renames to set_last_online_auth(), and a new funciotn names set_last_login() should be created and called when any auth (online or offline) succeeds.
We were already storing the lastLogin for offline auth. It was overlooked for online auth.
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
On Tue, 23 Feb 2010 11:59:27 -0500 Stephen Gallagher sgallagh@redhat.com wrote:
[PATCH 1/2] Store lastLogin attribute when authenticating online This is needed for the second patch as we rely on lastLogin to decide whether to delete an entry or not.
This one seem to save the last login _only_ when the authentication was made online.
Looks like the wrong way to record the last login. IT seem that the function is misnamed and should be renames to set_last_online_auth(), and a new funciotn names set_last_login() should be created and called when any auth (online or offline) succeeds.
We were already storing the lastLogin for offline auth. It was overlooked for online auth.
Uhmm I see, then the patch is fine. Although we may want to open an RFE to make a better interface to set this information, or at the very least add comments in the code that makes clear why we are doing this here this way.
Simo.
On Tue, 23 Feb 2010 13:50:42 +0100 Jakub Hrozek jhrozek@redhat.com wrote:
= dp_opt_get_int(opts->basic,SDAP_LOGIN_CACHE_TIMEOUT);- if (!offline_credentials_expiration && ldap_cred_expiration) {
DEBUG(1, ("Conflicting values for options %s (unlimited) ""and %s (%d)\n",opts->basic[SDAP_LOGIN_CACHE_TIMEOUT].opt_name,CONFDB_PAM_CRED_TIMEOUT,offline_credentials_expiration));ret = EINVAL;goto done;- }
- if (offline_credentials_expiration && ldap_cred_expiration &&
offline_credentials_expiration >= ldap_cred_expiration) {DEBUG(1, ("Value of %s (now %d) must be larger ""than value of %s (now %d)\n",opts->basic[SDAP_LOGIN_CACHE_TIMEOUT].opt_name,ldap_cred_expiration,CONFDB_PAM_CRED_TIMEOUT,offline_credentials_expiration));ret = EINVAL;goto done;- }
This snipped is very confusing. It looks like ldap_cred_expiration should really be called login_cache_timeout, or what I am not understanding here?
Can you also add a comment that explains the checks ? Given the special case (0 == unlimited) I think a comment would help a lot the casual reader understanding what is going on here.
Simo.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/23/2010 06:03 PM, Simo Sorce wrote:
This snipped is very confusing. It looks like ldap_cred_expiration should really be called login_cache_timeout, or what I am not understanding here?
Yes, this is confusing. As you mentioned in the other mail, the name of the new option was chosen badly, I'll change the variable along with the option name.
Can you also add a comment that explains the checks ? Given the special case (0 == unlimited) I think a comment would help a lot the casual reader understanding what is going on here.
Yes, will do.
On Tue, 23 Feb 2010 13:50:42 +0100 Jakub Hrozek jhrozek@redhat.com wrote:
<varlistentry><term>login_cache_timeout (integer)</term><listitem><para>
Just thinking out loud, but the name looks not very clear. I would rather call it something like account_cache_expiration, although login_cache_expiration might be also good enough.
"Timeout" looks like the wrong term here, as this is not a timeout but an expiration term.
Simo.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/23/2010 06:05 PM, Simo Sorce wrote:
I would rather call it something like account_cache_expiration,
Right, thanks for the suggestion. I like account_cache_expiration
On Tue, 23 Feb 2010 13:50:42 +0100 Jakub Hrozek jhrozek@redhat.com wrote:
if (!req) {@@ -281,19 +286,41 @@ static struct tevent_req *cleanup_users_send(TALLOC_CTX *memctx, }
state->ev = ev;
- state->sysdb = sysdb;
- state->domain = domain;
- state->sysdb = ctx->be->sysdb;
- state->domain = ctx->be->domain;
- state->ctx = ctx; state->msgs = NULL; state->count = 0; state->cur = 0;
- subfilter = talloc_asprintf(state, "(&(!(%s=0))(%s<=%ld))",
- login_cred_expiration = dp_opt_get_int(state->ctx->opts->basic,
SDAP_LOGIN_CACHE_TIMEOUT);- DEBUG(9, ("Cache expiration is set to %d days\n",
login_cred_expiration)); +
- subfilter = talloc_asprintf(state,
"(&(!(%s=0))(%s<=%ld)", SYSDB_CACHE_EXPIRE,
SYSDB_CACHE_EXPIRE, (long)now);
SYSDB_CACHE_EXPIRE,(long) now);
talloc_zfree(req);return NULL;
tevent_req_error(req, ENOMEM);- }
this change looks wrong ^^^^
- if (login_cred_expiration > 0) {
subfilter = talloc_asprintf_append(subfilter,"(|(!(%s=*))(%s<=%ld)))",SYSDB_LAST_LOGIN,SYSDB_LAST_LOGIN,(long) (now - (login_cred_expiration
- 86400)));
- } else {
subfilter = talloc_asprintf_append(subfilter,"(!(%s=*)))",SYSDB_LAST_LOGIN);- }
- if (!subfilter) {
DEBUG(2, ("Failed to build filter\n"));tevent_req_error(req, ENOMEM);
this looks wrong ^^^^
subreq = sysdb_search_users_send(state, state->ev,@@ -301,11 +328,9 @@ static struct tevent_req *cleanup_users_send(TALLOC_CTX *memctx, state->domain, subfilter, attrs); if (!subreq) { DEBUG(2, ("Failed to send entry search\n"));
talloc_zfree(req);return NULL;
tevent_req_error(req, ENOMEM);
this looks wrong ^^^^
tevent_req_set_callback(subreq, cleanup_users_process, req);
- return req;
}
Why did you change these error conditions ? You do not return on error and you are setting an error without a tevent_req_post() in the call that creates the request.
Simo.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/23/2010 06:19 PM, Simo Sorce wrote:
Why did you change these error conditions ?
One of the versions of the patch had these conditions in a callback (there was an interim step), so I used tevent_req_error
I could swear I changed the error cases into tevent_req_post..this is a clear bug, thanks for spotting it.
You do not return on error and you are setting an error without a tevent_req_post() in the call that creates the request.
Simo.
On Tue, 23 Feb 2010 13:50:42 +0100 Jakub Hrozek jhrozek@redhat.com wrote:
- ret = get_uid_table(state, &state->uid_table);
- if (ret != EOK) {
tevent_req_error(req, ret);return;- }
On non-linux platforms this returns ENOSYS You can't make it a hard error IMO.
Simo.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/23/2010 06:22 PM, Simo Sorce wrote:
On non-linux platforms this returns ENOSYS You can't make it a hard error IMO.
Correct..I presume that in this case we don't delete any users? As deleting a user who is logged in is..bad.
On Tue, 23 Feb 2010 18:29:20 +0100 Jakub Hrozek jhrozek@redhat.com wrote:
On 02/23/2010 06:22 PM, Simo Sorce wrote:
On non-linux platforms this returns ENOSYS You can't make it a hard error IMO.
Correct..I presume that in this case we don't delete any users? As deleting a user who is logged in is..bad.
Not sure how bad it is, it is a quite rare condition. A user that is logged in for days and days past all expiration timeouts ... even if we delete it, as soon as the system needs to retrieve info about it it will be fetched back. Of course pure users or groups enumerations would be sort of inconsistent ...
I say we delete it w/o doing the check for now.
Simo.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Attached is a revised patch.
The changes: * reverted the error condition handling in cleanup_users_send() * handles ENOSYS returned from get_uid_table() * commented the checks on offline_credentials_expiration * the new option was renamed to account_cache_expiration * "successful" was misspelled in the manpage and ConfigAPI description * ipa options did not include the new option
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/23/2010 01:43 PM, Jakub Hrozek wrote:
Attached is a revised patch.
The changes:
- reverted the error condition handling in cleanup_users_send()
- handles ENOSYS returned from get_uid_table()
- commented the checks on offline_credentials_expiration
- the new option was renamed to account_cache_expiration
- "successful" was misspelled in the manpage and ConfigAPI description
- ipa options did not include the new option
"successful" wasn't the only part of the ConfigAPI description that needed changing. It's still bad English.
_('How long to keep cached entries with after last successful login'), Should read _('How long to keep cached entries after last successful login (days)')
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/23/2010 07:46 PM, Stephen Gallagher wrote:
"successful" wasn't the only part of the ConfigAPI description that needed changing. It's still bad English.
_('How long to keep cached entries with after last successful login'), Should read _('How long to keep cached entries after last successful login (days)')
Fixed & attached
On Tue, 23 Feb 2010 19:52:50 +0100 Jakub Hrozek jhrozek@redhat.com wrote:
- subfilter = talloc_asprintf(state, "(&(!(%s=0))(%s<=%ld))",
- account_cache_expiration =
dp_opt_get_int(state->ctx->opts->basic,
SDAP_ACCOUNT_CACHE_EXPIRATION);
- DEBUG(9, ("Cache expiration is set to %d days\n",
account_cache_expiration));- subfilter = talloc_asprintf(state,
"(&(!(%s=0))(%s<=%ld)", SYSDB_CACHE_EXPIRE,
SYSDB_CACHE_EXPIRE, (long)now);
SYSDB_CACHE_EXPIRE,(long) now);- if (!subfilter) {
DEBUG(2, ("Failed to build filter\n"));talloc_zfree(req);return NULL;- }
- if (account_cache_expiration > 0) {
subfilter = )subfilter,"(|(!(%s=*))(%s<=%ld)))",SYSDB_LAST_LOGIN,SYSDB_LAST_LOGIN,(long) (now -(account_cache_expiration * 86400)));
- } else {
subfilter = talloc_asprintf_append(subfilter,"(!(%s=*)))",SYSDB_LAST_LOGIN);- } if (!subfilter) { DEBUG(2, ("Failed to build filter\n")); talloc_zfree(req);
Given you are always calling talloc_asprintf_append() in either case, it would seem more efficient to simply have 2 completely separate talloc_asprintf() calls, even if that means repeating the same subset of arguments in the two branches of the if statement.
It took me a while to realize what was going on here (I didn't immediately realize these where _append()s )
Although this is not something I would consider nacking the patch for.
Simo.
On Tue, 23 Feb 2010 19:52:50 +0100 Jakub Hrozek jhrozek@redhat.com wrote:
On 02/23/2010 07:46 PM, Stephen Gallagher wrote:
"successful" wasn't the only part of the ConfigAPI description that needed changing. It's still bad English.
_('How long to keep cached entries with after last successful login'), Should read _('How long to keep cached entries after last successful login (days)')
Fixed & attached
Aside from the talloc_asprintf_append() point in the other mail, patches looks good to me.
Simo.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/23/2010 08:30 PM, Simo Sorce wrote:
Aside from the talloc_asprintf_append() point in the other mail, patches looks good to me.
Simo.
Thanks for the review, new patches are attached.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/23/2010 04:08 PM, Jakub Hrozek wrote:
On 02/23/2010 08:30 PM, Simo Sorce wrote:
Aside from the talloc_asprintf_append() point in the other mail, patches looks good to me.
Simo.
Thanks for the review, new patches are attached.
Looks good to me. Ack!
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/23/2010 04:11 PM, Stephen Gallagher wrote:
On 02/23/2010 04:08 PM, Jakub Hrozek wrote:
On 02/23/2010 08:30 PM, Simo Sorce wrote:
Aside from the talloc_asprintf_append() point in the other mail, patches looks good to me.
Simo.
Thanks for the review, new patches are attached.
Looks good to me. Ack!
Pushed to master.
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Simo Sorce -
Stephen Gallagher