Remote API fails on Insert with null sys_userid when Client ID is invalid
My original report with a different title was inaccurate. I've revised this (1-Nov) after reviewing the detail. See "[EDIT]".
Try to use mail_user_add and it fails with
Error : Incorrect integer value: '' for column `dbispconfig`.`mail_user`.`sys_userid` at row 1 INSERT INTO `mail_user`
Yes, the value in the constructed query is null. But that's not the value that was provided. The source params array includes these values:
[sys_userid] => 1
[sys_groupid] => 2
[EDIT]: Based on what I think I know now and the info below, I think the settings for sys_userid and sys_groupid are appropriately ignored and should not be included in a remote call. The relevant field is the client ID.
This issue is not specific to mail users but I believe it is unique to new records and Insert queries. In the _getSQL function of tform_base.inc.php it gets down to these lines at 1428:
if($action == "INSERT") {
if($this->formDef['auth'] == 'yes') {
// Set user and group
$sql_insert_key .= "`sys_userid`, ";
$sql_insert_val .= ($this->formDef["auth_preset"]["userid"] > 0)?
"'".$this->formDef["auth_preset"]["userid"]."', ":
"'".$_SESSION["s"]["user"]["userid"]."', ";
$sql_insert_key .= "`sys_groupid`, ";
$sql_insert_val .= ($this->formDef["auth_preset"]["groupid"] > 0)?
"'".$this->formDef["auth_preset"]["groupid"]."', ":
"'".$_SESSION["s"]["user"]["default_group"]."', ";
In this scenario there is no auth_preset userid, so the value comes from $_SESSION. But the user ID has not been loaded into $_SESSION ... that results in null values on the INSERT, which get rejected by the constraints on the mail_user table.
At that point, the value of $_SESSION is as follows:
Array
(
[client_login] => 0
[client_sys_userid] => 0
[s] => Array
(
[user] => Array
(
[typ] => admin
[username] =>
[userid] =>
[default_group] =>
[groups] =>
[client_id] => 2
)
)
)
[EDIT] Removed original notes. Note above that the client_id is 2. This was an invalid ID as in this environment there is only one client, myself, with client_id 1. The problem comes from remoting_lib.inc.php, where an invalid client ID results in an empty sys_userid. That's where that empty field comes from.
I believe the solution to this issue is that when there is an invalid client ID, "Invalid Client ID" is the message that should be communicated back up to the client caller, not the "downstream" issue that there is an invalid sys_user.sys_userid.
Note to myself for documentation: If the client ID is 0, then the default admin user ID "1" is assigned to "userid". This means only the ISPC administrator can see/update this email user. When the client ID is a non-zero valid ID, the "userid" is the CP ID that allows a specific CP user for the client to maintain that user. Email addresses for clients should always use the client ID, though there will not be any error or notification if 0 is used. Using zero could result in "where is my mail user?". This concept should be documented, where the client owns an email address that they cannot see/change.
Another related note: The UID+GID values should not be set in parameters to empty string. The code checks for the "set" state of these parameters, and will not replace an empty string. I believe the same is true for sys_userid and sys_groupid. In summary, I think some changes might be made to set all values to empty string if they are unset, and then process the values. Or the doc can be updated to describe exactly how this works.
The bottom line on this stuff : I came into this understanding that there is little to no data validation on API calls. Now I'm seeing exactly how that works. I'm proposing changes that help to keep client code from falling into problems, but I'd agree to the suggestion that all of this would be solved in doc updates and not in code. You guys call it.