Skip to content

[ADD] base_mixin_restrict_field_access - #396

Merged
pedrobaeza merged 4 commits into
OCA:8.0from
hbrunn:8.0-base_mixin_restrict_field_access
Sep 28, 2017
Merged

pedrobaeza merged 4 commits into
OCA:8.0from
hbrunn:8.0-base_mixin_restrict_field_access

Conversation

@hbrunn

@hbrunn hbrunn commented Mar 21, 2016

Copy link
Copy Markdown
Member

This is the result of #332:

Restrict field access

This module was written to help developers restricting access to fields in a
secure and flexible manner.

If you're not a developer, this module is not for you as you need to write code
in order to actually use it.

Usage

To use this module, you need to:

    class ResPartner(models.Model):
        # inherit from the mixin
        _inherit = ['restrict.field.access.mixin', 'res.partner']
        _name = 'res.partner'

        @api.multi
        def _restrict_field_access_get_field_whitelist(self, action='read'):
            # return a whitelist (or a blacklist) of fields, depending on the
            # action passed
            whitelist = [
                'name', 'parent_id', 'is_company', 'firstname', 'lastname',
                'infix', 'initials',
            ] + super(ResPartner, self)\
                ._restrict_field_access_get_field_whitelist(action=action)
            if action == 'read':
                whitelist.extend(['section_id', 'user_id'])
            return whitelist

        @api.multi
        def _restrict_field_access_is_field_accessible(self, field_name,
                                                       action='read'):
            # in case the whitelist is not enough, you can also decide for
            # specific records if an action can be carried out on it or not
            result = super(ResPartner, self)\
                ._restrict_field_access_is_field_accessible(
                    field_name, action=action)
            if result or not self:
                return result
            return all(this.section_id in self.env.user.section_ids or
                       this.user_id == self.env.user
                       for this in self)

        @api.multi
        @api.onchange('section_id', 'user_id')
        @api.depends('section_id', 'user_id')
        def _compute_restrict_field_access(self):
            # if your decision depends on other fields, you probably need to
            # override this function in order to attach the correct onchange/
            # depends decorators
            return super(ResPartner, self)._compute_restrict_field_access()

        @api.model
        def _restrict_field_access_inject_restrict_field_access_domain(
                self, domain):
            # you also might want to decide with a domain expression which
            # records are visible in the first place
            domain[:] = expression.AND([
                domain,
                [
                    '|',
                    ('section_id', 'in', self.env.user.section_ids.ids),
                    ('user_id', '=', self.env.user.id),
                ],
            ])

The example code here will allow only reading a few fields for partners of
which the current user is neither the sales person nor in this partner's sales
team.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+1.05%) to 56.966% when pulling 51ec5bc on hbrunn:8.0-base_mixin_restrict_field_access into 6e097c5 on OCA:8.0.

@hbrunn

hbrunn commented Mar 21, 2016

Copy link
Copy Markdown
Member Author

travis' complaint is a false positive discussed here: OCA/maintainer-quality-tools#307

@hbrunn hbrunn added this to the 8.0 milestone Mar 21, 2016
@pedrobaeza

Copy link
Copy Markdown
Member

You can add a comment for disabling the check on that line:

# pylint: disable=<warning-code>

@hbrunn

hbrunn commented Mar 22, 2016

Copy link
Copy Markdown
Member Author

@pedrobaeza thanks

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+1.08%) to 56.992% when pulling 1ad103e on hbrunn:8.0-base_mixin_restrict_field_access into 6e097c5 on OCA:8.0.

@pedrobaeza

Copy link
Copy Markdown
Member

Almost done:

base_mixin_restrict_field_access/tests/test_base_mixin_restrict_field_access.py:5:1: F401 'RegistryManager' imported but unused

@hbrunn

hbrunn commented Mar 22, 2016

Copy link
Copy Markdown
Member Author

saw it, thanks. Weird nothing complained about this one before. Anyways

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+1.08%) to 56.992% when pulling d993ccc on hbrunn:8.0-base_mixin_restrict_field_access into 6e097c5 on OCA:8.0.

Known issues / Roadmap
======================

* the code contains some TODOs which should be done

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to specify here the big issues: read_group, not so secure call and the rest of possible operations not covered.

@pedrobaeza

Copy link
Copy Markdown
Member

Little comment, but 👍

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+1.1%) to 57.014% when pulling c0c5548 on hbrunn:8.0-base_mixin_restrict_field_access into 6e097c5 on OCA:8.0.

@guewen

guewen commented Mar 31, 2016

Copy link
Copy Markdown
Member

👍

@dannyadair

Copy link
Copy Markdown

This doesn't seem to apply to CSV exports?

To reproduce please see this code:

`
from openerp.osv import osv
from openerp import api

class hr_employee(osv.osv):
_inherit = ['restrict.field.access.mixin', 'hr.employee']
_name = 'hr.employee'

@api.multi
@api.onchange('user_id')
@api.depends('user_id')
def _compute_restrict_field_access(self):
    """Re-determine field access when the employee's user changes."""
    return super(hr_employee, self)._compute_restrict_field_access()

@api.multi
def _restrict_field_access_is_field_accessible(self, field_name,
                                               action='read'):
    """Determine field access dynamically."""
    if field_name == 'birthday':
        return False
    result = super(hr_employee, self)._restrict_field_access_is_field_accessible(
        field_name, action=action
    )
    if result or not self:
        return result
    return True

`

(in the full code I work with two lists FIELDS_ONLY_SELF_OR_MANAGER and FIELDS_ONLY_MANAGER corresponding to the web interface's "Public Information"/"Personal Information"/"HR Settings" tabs)

So birthdate should always be inaccessible. And indeed, "Date of birth" on the form shows no value although there is one (even for the admin user). However, CSV export doesn't seem to care, and will always happily contain it.

When I click "Export To File" then _restrict_field_access_is_field_accessible() gets called with field_name 'birthday' and action 'read' but I assume that's just a side effect of reading the full record somewhere along the line.

I briefly looked at odoo.openerp.fields.convert_to_export()
Maybe inherit and use the same lists of restricted fields there to ensure they come out blank?
The field names themselves can't seem to be hidden but of course it's the value that's most important.

see also related odoo/odoo#12077

@hbrunn

hbrunn commented Jun 28, 2016

Copy link
Copy Markdown
Member Author

thanks for the pointer, I updated https://github.com/OCA/server-tools/pull/396/files#diff-9797ae3b8487be8a1f1cd8e6faea9ad8R14 accordingly.

The solution will be to check field access in https://github.com/OCA/OCB/blob/8.0/openerp/models.py#L859 and drop fields from the list as appropriate.

As the project for which I started this got suspended for a while, I won't be working on this any time soon, but I'll be happy to merge your PRs on this branch. As it seems you need this to be air tight, don't forget to also fix read_group as the comment above indicates. Setting this to work in progress meanwhile.

@hbrunn

hbrunn commented Jun 28, 2016

Copy link
Copy Markdown
Member Author

PS: The list of fields to export is compiled in https://github.com/OCA/OCB/blob/8.0/addons/web/controllers/main.py#L1291 if you want to change it

@dannyadair

Copy link
Copy Markdown

Thanks Holger, I monkeypatched web.ExportFormat.base to drop the fields. get_fields() looks like a cleaner location, and seems to have everything that's needed (model, request's user).

Hadn't noticed the read_group() issue until now, thanks for mentioning this. I'm going to Germany for a month after next week so this won't go in production here either anytime soon. :-)

@dannyadair

Copy link
Copy Markdown

Took me also a while to get back to this.
Please check out hbrunn#3

@hbrunn

hbrunn commented Sep 27, 2017

Copy link
Copy Markdown
Member Author

@guewen @pedrobaeza you've thumbed this up, can we merge if there's a green runbot?

@pedrobaeza

Copy link
Copy Markdown
Member

Do you plan to squash a bit the commit history?

hbrunn and others added 3 commits September 27, 2017 18:18
* Add implementation for restricted exporting and grouping

* Use original __export_rows() for models not using the mixin

* Assume all fields if not specified, like in original read_group()

* Return inaccessible fields in read_group() with null values

* No need to remove 'restrict_field_access'
@hbrunn
hbrunn force-pushed the 8.0-base_mixin_restrict_field_access branch from e40394a to ad02c34 Compare September 27, 2017 16:19
@hbrunn

hbrunn commented Sep 27, 2017

Copy link
Copy Markdown
Member Author

I did not, but you're right, that should happen. Done

@pedrobaeza
pedrobaeza merged commit d18bf0f into OCA:8.0 Sep 28, 2017
SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024
Syncing from upstream OCA/server-tools (10.0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants