#49309 Referential Integrity does not perform proper syntax checking on referint-update-delay
Closed: wontfix 4 years ago Opened 4 years ago by ilias95.

Issue Description

According to the documentation when referint-update-delay is set to -1, it means that "No check for referential integrity is performed". However, the plug-in will not accept such a value. Additionally, if we set a non-numerical value such as the string "example" it will happily accept it without complaining.

Package Version and Platform

Fedora 25, git master

Metadata Update from @ilias95:
- Issue assigned to ilias95

4 years ago

Metadata Update from @ilias95:
- Custom field component adjusted to None
- Custom field origin adjusted to None
- Custom field reviewstatus adjusted to None
- Custom field type adjusted to None
- Custom field version adjusted to None

4 years ago

Metadata Update from @ilias95:
- Custom field reviewstatus adjusted to review (was: None)

4 years ago

We should not change the setting to "-2" especially since the docs say to use "-1". Is there a reason to change it?

This is not the case.

Here is how it works currently (and where the bug resides):

  • Initially it sets tmp_config->delay to -1. This is supposed to be an incorrect value (but it reallity it isn't).
  • Later it attmpets to store a new value to tmp_config->delay, so it expects it now to have a different value.
  • Finally, it checks whether the value of tmp_config->delay has been changed from -1 to something else. If it is still -1 it means that something went wrong.

But this process is actually wrong, because we want -1 to be a valid value as well.

So I had to change the "default error value" to some other invalid value. I picked -2, but I guess it doesn't matter that much. Could be -3, -4, etc.

Additionally, by using strtol instead of atoi I also check if the value that the user provided is actually a number, and if it's not I set the value to -2 as well. In the way it is implemented right now you can store "fdsafdsa" in referint-update-delay and it will accept it.

I guess I have to improve the commit message as well to reflect this.

Ah okay, thanks for the clarification. One additional request to the fix, can you print the "original" invalid value in the error log message? It might be easier to move the log message into the code block where "-2" is set.



One thing now is that when we provide an invalid value it will log both "invalid value" and "plugin configuration is missing" messages. However, I think it's no big issue. If we want to change this, it will make the code much more complex.

We should fix it up right. So we need to rework it a bit. When we get an invalid value, log an error, set rc = SLAPI_PLUGIN_FAILURE, and "goto done".

we should also validate the other attributes, if possible, like REFERINT_ATTR_LOGCHANGES, etc.

You are right, it was easier than I thought.


referint-logchanges does absolutely nothing: https://pagure.io/389-ds-base/issue/48185
Should we remove it instead?

Leave it for now, and we will clean it up in 48185

Metadata Update from @mreynolds:
- Custom field component adjusted to Plugins (was: None)
- Custom field origin adjusted to Community (was: None)
- Custom field reviewstatus adjusted to ack (was: review)
- Custom field type adjusted to defect (was: None)

4 years ago

commit 1f25673
To ssh://git@pagure.io/389-ds-base.git
33db32a..1f25673 master -> master

Metadata Update from @ilias95:
- Issue close_status updated to: fixed
- Issue status updated to: Closed (was: Open)

4 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/2368

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.

Metadata Update from @spichugi:
- Issue close_status updated to: wontfix (was: fixed)

2 years ago

Login to comment on this ticket.