Code vs Access Rights

[Cédric Krier]

Hi,

I'm facing an issue with the access right.
Here is a summary of the issue:

The access rights are enforced in Tryton in the CRWD methods, this
means that everywhere those rights are enforced except if the
context is switched to root.
So the problem is that most of the time, the developper forget about
this when writing code by assuming he will have access to
everything.
For example in account_stock_anglo_saxon:
The opening of invoice needs to read the moves via the
sale/purchase, but the user could have only access to account
stuffs.

I see two solutions to this issue:

- Using the root context switching in the code everywhere it is
needed based on the default access right define in Tryton.
This means the developper must take care of this everytime he
write code. This is a little bit constraining but it has the
advantage to execute the smaller part of the code as root.

- Remove the access right from within the CRUD to move it just on
the rpc calls and run all the code as root.
This has the advantage to be simple but it is a bad design for
security principle to run the least possible code as root.

So I would like to collect thoughts about this topic to decide how to
fix "account_stock_anglo_saxon" and the future issues.

--
Cédric Krier

B2CK SPRL
Rue de Rotterdam, 4
4000 Liège
Belgium
Tel: +32 472 54 46 59
Email/Jabber: cedric...@b2ck.com
Website: http://www.b2ck.com/

[Sharoon Thomas]

On May 18, 2012, at 3:33 PM, Cédric Krier wrote:

> I see two solutions to this issue:
>
> - Using the root context switching in the code everywhere it is
> needed based on the default access right define in Tryton.
> This means the developper must take care of this everytime he
> write code. This is a little bit constraining but it has the
> advantage to execute the smaller part of the code as root.

+1 seems like the best thing to do.

>
> - Remove the access right from within the CRUD to move it just on
> the rpc calls and run all the code as root.
> This has the advantage to be simple but it is a bad design for
> security principle to run the least possible code as root.

I think limiting security to RPC (i think that implementation will
be at the dispatcher level), will be bad for projects which use
tryton as a module. They will need to reimplement the same thing
over and over.

However, your suggestion brings us to a whole new way
in which we could probably enforce access control logic. At the
moment we defined ACL on groups and the membership of the user
to these groups define the rights of the user. While, retaining this
behavior we could switch the CRUD definitions also to python code
like what we did with Workflow transitions (From xml through database
to Python code).

Eg:

@Model.user_in_group(['sale.sale', 'product.product_admin'])
def create(self, vals):
do create only if the user has permissions

This will allow us to reuse the same decorator for steps in workflow
as well like:

@Model.user_in_group(['sale.sale_manager'])
@ModelView.button
@Workflow.transition('confirmed')
def confirm_without_credit_check(self, ids):
pass


Thanks

Sharoon Thomas
Openlabs Technologies & Consulting (P) Limited

w: http://www.openlabs.co.in
t: @sharoonthomas

- We CARE for our customers

[Cédric Krier]

On 18/05/12 20:06 +0530, Sharoon Thomas wrote:
> However, your suggestion brings us to a whole new way
> in which we could probably enforce access control logic. At the
> moment we defined ACL on groups and the membership of the user
> to these groups define the rights of the user. While, retaining this
> behavior we could switch the CRUD definitions also to python code
> like what we did with Workflow transitions (From xml through database
> to Python code).
>
> Eg:
>
> @Model.user_in_group(['sale.sale', 'product.product_admin'])
> def create(self, vals):
> do create only if the user has permissions
>
> This will allow us to reuse the same decorator for steps in workflow
> as well like:
>
> @Model.user_in_group(['sale.sale_manager'])
> @ModelView.button
> @Workflow.transition('confirmed')
> def confirm_without_credit_check(self, ids):
> pass

I think it is wrong to put xml_id of groups in the code because it
prevents to create new groups with similar access rights without monkey
patching the method.

[Romain Séon]

>    - Using the root context switching in the code everywhere it is
>      needed based on the default access right define in Tryton.
>      This means the developper must take care of this everytime he
>      write code. This is a little bit constraining but it has the
>      advantage to execute the smaller part of the code as root.
>
>    - Remove the access right from within the CRUD to move it just on
>      the rpc calls and run all the code as root.
>      This has the advantage to be simple but it is a bad design for
>      security principle to run the least possible code as root.

+1 on this solution

[Korbinian Preisler]

Am Freitag, den 18.05.2012, 20:06 +0530 schrieb Sharoon Thomas:
> On May 18, 2012, at 3:33 PM, Cédric Krier wrote:
>
> > I see two solutions to this issue:
> >
> > - Using the root context switching in the code everywhere it is
> > needed based on the default access right define in Tryton.
> > This means the developper must take care of this everytime he
> > write code. This is a little bit constraining but it has the
> > advantage to execute the smaller part of the code as root.
>
> +1 seems like the best thing to do.

I agree that this should be the best way to solve the issue.

--
Korbinian Preisler
____________________________________
virtual things
Preisler & Spallek GbR
Munich - Aix-la-Chapelle

Windeckstr. 77
81375 Munich - Germany
Tel: +49 (89) 710 481 55
Fax: +49 (89) 710 481 56

in...@virtual-things.biz
http://www.virtual-things.biz

[Giedrius Slavinskas]

2012 m. gegužė 18 d., penktadienis 17:36:52 UTC+3, Sharoon Thomas rašė:

On May 18, 2012, at 3:33 PM, Cédric Krier wrote:

> I see two solutions to this issue:
>
>    - Using the root context switching in the code everywhere it is
>      needed based on the default access right define in Tryton.
>      This means the developper must take care of this everytime he
>      write code. This is a little bit constraining but it has the
>      advantage to execute the smaller part of the code as root.

+1 seems like the best thing to do.

>
>    - Remove the access right from within the CRUD to move it just on
>      the rpc calls and run all the code as root.
>      This has the advantage to be simple but it is a bad design for
>      security principle to run the least possible code as root.

It is more elegant solution than switching context, but it won't work on
current cumbersome design of Tryton. The main problem of current
design is that there is no separation between controller (interface for
objects dispatch) and model (data structure, application state). I see
some work is made in right direction by implementing Active record
pattern on models. So now (if i'm not wrong) @classmethod represents
controller while other methods are used for changing state of Active
Record instance. By having this design we can go further by declaring
that access rights should never go on model level, its place is in
controller level. Also we must make sure that dispatched method
is never called by another dispatched method.

I think limiting security to RPC (i think that implementation will
be at the dispatcher level), will be bad for projects which use
tryton as a module. They will need to reimplement the same thing
over and over.

It's even better. You can use data model without any security and
integrate them with different security system. Or if you don't want to
do this, you can use dispatched methods from Tryton itself.
 

However, your suggestion  brings us to a whole new way
in which we could probably enforce access control logic. At the
moment we defined ACL on groups and the membership of the user
to these groups define the rights of the user. While, retaining this
behavior we could switch the CRUD definitions also to python code
like what we did with Workflow transitions (From xml through database
to Python code).

Eg:

@Model.user_in_group(['sale.sale', 'product.product_admin'])
def create(self, vals):
    do create only if the user has permissions

This will allow us to reuse the same decorator for steps in workflow
as well like:

@Model.user_in_group(['sale.sale_manager'])
@ModelView.button
@Workflow.transition('confirmed')
def confirm_without_credit_check(self, ids):
    pass

It's generally bad idea to use xml IDs in python code. But we need
something if we going to allow security not only on 4 dispatched
methods (CRUD). The number of dispatched methods (RPC calls) is
indeterminate. It could be implemented like this:

class SaleOrder(...):
    __name__ = 'sale.order'

    @Permission.need('create', 'process')
    @classmethod
    def do_something_fancy(self, ids):
        pass

Groups is described in xml file with provided permissions.
sales_manager_group = {
    'sale.order': ['create', 'process', 'delete', 'confirm', 'update', 'read', 'search']
}

[Cédric Krier]

On 22/05/12 02:38 -0700, Giedrius Slavinskas wrote:
> > >
> > > - Remove the access right from within the CRUD to move it just on
> > > the rpc calls and run all the code as root.
> > > This has the advantage to be simple but it is a bad design for
> > > security principle to run the least possible code as root.
> >
> >
> It is more elegant solution than switching context, but it won't work on
> current cumbersome design of Tryton. The main problem of current
> design is that there is no separation between controller (interface for
> objects dispatch) and model (data structure, application state).

It could be also an advantage. Because of this design, the record rule
can exist. And in some way, it makes the access right very simple just
CRUD (almost like for unix)

> I see
> some work is made in right direction by implementing Active record
> pattern on models. So now (if i'm not wrong) @classmethod represents
> controller while other methods are used for changing state of Active
> Record instance.

No, indeed the principle are still the same but instead of having an
instance as "controller" it is a class.
And "changing state" will still be done by class methods.

> By having this design we can go further by declaring
> that access rights should never go on model level, its place is in
> controller level. Also we must make sure that dispatched method
> is never called by another dispatched method.

But that's a big issue because the main dispatched methods are CRUD
because we use a thin client that can almost only do that.
More over, one method call can do many things like operation on xxx2many
fields, so if we check only the dispatched method we allow any calls on
xxx2many methods.

> I think limiting security to RPC (i think that implementation will
> > be at the dispatcher level), will be bad for projects which use
> > tryton as a module. They will need to reimplement the same thing
> > over and over.
> >
> >
> It's even better. You can use data model without any security and
> integrate them with different security system. Or if you don't want to
> do this, you can use dispatched methods from Tryton itself.

That's really something out of the scope.

[Albert Cervera i Areny]

A Divendres, 18 de maig de 2012 12:03:02, C�dric Krier va escriure:

> I see two solutions to this issue:

>

> - Using the root context switching in the code everywhere it is

> needed based on the default access right define in Tryton.

> This means the developper must take care of this everytime he

> write code. This is a little bit constraining but it has the

> advantage to execute the smaller part of the code as root.

>

> - Remove the access right from within the CRUD to move it just on

> the rpc calls and run all the code as root.

> This has the advantage to be simple but it is a bad design for

> security principle to run the least possible code as root.

>

 

I think there's a third option. We can add a new boolean field called "Use root context" to the new button access rights model that was introduced in 2.4. If this fields is set to True, the decorator of the button will use the root context. If not, it will work as it currently does.

 

The patch to implement this would be trivial and allows users to define their own access rights from scratch without touching a single line of code and give options to those who prefer action-based access rights override CRUD rights but also allow those that want to be more paranoid and force the definition specific CRUD permissions.

--

Albert Cervera i Areny

http://www.NaN-tic.com

Tel: +34 93 553 18 03

 

http://twitter.com/albertnan

http://www.nan-tic.com/blog

 

[Cédric Krier]

On 23/05/12 17:55 +0200, Albert Cervera i Areny wrote:
> A Divendres, 18 de maig de 2012 12:03:02, Cédric Krier va escriure:

> > I see two solutions to this issue:
> >
> > - Using the root context switching in the code everywhere it is
> > needed based on the default access right define in Tryton.
> > This means the developper must take care of this everytime he
> > write code. This is a little bit constraining but it has the
> > advantage to execute the smaller part of the code as root.
> >
> > - Remove the access right from within the CRUD to move it just on
> > the rpc calls and run all the code as root.
> > This has the advantage to be simple but it is a bad design for
> > security principle to run the least possible code as root.
> >
>
> I think there's a third option. We can add a new boolean field called "Use root
> context" to the new button access rights model that was introduced in 2.4. If
> this fields is set to True, the decorator of the button will use the root
> context. If not, it will work as it currently does.

It is not only linked to the button methods. Some method could be
triggered by a CRUD operation.

[Albert Cervera i Areny]

A Dimecres, 23 de maig de 2012 18:06:04, C�dric Krier va escriure:

> On 23/05/12 17:55 +0200, Albert Cervera i Areny wrote:

> > A Divendres, 18 de maig de 2012 12:03:02, C�dric Krier va escriure:

> > > I see two solutions to this issue:

> > > - Using the root context switching in the code everywhere it is

> > >

> > > needed based on the default access right define in Tryton.

> > > This means the developper must take care of this everytime he

> > > write code. This is a little bit constraining but it has the

> > > advantage to execute the smaller part of the code as root.

> > >

> > > - Remove the access right from within the CRUD to move it just on

> > >

> > > the rpc calls and run all the code as root.

> > > This has the advantage to be simple but it is a bad design for

> > > security principle to run the least possible code as root.

> >

> > I think there's a third option. We can add a new boolean field called

> > "Use root context" to the new button access rights model that was

> > introduced in 2.4. If this fields is set to True, the decorator of the

> > button will use the root context. If not, it will work as it currently

> > does.

>

> It is not only linked to the button methods. Some method could be

> triggered by a CRUD operation.

 

But, we could add the button decorator to the triggered method.

[Cédric Krier]

On 23/05/12 18:23 +0200, Albert Cervera i Areny wrote:
> A Dimecres, 23 de maig de 2012 18:06:04, Cédric Krier va escriure:

> > On 23/05/12 17:55 +0200, Albert Cervera i Areny wrote:

> > > A Divendres, 18 de maig de 2012 12:03:02, Cédric Krier va escriure:

> > > > I see two solutions to this issue:
> > > > - Using the root context switching in the code everywhere it is
> > > >
> > > > needed based on the default access right define in Tryton.
> > > > This means the developper must take care of this everytime he
> > > > write code. This is a little bit constraining but it has the
> > > > advantage to execute the smaller part of the code as root.
> > > >
> > > > - Remove the access right from within the CRUD to move it just on
> > > >
> > > > the rpc calls and run all the code as root.
> > > > This has the advantage to be simple but it is a bad design for
> > > > security principle to run the least possible code as root.
> > >
> > > I think there's a third option. We can add a new boolean field called
> > > "Use root context" to the new button access rights model that was
> > > introduced in 2.4. If this fields is set to True, the decorator of the
> > > button will use the root context. If not, it will work as it currently
> > > does.
> >
> > It is not only linked to the button methods. Some method could be
> > triggered by a CRUD operation.
>
> But, we could add the button decorator to the triggered method.

Then it is the same as switching context on each methods.

[Cédric Krier]

On 18/05/12 12:03 +0200, Cédric Krier wrote:
> Hi,
>
> I'm facing an issue with the access right.
> Here is a summary of the issue:
>
> The access rights are enforced in Tryton in the CRWD methods, this
> means that everywhere those rights are enforced except if the
> context is switched to root.
> So the problem is that most of the time, the developper forget about
> this when writing code by assuming he will have access to
> everything.
> For example in account_stock_anglo_saxon:
> The opening of invoice needs to read the moves via the
> sale/purchase, but the user could have only access to account
> stuffs.
>
> I see two solutions to this issue:
>
> - Using the root context switching in the code everywhere it is
> needed based on the default access right define in Tryton.
> This means the developper must take care of this everytime he
> write code. This is a little bit constraining but it has the
> advantage to execute the smaller part of the code as root.

So I will fix anglo_saxon using this method.
Let continue to use this pattern and we will see if in the future we
will face too much issues.