Add implementation for restricted exporting and grouping

[dannyadair]

No description provided.

[hbrunn]

this is great, thanks! Still a few comments, and don't forget to add yourself as contributor if you like, your contribution surely merits this.

But before using this in production, we need to fix https://github.com/hbrunn/server-tools/pull/3/files#diff-9797ae3b8487be8a1f1cd8e6faea9ad8L197, otherwise, knowledgeable users can craft RPC requests setting the key in the context and circumvent all the restrictions. What I have in mind here is creating a class pretending to be an int as in https://github.com/OCA/server-tools/blob/8.0/base_suspend_security/base_suspend_security.py#L22. Then you can sudo to the current user id wrapped into this class, and check for the type of uid in _restrict_field_access_get_is_suspended

Cool, we're nearly there!

[hbrunn]

watch out for line length, the OCA conventions want 80 characters max

[hbrunn]

any reason not to use v8 api here?

[dannyadair]

I just took the original as the starting point. Afraid of others calling with positional arguments.

[hbrunn]

note that super uses all fields if this is falsy, you should do the same: https://github.com/OCA/OCB/blob/8.0/openerp/models.py#L2079

[hbrunn]

this call is tricky btw, because the idea with this mixin is that developers can restrict field access per record. As you call this on no record, this will break some of the logic. I see two ways to cope with that:

• call super first, loop through the results, use the __domain key per row to query the records involved, and null out the field for the row if one of the rows involved doesn't allow field access. This is the cleanest solution, but horribly slow
• invent another action read_group, then the user of the mixin needs to take care to take sensible action for this case. This should be reflected in the docstrings of all functions which get an action passed

[hbrunn]

last thing here: To be symmetric to read, the result should contain keys for those fields, but with the null value of the field type. This is important in order not to break views, because a lot of code in the web client expects a key for a field to exist if it was requested

[dannyadair]

You're right I'll adjust so fields default to all columns (before getting sanitised).

Determining dynamically is still possible, you just need to decide what it means when self is falsy. For our HR module we use this logic:

    @api.multi
    def _restrict_field_access_is_field_accessible(self, field_name,
                                                   action='read'):
        """Determine field access dynamically."""
        if field_name in FIELDS_ONLY_MANAGER:
            # "HR Settings" tab: HR Manager only
            return self.is_hr_manager()
        elif field_name in FIELDS_ONLY_SELF_OR_MANAGER:
            # "Personal Information" tab: HR Manager or Employee's user only
            if self.is_hr_manager():
                return True
            else:
                if not self:
                    # Not called on particular instance (e.g. export)
                    return self.is_hr_manager()
                else:
                    return all(this.user_id == self.env.user for this in self)
        else:
            # "Public Information" tab
            if action == 'read':
                # Read only: no field restriction
                return True
            else:   # 'create'/'write'
                # Create/Write: HR Manager or Employee's user only
                if self.is_hr_manager():
                    return True
                else:
                    if not self:
                        # Not called on particular instance
                        return self.is_hr_manager()
                    else:
                        return all(this.user_id == self.env.user for this in self)

I've got something for using null values in the grouping, but I'm not sure if I like it. They all go into an "Undefined" group, also not sure if count zero makes sense. Will update the PR soon.

[hbrunn]

does it hurt to have this?

[hbrunn]

it might be simpler to do this in https://github.com/OCA/OCB/blob/8.0/openerp/models.py#L1941
Here, there's no chance in hell to do this per record, so this should be its own action for sure, I don't have an opinion currently if we need read_group_groupby, read_group_order and read_group or if one suffices for all of them

[hbrunn]

changing the groupbys of course changes the shape of the result set, which might also cause trouble in calling code. If you go down the horribly slow route described above, you'd be better off ditching rows where a record with the groupby field inaccessible is involved

[dannyadair]

So records will disappear because they are grouped by a fields I can't access? Have a look at the latest changes, they'll show "Undefined" unless you have access to the value.

[hbrunn]

why do you need BaseModel's version here?

[hbrunn]

move this class to controllers/main.py, that's the conventional place where controllers should live

[hbrunn]

Oh, and please also don't forget to add tests proving that those functions are restricted too

[dannyadair]

Thanks for the heads up. I have to look more into _restrict_field_access_suspend(), I currently don't understand your suggested approach.

[hbrunn]

this looks very good already! Now just move the controller into its own file, then that's fine.

About _restrict_field_access_suspend: For the time being, I think you can simply depend on base_suspend_security, and return self.suspend_security() in _restrict_field_access_suspend. Then in _restrict_field_access_get_is_suspended, just check if the current uid is an instance of BaseSuspendSecurityUid, as in https://github.com/OCA/server-tools/blob/8.0/base_suspend_security/models/ir_model_access.py#L30, that's it. I think now this is even the better solution, because this way, we don't introduce another caching context.

[hbrunn]

@dannyadair my customer just reactivated this project. I'll thankfully merge your code as it is now, and implement my comments myself