TIP: Use Markdown or, <pre> for multi line code blocks / <code> for inline code.
Why do you extend ORM?
  • samwilsonsamwilson
    I was just wondering if anyone routinely extends the ORM class, and if so, what for?

    For example, on one project I'm working on, we have a ORM::save_from_post() method, which looks (something) like this:


    public function save_from_post()
    {
    if (count($_POST) > 0)
    {
    // Never save the PK; but load the record.
    if (isset($_POST[$this->_primary_key]))
    {
    $this->find($_POST[$this->_primary_key]);
    unset($_POST[$this->_primary_key]);
    }

    // Set each supplied attribute that exists in the model.
    foreach ($this->_object as $key => $old_val)
    {
    // Only save if the key has been POST'd and the value has changed.
    if (isset($_POST[$key]) && $_POST[$key] != $old_val)
    {
    $new_val = $_POST[$key];
    $new_val = (is_string($new_val) && empty($new_val)) ? NULL : $new_val;
    $this->$key = $new_val;
    }
    }
    if ($this->check())
    {
    $this->save();
    // Save files
    foreach ($_FILES as $file)
    {
    if (empty($file['name'])) continue;
    $dest_dir = DATAPATH.$this->db_name()
    .DIRECTORY_SEPARATOR.$this->_table_name
    .DIRECTORY_SEPARATOR.$this->id;
    File::make_writable_dir($dest_dir);
    File::copy_with_rename($file['tmp_name'], $dest_dir.DIRECTORY_SEPARATOR.$file['name']);
    }
    return TRUE;
    }
    }
    return FALSE;
    }


    Basically (and there is a bunch of validation and logging stuff missing from the above), it goes through $_POST and saves values who's keys exist in the model, and then saves all POST'd files as well (to DATAPATH/db_name/table_name/ID/).

    We also have a few logging methods, that make creating an audit trail pretty easy (any model, when saved, is forced to log all modifications; there's then a standard view that's rendered wherever necessary to show the history of some entity).

    Anyone got anything similar? Or care to rip the above apart as daft and improper?! ;-)
  • samwilsonsamwilson
    (Ugh, sorry about the formatting of the above. The code is indented, I promise; I guess the new version of Vanilla doesn't like markdown yet.)

    [Edit] Okay, figured it out. No markdown, just HTML.
  • OsmodiarOsmodiar
    We usually extend ORM just to set the following variables

    protected $_db = "my_db_alias";
    protected $_table_names_plural = FALSE;

    To avoid setting them on every single model. Also sometimes we add new methods for data type conversion used on all models.
  • Zeelot3kZeelot3k
    off-topic but your method is scary and insecure. First of all, the only part of your application that should be accessing $_POST, $_FILES, and $_GET, is the controller. The scary part though, was the `if (isset($_POST[$this->_primary_key]))` line. Does this mean I can edit $_POST before submitting it and insert an 'id' field with a value of my own? By doing this on any form, it seems I successfully changed any creation form, into an editing form. You might have already caught this and checked for it in the controller, but doing it that way leaves you to very easily make a mistake and forget to for a very tiny form somewhere. You shouldn't have such a dangerous security hole for the sake of writing less code per form.

    Anyway, I'm not sure if you are using that exact method up there or if you have extra code to prevent things like that, but you should start off by removing any calls to the superglobals because the model should not know about them.

    Hope that was helpful.
  • WebthinkWebthink
    The ID thing zeelot3k points out is by far the biggest potential problem there but the security problems don't end there. Depending on how you're controller uses this method it's possible for someone to edit the form to include just about any imaginable field in the table provided they can guess the field names. A profile edit page for instance may allow me to alter any data saved in the user record rather than just the limited few your app presents to me.

    As an aside misuse of the values() method can allow the same hole. I've seen this in more than a few examples posted here.
  • IsaiahIsaiah
    @Webthink, yeah I've seen a lot of issues with that as well. It's one of the issues that @Zeelot3k and I have tried to address in the updated ORM (not finished yet).
  • thom2233thom2233
    It would be nice to pass some kind of whitelist to the values function to prevent this.
  • Zeelot3kZeelot3k
    haha: http://github.com/Zeelot/kohana-orm/commit/31277e6977a43feb79cd011dd6c6e7056eed594a

    3.1 should be much nicer. The $expected array is optional (although very preferred for security reasons and clarity) but will assume you are setting all values EXCEPT the primary key (ID usually) when you don't pass it in.
  • thom2233thom2233
    :D
Add a Comment