Issue 5834 - AccountPolicyPlugin erroring for some users #5866
Conversation
Bug Description: With the account policy plugin enabled and lastloginhistory size set to non 0 an issue occurs during simultaneous binds of the same user. In this case the timestamp to be stored in the lastloginHistory attribute already exists from a previous bind, and generates an error message. A side effect of lastloginHistory feature is that the modifytimestamp value is updated after a successful bind, even when the feature is disabled. Fix Description: Before a timestamp is added to the lastloginHistory attribute a check is performed to make sure it doesnt already exist. Ensure the entry is not modified when this feature is disabled. Fixes: 389ds#5834 Relates:389ds#5752 Reviewed by:
| } | ||
| if (cfg->login_history_size != 0) { | ||
| /* realloc and append latest value */ | ||
| login_hist = (char **)slapi_ch_realloc((char *)login_hist, sizeof(char *) * (cfg->login_history_size + 2)); |
There was a problem hiding this comment.
realloc size is wrong ( should be login_history_size+1) and BTW I do not think that you
should even realloc in this case (the buffer is large enough to store the values and will be freed soon (within the pblock_destroy))
There was a problem hiding this comment.
realloc size is wrong ( should be login_history_size+1) and BTW I do not think that you should even realloc in this case (the buffer is large enough to store the values and will be freed soon (within the pblock_destroy))
Yes you are correct, in this case there is no need for a realloc.
| config_rd_lock(); | ||
| cfg = get_config(); | ||
|
|
||
| /* does this value already exist in the entry */ |
There was a problem hiding this comment.
You may check that 'e' is not NULL.
| slapi_pblock_get(mod_pb, SLAPI_PLUGIN_INTOP_RESULT, &rc); | ||
| if (rc != LDAP_SUCCESS) { | ||
| slapi_log_err(SLAPI_LOG_ERR, "acct_update_login_history", "Modify error %d on entry '%s'\n", rc, dn); | ||
| if (mod_required) { |
There was a problem hiding this comment.
I have a doubt, if login_history_size==0 what will be the result ?
An attribute 'lastloginhistory' present with no value, or no 'lastloginhistory' attribute ?
I think it would be better to have no 'lastloginhistory' attribute.
There was a problem hiding this comment.
No problems here:
in a modify replace a NULL or an empty modifier list means that the attribute is removed.
BTW in ldap there is no such thing as an attribute without value.
( Attribute always have a value (that value may be empty but in this case you have to specify explicitly that a value is present and the value size is 0)
There was a problem hiding this comment.
I have a doubt, if login_history_size==0 what will be the result ?
If login_history_size == 0 then the feature is disabled and we do nothing unless the entries array has been trimmed.
Bug Description: With the account policy plugin enabled and lastloginhistory size set to non 0 an issue occurs during simultaneous binds of the same user. In this case the timestamp to be stored in the lastloginHistory attribute already exists from a previous bind, and generates an error message. A side effect of lastloginHistory feature is that the modifytimestamp value is updated after a successful bind, even when the feature is disabled. Fix Description: Before a timestamp is added to the lastloginHistory attribute a check is performed to make sure it doesnt already exist. Ensure the entry is not modified when this feature is disabled. Fixes: #5834 Relates:#5752 Reviewed by: @progier389, @tbordaz (Thank you)
Bug Description: With the account policy plugin enabled and lastloginhistory size set to non 0 an issue occurs during simultaneous binds of the same user. In this case the timestamp to be stored in the lastloginHistory attribute already exists from a previous bind, and generates an error message. A side effect of lastloginHistory feature is that the modifytimestamp value is updated after a successful bind, even when the feature is disabled. Fix Description: Before a timestamp is added to the lastloginHistory attribute a check is performed to make sure it doesnt already exist. Ensure the entry is not modified when this feature is disabled. Fixes: #5834 Relates:#5752 Reviewed by: @progier389, @tbordaz (Thank you)
Bug Description: With the account policy plugin enabled and lastloginhistory size set to non 0 an issue occurs during simultaneous binds of the same user. In this case the timestamp to be stored in the lastloginHistory attribute already exists from a previous bind, and generates an error message.
Fix Description: Before a timestamp is added to the lastloginHistory attribute a check is performed to make sure it doesnt already exist.
Bug Description: A side effect of lastloginHistory feature is that the modifytimestamp value is updated after a successful bind, even when the feature is disabled.
Fix Description: Ensure the entry is not modified when this feature is disabled.
Fixes: #5834
Relates:#5752
Reviewed by: