Add site with specified system_user via remoting api

Discussion in 'Developers' Forum' started by ispcomm, Sep 4, 2017.

  1. ispcomm

    ispcomm Member

    In some instances, I need to be able to select the system used from the remoting api, and force it to be something else.
    Currently, the "system user" (webNNN) for a site added trough remoting api is generated automatically and if "system_user" is passed in the parameters, it's overwritten (and ignored).
    I would like to propose a change where the system user in the parameters is accepted and used instead of ignored.
    I could provide patches to ispconfig for such usage scenario.
    To prevent problems, the remoting api could generate an error in case the system user is already in use on the system (perhaps with a possibility to force it anyway).
    Would there be any strong reasons against such behavior?
    ispcomm
     
  2. till

    till Super Moderator Staff Member ISPConfig Developer

    That would be nice.

    This will not work on multiserver, at least when you want to compare it to existing Linux users and not just to existing users in the ispconfig db) as the remote api endpoint is on the master and it can not check usernames against the passwd file on slaves.
     
  3. ispcomm

    ispcomm Member

    Yes, agreed.
    It is only an issue on the same server if two different sites share the same userid. Since we're acting on the master, the master could check if there's another site already using the same user id (webNNN).
    Web users uniqueness could be enforced or not, to be decided. I'd prefer to not (hard) enforce as there are usage cases where this might come up handy. Perhaps one more parameter like _ispconfig_allow_user_duplicates can control this.
    ispcomm
     
  4. ispcomm

    ispcomm Member

    So, today I eventually had some time to do some work on this feature.

    As far as I understand, the remoting api does not care about the system_user/system_group, document_root and the other parameters. They make it safely to the database.

    However, after the insertion of the site, an event is generated (sites:web_vhost_domain:eek:n_after_insert) which loads sites_web_vhost_domain plugin and calls the web_vhost_domain_edit function.

    This function will take the id of the newly generated site and overwrite the system_user, group, document_root, open_base_dir etc parameters of the site. Then it will write them back, ready for propagation to the slaves.

    Correct me if I'm wrong, but I think all is tied and originates from the website id (and client id).

    The cleanest solution I can come up with is to force the web site id in the remoting api. This will permit all the rest of ispconfig to work properly. It will also permit a quick check whether that ID is already associated with a web site, in which case I'd just return an error.

    This solution will also permit the keep the 1:1 mapping of the web user and the domain ID, just in case this is used (implicitly) somewhere.

    I'd like to have your opinion before going in changing code. I think it will be a simple form change and some additional checks for domain_id availability.

    ispcomm.
     
  5. ispcomm

    ispcomm Member

    @till, I created a merge request for a proof of concept on this here: https://git.ispconfig.org/ispconfig/ispconfig3/merge_requests/638
    This is the simplest way I found (and the only one actually) to do this as the domain_id needs to be updated before the events are fired.
    My preliminary tests are working and the db is populated properly. The site is created with the proper system user (and if the client is the same as before, on exactly the same path).
    As you guess, this is most useful for deleting/restoring backups from sites without touching their files.
    I created the PR for comments. If this way is accepted as a viable solution, I will add proper checks and error handling.
    ispcomm
     
  6. till

    till Super Moderator Staff Member ISPConfig Developer

    The more I think of this issue, the more I come to the conclusion that we should use a general approach for this and allow primary ID overrides in all remote functions.

    Here a short (not yet tested) way to add this globally.

    The goal is to have a key $params['_primary_id'] available and when this key is set then the api will force it for the primary column of the table during insert.

    in interface/lib/classes/tform_base.inc.php (master branch) we add:

    line 107:

    Code:
    var $primary_id_override = false;
    line 1273:

    Code:
    if($this->primary_id_override && isset($record['_primary_id'])) {
                $sql_insert_key .= '`'.$this->formDef["db_table_idx"].'`, ';
                $sql_insert_val .= intval($record['_primary_id']).", ";
            }
    
    and in interface/lib/classes/remoting_lib.inc.php we add on line 202:

    $this->primary_id_override = true;

    The primary_id_override switch exists only to ensure that this function can not be used from forms in the interface (at least not without activating it explicitly in the code).

    So when you add a website with the api now and the domain_id shall be 10, then you just add:

    $params['_primary_id'] = 10;

    What do you think about this solution?

    A check should be added in tform_base.inc.php if a record with this ID exists though. Either we skip to set the primary ID then or we throw an error.
     
  7. ispcomm

    ispcomm Member

    Yes, I like your approach better. It's good it can be extended to other objects too.
    I failed to understand exactly how to force the ID in the insert query. I tried adding the _force variable in $params but it never made it to the remoting sites plugin.
    Today I will try your suggested changes and see how they work.
     
  8. till

    till Super Moderator Staff Member ISPConfig Developer

    This probably happens because ispconfig cleans all variables that do not exist in the form file before it passes the array to the plugins. If this hapens in my proposed change as well, then please let me know and I'll find a way to prevent that.
     
  9. ispcomm

    ispcomm Member

    I can confirm that _primary_id is wiped out somewhere. in getSQL of remoting_lib.inc.php (line 203) the $record contains only keys present in the "form".
    At some point yesterday I added my '_primary_id' to the $form and it showed up in $record. However I was not sure how this would interact with the gui and what validators were needed.
    I'll wait for your workaround.
     
  10. till

    till Super Moderator Staff Member ISPConfig Developer

    Please try this in tform_base.inc.php. Move line 1266:

    $record = $this->encode($record, $tab, true);

    so that it is right after the block that we added at line 1273. The encode function is the one that cleans the data record.
     
  11. ispcomm

    ispcomm Member

    So, _primary_id works and the site is inserted with the correct domain_id. However, when doing the query by inserting a domain_id by forcing it instead of using auto_incremented values, mysql will return '0' for the last inserted id.
    I'll change the $force_primary_id to be a numeric, with '0' meaning not force and >0 meaning the id we want.
    The code is diverging a little, I'll paste complete chunks later for your review.
    Sorry for bad info. The _primary_id survives. I'll update shortly with results.
    Nope.
    remoting_lib.inc.php breaks first in getSQL line 203. At this point $record contains only variables (keys) that are defined in the form definition. _getSQL is called after and reaches the tform_base on line 1266 but the $record was cleaned already.
    I think the form handling routines construct a new $record from variables present in the form.
     
    Last edited: Sep 9, 2017
  12. ispcomm

    ispcomm Member

    Ok, here's my proposed chage. Seems to be working. Inserted a site with correct ID and did no harm when adding other objects (db, db user).
    tfom_base_inc.php becomes, around line 107:
    Code:
    var $primary_id_override = 0;
    Then, same file, _getSQL I force '_primary_id' past the check of encode. Line 1259 and below look like this:
    Code:
    protected function _getSQL($record, $tab, $action = 'INSERT', $primary_id = 0, $sql_ext_where = '', $api = false) {
    
    global $app;
    
    $this->action = $action;
    $this->primary_id = $primary_id;
    
    $sql_insert_key = '';
    $sql_insert_val = '';
    $sql_update = '';
    
    $record = $this->encode($record, $tab, true);
    if(($this->primary_id_override > 0)) {
        $sql_insert_key .= '`'.$this->formDef["db_table_idx"].'`, ';
        $sql_insert_val .= $this->primary_id_override.", ";
        $record['_primary_id'] = $this->primary_id_override;
    }
    
    if($api == true) $fields = &$this->formDef['fields'];
    else $fields = &$this->formDef['tabs'][$tab]['fields'];
    
    // go trough all fields of the tab
    if(is_array($record)) {
    
    
    This _primary_id is feeded trough remoting, and bypasses the checks in encode, so I make sure to sanitize it first. remoting_lib.inc.php, line 203, add:
    Code:
    // early usage. make sure _primary_id is sanitized if present.
    if ( isset($record['_primary_id']) && is_numeric($record['_primary_id'])) {
        $_primary_id = intval($record['_primary_id']);
        if ($_primary_id > 0)
            $this->primary_id_override = intval($record['_primary_id']);
    }
    
    And remoting.inc.php, line 335, modify to read:
    Code:
    if ( isset($params['_primary_id'] ))
        $insert_id = $params['_primary_id'];
    else
        $insert_id = $app->db->insertID();
    
    In case the primary_id is already present in the database, mysql will spit out an error, which will get returned by the remoting api as an exception (my guess. have not tested).

    My commit is here: https://git.ispconfig.org/ispcomm/ispconfig3/commit/6dfc6d26e07010b575627b073c836b6d4ebce7b9

    Let me know what you think.
    ispcomm
     
    Last edited: Sep 9, 2017
  13. till

    till Super Moderator Staff Member ISPConfig Developer

    The implementation looks nice. Thank you for your effort to develop and test it!
    Please make a merge request.

    Till
     
    ispcomm likes this.
  14. ispcomm

    ispcomm Member

    thanks for providing some insight and pointers in the code. I'll test this now with a few domains on a live server and will issue a PR if not problems are found.
     
    till likes this.
  15. ispcomm

    ispcomm Member

Share This Page