Quantcast

Need comments on a better/different way to apply security to properties.

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Need comments on a better/different way to apply security to properties.

John P. Rouillard
Hi all:

Currently security permissions can be defined using addpermision() with
the following arguments:

  name: either standard permissions understood by the core:
          View, Create, Edit, Retire, Restore,
          Search, "Email Access", "Web Access",
          Register (I think this is all the core permissions,
          does anybody know of one I missed?)

        or you can create a new permissions for use by your
      tracker but not understood by the core.

  description: human readable string describing what the permission is for.

  klass (optional): string which is the name of a class in the database

  properties (optional): a tuple of strings which are the names of
       properties of the Klass.

  check function (optional): a function that is called with the
      arguments: (db, userid, itemid). If it returns true access is
      granted.  *NOTE* no property name is passed here even if the
      check is being used for a specific property.

Currently setting up property permissions is a pain if you are using a
check function. Since the check function doesn't know what property is
being checked, there has to be one

   security.addPermission(name='View', klass='issue',
                          properties=['verynosy'],
                          check=issue_prop_vn_view,
                          description="View issue property verynosy")

for every property that has a unique check function.

What I propose is to change the signature of the check function.
If the check function is defined as:

   def check_function(db, userid, itemid, **other):

the core code will call it as:

      check_function(db, userid, itemid,
        property=property,
        permission=permission,
        classname=classname):

E.G. check_function takes all the arguments of

  security.py:Permission::test(self, db, permission, classname, property, userid, itemid)

In the check function I can:

    def check_function(db, userid, itemid, **other):
        if 'property' in other:
            # caled with all args, assign them
            property = other['property']
            permission = other['permission']
        else:
            # This is a class check, not a property check
            # Allow edit and view.
            return True  

        # get some variables from the db

        if property in ('verynosy', 'nosy'):
            if permission = 'Edit':
                if userid in db.issue.get(itemid,property):
                    if property == 'verynosy':
                        if allow_verynosy_change(userid):
                            return True
                        else:
                            return False
                    return True
                return False    
            elif permission = 'View':
                do something different
        elif property in ('assignedto'):
            do something else
        elif property in ('dependson', 'groups', 'seealso'):
            do a third thing
        else:
             # let them edit/view the prop
             return True

In schema.py I only need to create one function and not seven (one
for each set of properties and 3 additional ones for Edit and View
functions for the first two properties).

With this new method I have only 1 check function and 4 permissions to
assign:

  security.addPermission(name='View', klass='issue',
            check=issue_proper_non_vn_editview,
            description="prop checks")

  security.addPermission(name='View', klass='issue',
            check=issue_proper_non_vn_editview,
            properties=('verynosy', 'nosy', 'assignedto',
                   'dependson', 'groups', 'seealso'),
            description="prop checks")

  security.addPermission(name='Edit', klass='issue',
            check=issue_proper_non_vn_editview
            properties=('verynosy', 'nosy')
            description="prop checks")

In the current method I need seven different check functions and 6
permission definitions.

Since this is a backwards incompatible change (I can't call a function
defined as: check_function(db, userid, itemid): with 6 parameters), I
am using the inspect module and:

     args=inspect.getargspec(check)

looking to see if the function id defined with a **variable (**other
in the example above). If so then I record that fact on initialization
and when the permission check function is called it is called using
the newer form.

I think this new mechanism should make it easier to generate complex
access controls and reduce the number of supporting functions in the
schema.

However I am worried that it makes the security command in
roundup-admin less useful which may impact the ability of people to
audit their configurations. I also don't know what else I am missing
in the permission model.

Quips, comments, evasions, questions or answers welcome.

--
                                -- rouilj
John Rouillard
===========================================================================
My employers don't acknowledge my existence much less my opinions.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Need comments on a better/different way to apply security to properties.

Thomas Arendsen Hein
* John P. Rouillard <[hidden email]> [20170212 02:36]:

> In the current method I need seven different check functions and 6
> permission definitions.
>
> Since this is a backwards incompatible change (I can't call a function
> defined as: check_function(db, userid, itemid): with 6 parameters), I
> am using the inspect module and:
>
>      args=inspect.getargspec(check)
>
> looking to see if the function id defined with a **variable (**other
> in the example above). If so then I record that fact on initialization
> and when the permission check function is called it is called using
> the newer form.

This is too magic for me, I'd recommend using a second parameter,
e.g. checkprops=foo, in addition to the existing check=foo.

This way I know: If I'm using just "check=foo", there won't be
different results for different properties.

> I think this new mechanism should make it easier to generate complex
> access controls and reduce the number of supporting functions in the
> schema.
>
> However I am worried that it makes the security command in
> roundup-admin less useful which may impact the ability of people to
> audit their configurations. I also don't know what else I am missing
> in the permission model.

I think offering this flexibility in addition to the current way
would be fine.

But make sure to define the behaviour if both, check and checkprops,
are specified: Either bail out with requiring to specify at most
one, or do both checks and require both to pass.

Regards,
Thomas

--
[hidden email] - http://intevation.de/~thomas/ - OpenPGP key: 0x5816791A
Intevation GmbH, Neuer Graben 17, 49074 Osnabrueck - AG Osnabrueck, HR B 18998
Geschaeftsfuehrer: Frank Koormann, Bernhard Reiter, Dr. Jan-Oliver Wagner

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Need comments on a better/different way to apply security to properties.

John P. Rouillard
Hello Thomas:

In message <[hidden email]>,
Thomas Arendsen Hein writes:

>* John P. Rouillard <[hidden email]> [20170212 02:36]:
>> In the current method I need seven different check functions and 6
>> permission definitions.
>>
>> Since this is a backwards incompatible change (I can't call a function
>> defined as: check_function(db, userid, itemid): with 6 parameters), I
>> am using the inspect module and:
>>
>>      args=inspect.getargspec(check)
>>
>> looking to see if the function id defined with a **variable (**other
>> in the example above). If so then I record that fact on initialization
>> and when the permission check function is called it is called using
>> the newer form.
>
>This is too magic for me,

Yeah I understand your concern. I was worried that I was being too
tricky and that it would break in the port to python 3. But it is
supported in python 3.

>I'd recommend using a second parameter,
>e.g. checkprops=foo, in addition to the existing check=foo.
>
>This way I know: If I'm using just "check=foo", there won't be
>different results for different properties.

That still means that I need to write two check functions rather than
one since I can't reuse foo for check and checkprops.

Also I would like to get rid of the old style calls and make the
signature of every check script:

   check_script(db, userid, itemid, **other)

so there is only one form to worry about. If people don't need the
**other dict, they can just ignore it similar to how itemid can be
ignored if unneeded.

>> I think this new mechanism should make it easier to generate complex
>> access controls and reduce the number of supporting functions in the
>> schema.
>>
>> However I am worried that it makes the security command in
>> roundup-admin less useful which may impact the ability of people to
>> audit their configurations. I also don't know what else I am missing
>> in the permission model.
>
>I think offering this flexibility in addition to the current way
>would be fine.
>
>But make sure to define the behaviour if both, check and checkprops,
>are specified: Either bail out with requiring to specify at most
>one, or do both checks and require both to pass.

That is another reason I went with just the single check parameter.  I
think regardless of which choice I go with it is not going to meet
somebody's use case.

Also I am not exactly sure what order calls (should) happen in when
you have

  security.addPermission(name='Edit', klass='issue',
                         properties=['verynosy'],
                         check=issue_edit_prop_verynosy,
                         description="Edit issue property verynosy")

  security.addPermission(name='Edit', klass='issue',
                              properties=['nosy'],
                              check=issue_edit_prop_nosy,
                              description="Edit issue property nosy")

  security.addPermission(name='Edit', klass='issue',
                              check=issue_edit,
                              description="Edit issue")

and similar for View permissions and you try to display the nosy or
verynosy props in issue.item.html.

I did a fast audit and I think adding a checkprops is a bit more
invasive as well. Implementing my change as above is 14 lines in two
functions. I see at least 8 functions that need to change to do
checkprops.

Does anybody else have any opinion here?

--
                                -- rouilj
John Rouillard
===========================================================================
My employers don't acknowledge my existence much less my opinions.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Need comments on a better/different way to apply security to properties.

Thomas Arendsen Hein
* John P. Rouillard <[hidden email]> [20170217 02:22]:
> In message <[hidden email]>,
> Thomas Arendsen Hein writes:
> >This is too magic for me,
>
> Yeah I understand your concern. I was worried that I was being too
> tricky and that it would break in the port to python 3. But it is
> supported in python 3.

OK, so no problem here.

> >I'd recommend using a second parameter,
> >e.g. checkprops=foo, in addition to the existing check=foo.
> >
> >This way I know: If I'm using just "check=foo", there won't be
> >different results for different properties.
>
> That still means that I need to write two check functions rather than
> one since I can't reuse foo for check and checkprops.

I thought you could reuse it, because all you have to do is throwing
away the **other part.

> Also I would like to get rid of the old style calls and make the
> signature of every check script:
>
>    check_script(db, userid, itemid, **other)
>
> so there is only one form to worry about. If people don't need the
> **other dict, they can just ignore it similar to how itemid can be
> ignored if unneeded.

OK, no problem here either.

> >I think offering this flexibility in addition to the current way
> >would be fine.
> >
> >But make sure to define the behaviour if both, check and checkprops,
> >are specified: Either bail out with requiring to specify at most
> >one, or do both checks and require both to pass.
>
> That is another reason I went with just the single check parameter.  I
> think regardless of which choice I go with it is not going to meet
> somebody's use case.

That's definitively an advantage for your suggestion.

> Also I am not exactly sure what order calls (should) happen in when
> you have
>
>   security.addPermission(name='Edit', klass='issue',
>                          properties=['verynosy'],
>                          check=issue_edit_prop_verynosy,
>                          description="Edit issue property verynosy")
>
>   security.addPermission(name='Edit', klass='issue',
>                               properties=['nosy'],
>                               check=issue_edit_prop_nosy,
>                               description="Edit issue property nosy")
>
>   security.addPermission(name='Edit', klass='issue',
>                               check=issue_edit,
>                               description="Edit issue")

I'd read that as: The order does not matter, because as soon as one
of the checks say "it is fine", it should be allowed.

You would need a long list of properties for the third call if you
don't want to allow checks on nosy and verynosy here.

And with that you convinced me :)

Go ahead!

Regards,
Thomas

--
[hidden email] - http://intevation.de/~thomas/ - OpenPGP key: 0x5816791A
Intevation GmbH, Neuer Graben 17, 49074 Osnabrueck - AG Osnabrueck, HR B 18998
Geschaeftsfuehrer: Frank Koormann, Bernhard Reiter, Dr. Jan-Oliver Wagner

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Need comments on a better/different way to apply security to properties.

Ralf Schlatterbeck-3
In reply to this post by John P. Rouillard
On Thu, Feb 16, 2017 at 08:22:26PM -0500, John P. Rouillard wrote:
>
> I did a fast audit and I think adding a checkprops is a bit more
> invasive as well. Implementing my change as above is 14 lines in two
> functions. I see at least 8 functions that need to change to do
> checkprops.
>
> Does anybody else have any opinion here?

Looks good to me, so I'm with Thomas here.
I *do* have quite complicated and many permission checks in some of the
trackers I'm using and this would considerably improve things. It has
also turned out for me that maintaining permissions is occurring quite
often (as opposed to simple trackers where you can probably get away
with writing the permission checks once and never touching them again).

Ralf
--
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   http://www.runtux.com
Reichergasse 131, A-3411 Weidling       email: [hidden email]

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Need comments on a better/different way to apply security to properties.

John P. Rouillard
In reply to this post by Thomas Arendsen Hein
Hi Thomas:

In message <[hidden email]>,
Thomas Arendsen Hein writes:

>* John P. Rouillard <[hidden email]> [20170217 02:22]:
>> In message <[hidden email]>,
>> Thomas Arendsen Hein writes:
>> >This is too magic for me,
>> Yeah I understand your concern. I was worried that I was being too
>> tricky and that it would break in the port to python 3. But it is
>> supported in python 3.
>OK, so no problem here.
>> >I'd recommend using a second parameter,
>> >e.g. checkprops=foo, in addition to the existing check=foo.
>> >
>> >This way I know: If I'm using just "check=foo", there won't be
>> >different results for different properties.
>>
>> That still means that I need to write two check functions rather than
>> one since I can't reuse foo for check and checkprops.
>
>I thought you could reuse it, because all you have to do is throwing
>away the **other part.

You are right, I can reuse the function if I define it the new way.
If my function is:

  check(db, userid, itemid, **other)

I can call it with:

   (db, userid, itemid) or
   (db, userid, itemid, properties=[], more named arguments)

without an issue. It's only if the signature is

  check(db, userid, itemid)

that I get in trouble calling it with:

   (db, userid, itemid, properties=[], more named arguments)

[...]

>> >But make sure to define the behaviour if both, check and checkprops,
>> >are specified: Either bail out with requiring to specify at most
>> >one, or do both checks and require both to pass.
>>
>> That is another reason I went with just the single check parameter.  I
>> think regardless of which choice I go with it is not going to meet
>> somebody's use case.
>
>That's definitively an advantage for your suggestion.
>
>> Also I am not exactly sure what order calls (should) happen in when
>> you have
>>
>>   security.addPermission(name='Edit', klass='issue',
>>                          properties=['verynosy'],
>>                          check=issue_edit_prop_verynosy,
>>                          description="Edit issue property verynosy")
>>
>>   security.addPermission(name='Edit', klass='issue',
>>                               properties=['nosy'],
>>                               check=issue_edit_prop_nosy,
>>                               description="Edit issue property nosy")
>>
>>   security.addPermission(name='Edit', klass='issue',
>>                               check=issue_edit,
>>                               description="Edit issue")
>
>I'd read that as: The order does not matter, because as soon as one
>of the checks say "it is fine", it should be allowed.
>
>You would need a long list of properties for the third call if you
>don't want to allow checks on nosy and verynosy here.

So is this a true series of statements:

  If I want to see if the user can edit an issue, for the issue and
  every property try each of the three permissions.

  * If the property is verynosy try permission 1 and permission 3.
  * If the property is nosy try permission 2 and permission 3.
  * If the property is something else try only permission 3.
  * If there is no property try only permission 3.

  If permission 3 usually passes for the user (because they can see
  the issue and most properties) then they end up editing the nosy and
  verynosy properties as well because 3 is always called and returns
  true.

So I can't use the third permission as is, I need to have a properties
array with every other property of the issue listed.

If that is true what do I pass to addPermission to test to see if just
the issue can be displayed? I need some permission without any
properties set right?

>And with that you convinced me :)

Whew. I was worried there for a bit.

>Go ahead!

Thanks. Now to write some tests for it.

--
                                -- rouilj
John Rouillard
===========================================================================
My employers don't acknowledge my existence much less my opinions.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Need comments on a better/different way to apply security to properties.

Thomas Arendsen Hein
* John P. Rouillard <[hidden email]> [20170218 01:21]:

> In message <[hidden email]>,
> Thomas Arendsen Hein writes:
> >* John P. Rouillard <[hidden email]> [20170217 02:22]:
> >> Also I am not exactly sure what order calls (should) happen in when
> >> you have
> >>
> >>   security.addPermission(name='Edit', klass='issue',
> >>                          properties=['verynosy'],
> >>                          check=issue_edit_prop_verynosy,
> >>                          description="Edit issue property verynosy")
> >>
> >>   security.addPermission(name='Edit', klass='issue',
> >>                               properties=['nosy'],
> >>                               check=issue_edit_prop_nosy,
> >>                               description="Edit issue property nosy")
> >>
> >>   security.addPermission(name='Edit', klass='issue',
> >>                               check=issue_edit,
> >>                               description="Edit issue")
> >
> >I'd read that as: The order does not matter, because as soon as one
> >of the checks say "it is fine", it should be allowed.
> >
> >You would need a long list of properties for the third call if you
> >don't want to allow checks on nosy and verynosy here.
>
> So is this a true series of statements:
>
>   If I want to see if the user can edit an issue, for the issue and
>   every property try each of the three permissions.
>
>   * If the property is verynosy try permission 1 and permission 3.

Yes, but apply a logical OR: If 1 already returns True, you are
allowed to edit. Or check 3 first if you like, does not matter.

>   * If the property is nosy try permission 2 and permission 3.

Yes, but apply a logical OR: If 2 already returns True, you are
allowed to edit. Or check 3 first if you like, does not matter.

>   * If the property is something else try only permission 3.

Yes.

>   * If there is no property try only permission 3.

If there is no property to edit, you're not editing anything, so no
check is needed. But code can "manually" use 3 to get a fuzzy idea
whether editing of an issue is possible or not, e.g. in the display
logic of templates to see if something should be rendered or not.

>   If permission 3 usually passes for the user (because they can see

see = view, not edit. But I think I get what you mean here.

>   the issue and most properties) then they end up editing the nosy and
>   verynosy properties as well because 3 is always called and returns
>   true.

Correct, that's why I currently need that long list of properties
with addPermission to allow editing all but very few properties,
unless I'm using an auditor to enforce this after the fact, which of
course would be problematic.

> So I can't use the third permission as is, I need to have a properties
> array with every other property of the issue listed.

Correct.

> If that is true what do I pass to addPermission to test to see if just
> the issue can be displayed? I need some permission without any
> properties set right?

Correct. And if you are allowed to view an issue, but none of its
properties, you will get a lot of "[hidden]" values displayed for
those properties.

> >And with that you convinced me :)
>
> Whew. I was worried there for a bit.

:-)

> >Go ahead!
>
> Thanks. Now to write some tests for it.

Thank you for doing the actual work. All I do is "talking" :)

Regards,
Thomas

--
[hidden email] - http://intevation.de/~thomas/ - OpenPGP key: 0x5816791A
Intevation GmbH, Neuer Graben 17, 49074 Osnabrueck - AG Osnabrueck, HR B 18998
Geschaeftsfuehrer: Frank Koormann, Bernhard Reiter, Dr. Jan-Oliver Wagner

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Need comments on a better/different way to apply security to properties.

John P. Rouillard
Hi Thomas:

In message <[hidden email]>,
Thomas Arendsen Hein writes:
>* John P. Rouillard <[hidden email]> [20170218 01:21]:
>> In message <[hidden email]>,
>> Thomas Arendsen Hein writes:
>> >* John P. Rouillard <[hidden email]> [20170217 02:22]:
>> >> Also I am not exactly sure what order calls (should) happen in when
>> >> you have

I have elided the discussion but yes my understanding agrees with your
statements.

>>   * If there is no property try only permission 3.
>
>If there is no property to edit, you're not editing anything, so no
>check is needed. But code can "manually" use 3 to get a fuzzy idea
>whether editing of an issue is possible or not, e.g. in the display
>logic of templates to see if something should be rendered or not.

Right I was thinking of context/is_edit_ok when in an issue.item.html
page.

>
>>   If permission 3 usually passes for the user (because they can see
>
>see = view, not edit. But I think I get what you mean here.
>
>>   the issue and most properties) then they end up editing the nosy and
>>   verynosy properties as well because 3 is always called and returns
>>   true.
>
>Correct, that's why I currently need that long list of properties
>with addPermission to allow editing all but very few properties,
>unless I'm using an auditor to enforce this after the fact, which of
>course would be problematic.

Yup allowing the user to edit something only to throw it away in the
auditor is not friendly.

Are you in a position to try the current roundup head and rewrite the
check command using the new **ctx parameter. I'll bet it cleans up a
lot of logic. I know it did for me. This past weekend I rewrote all
the security checks and for the first time I was able to implement the
security model I wanted.

Have a great week.

--
                                -- rouilj
John Rouillard
===========================================================================
My employers don't acknowledge my existence much less my opinions.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Loading...