shiro-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Claude Warren <cla...@xenei.com>
Subject Re: Defect in WildcardPermission evaluation?
Date Wed, 24 Jan 2018 20:51:08 GMT
I think we are getting closer to an understanding.

Try adding the following to your  tests:

@Test
    public void testWildcardLeftTermination() {
        WildcardPermission p1, p2;

        p1 = new WildcardPermission("one");
        p2 = new WildcardPermission("one:*");

        assertTrue( p1.implies(p2));
        assertFalse( p2.implies(p1));
    }

Now I assume the the last statement is correct.  but the test will fail.

Now to your example with JoeCoder

Assume your resource has the permission "newsletter"

Should the user joe with permissions "newsletter:*" be able to access that?

a user with "newsletter:one" would not.

So looking at it another way.

given: "newsletter:X" is the set of all permissions in the system that
start with "newsletter:" followed by any characters excluding ":" and "*".

then: any user with only the permissions from "newsletter:X" does not have
access to "newsletter"

given that "newsletter:*" is equivalent to the union of "newsletter:X" and
newsletter:X:X and newsletter:X:X:X ...

then: any user with only the permissions from "newsletter:*" should not
have access to "newsletter" as you can not imply "newsletter" from any of
the permissions in the set.

corollary:

if "newsletter:*" implies "newsletter" then for any permission Y the
permission Y:*  implies Y

replacing  Y with "newsletter:*" gives newsletter:*:*  implies newsletter:*
and newsletter:* implies newsletter so newsletter:*:* implies newsletter.
This is false.

so "newsletter:*" should not imply "newsletter"


@Test
    public void testWildcardLeftTermination() {
        WildcardPermission p1, p2, p3, p4;

        p1 = new WildcardPermission("one");
        p2 = new WildcardPermission("one:*");
        p3 = new WildcardPermission("one:*:*");

        assertTrue( p1.implies(p2));
        //assertFalse( p2.implies(p1));  //test 2
        assertTrue( p1.implies(p3));
        assertFalse( p3.implies(p1 )); // test 4
    }

To be consistent test 2 and  test 4 must both be true or both false.

Claude

On Wed, Jan 24, 2018 at 7:57 PM, Brian Demers <brian.demers@gmail.com>
wrote:

> I think I understand what you saying, (you can blame bias and assumptions,
> or my thick head)
>
> This isn't specific to an `*`, as `newsletter` => `newsletter:foo:bar:read`
>
> I added a couple quick tests here:
> https://github.com/apache/shiro/pull/78/files
>
> I always think about it this way. Users (Subjects) get generic permission
> strings, resources get specific ones. (this isn't the only way, but this
> how I think about it)
>
> My User Joe Coder would get the permission `newsletter` (or `newletter:*`
> or `newletter:*:*)
> The resource would check for the permission `newsletter:boston:read`
>
> You could check this programmatically with
> joeCoder.hasPermission("newsletter:boston:read")
>
> Or as an annotation example (in this case a JAXRS resource):
>
> @RequiresPermissions("newsletter:boston:read")
> @Path("/newletters/boston")
> @GET
> public String readTheBostonNewsletter() {
>     return "Some text here";
> }
>
>
> Does that help clear it up?
>
>
> On Wed, Jan 24, 2018 at 12:53 PM, Claude Warren <claude@xenei.com> wrote:
>
> > I was trying to make the printer example work for the issue I am seeing.
> > But I can't seem to get the problem across. :(
> >
> > newsletter:*:*:*  => newsletter:read
> >
> > is not the same as there is an asterisk to match with "read"
> >
> > newsletter:*:*:*  => newsletter
> >
> > would be closer and
> >
> > newsletter:*  => newsletter
> >
> > would be the same.
> >
> > So should the first wildcard include the permission just "above" it.
> >
> > Another way to look at this is that wildcards match any number of
> > characters, so the simple case of newsletter:* says match "newsletter:"
> and
> > then allow for any number of characters.  However when we start using ":"
> > as a part separator character then we say
> >
> > newsletter:* matches "newsletter" an any characters after.  though I
> don't
> > think this is quite true either as "newsletters" will not match
> > "newsletter:*"
> >
> > if "newsletter:*" matches "newsletter" then how do you specify that only
> > properties below "newsletter" are being granted?
> >
> > Claude
> >
> >
> >
> >
> >
> >
> >
> >
> > On Wed, Jan 24, 2018 at 3:05 PM, Brian Demers <brian.demers@gmail.com>
> > wrote:
> >
> > > Claude
> > >
> > > The test I linked to is:
> > > newsletter:*:*:*  => newsletter:read
> > >
> > > Which is similar to what you are saying.  I'm all for adding more test
> to
> > > make this more clear though!
> > >
> > > I think part of this issue might be a modeling one (maybe we need to
> > > improve the doc?)
> > >
> > > Typically you would assign your users a wildcard (or more loose
> > permission)
> > > and your resource a very specific one.
> > >
> > > In your example your printer tech would have `printer:*` (or just
> > > `printer), the 'guru' would have something like
> `printer:configure,print`
> > >
> > > When adding a new printer your application would check for the specific
> > > permission `printer:add`  (this would allow the tech to have access but
> > not
> > > the guru)
> > >
> > >
> > > You could also add a printer Id to the permissions for the same thing:
> > > The tech would have `printer:*:*`, the guru might have a restricted
> > access
> > > to configure a few printers:
> > > `printer:office-printer1,basement-printer-2:configure`, and also the
> > > permission to print to any printer `printer:*:print`
> > > This would allow the tech to add a printer (resource permission might
> be:
> > > `printer:*:add`, or `printer:new-id:add`, but the 'guru' could not add
> > any
> > > printer, and could only configure the `office-printer1` or
> > > `basement-printer2` (as well as printing to any printer)
> > >
> > > The options are pretty endless depending on how you want to model it,
> you
> > > could have printer:<location>:<name>:<action>.
> > >
> > > Does this help?
> > >
> > >
> > > On Wed, Jan 24, 2018 at 3:05 AM, Claude Warren <claude@xenei.com>
> wrote:
> > >
> > > > lines 182 - 189 do not cover this issue.
> > > >
> > > > In those lines the pattern can be simplified to "X:*" => X.Y"
> > > >
> > > > I am asking about "X:*" => X
> > > >
> > > > does a longer (more segments) pattern match a shorter wildcard free
> > > > pattern?
> > > >
> > > > I think it should not.
> > > >
> > > > Suppose you have 2 employees.  1 is the printing services tech and
> the
> > > > other is the printer guru.  Printing services tech can do anything
> with
> > > > printers, so you give him "printer" permissions.  guru is a wiz at
> > > > configuring printers but you don't want him messing with the print
> > > servers
> > > > or installing/removing printers.  So you give guru "printer:*"
> > > permissions.
> > > >
> > > > Now you have an operation to add a printer so you protect that by
> > > requiring
> > > > "printer" permission.
> > > >
> > > > As expected tech will be able to execute add printer, but so will
> guru.
> > > >
> > > > The only way, under the current implementation, to limit guru to
> > printers
> > > > only is to list each printer in his permissions list.  This is
> > unexpected
> > > > and unnecessary if the wildcard checking is performed differently.
> > > >
> > > > The change I see is that: until a wild card is encountered: once the
> > > > separator character ":" is found the matching pattern must have a
> > colon.
> > > >
> > > > so that "printer.*" not => printer
> > > >
> > > >
> > > > Claude
> > > >
> > > > On Tue, Jan 23, 2018 at 10:21 PM, Brian Demers <
> brian.demers@gmail.com
> > >
> > > > wrote:
> > > >
> > > > > Claude,
> > > > > I think I see what you are saying, but it looks like this is what
> > this
> > > > test
> > > > > does:
> > > > > https://github.com/apache/shiro/blob/master/core/src/
> > > > > test/java/org/apache/shiro/authz/permission/
> > > WildcardPermissionTest.java#
> > > > > L182-L189
> > > > >
> > > > > Maybe this is something a tweak to the doc can fix?
> > > > >
> > > > > On Tue, Jan 23, 2018 at 5:13 PM, Claude Warren <claude@xenei.com>
> > > wrote:
> > > > >
> > > > > > Brian,
> > > > > >
> > > > > > thanks for the response, but the document cited does not cover
> the
> > > > > question
> > > > > > nor do the test cases.
> > > > > >
> > > > > >
> > > > > > If we take the first two examples in the missing parts section:
> > > > > >
> > > > > > it clearly states that  ( assume the symbol => means implies)
> > > > > >
> > > > > > printer:print => printer:print:*
> > > > > >
> > > > > >
> > > > > > but says noting about
> > > > > >
> > > > > > printer:print:* => printer:print
> > > > > >
> > > > > > I think that this statement is false.  Logically the addition
of
> > the
> > > > ":*"
> > > > > > should indicate that permissions are grated to things under
> > > > > "printer:print"
> > > > > > but not to "printer:print" itself.
> > > > > >
> > > > > >
> > > > > > I hope this makes sense.
> > > > > >
> > > > > > Claude
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > On Tue, Jan 23, 2018 at 2:48 PM, Brian Demers <
> > > brian.demers@gmail.com>
> > > > > > wrote:
> > > > > >
> > > > > > > Hey Claude, I thought someone responded to this already
(sorry)
> > > > > > >
> > > > > > > To me, this seems in line with the doc:
> > > > > > > https://shiro.apache.org/permissions.html#missing-parts
> > > > > > > At first glance, there are similar test cases in
> > > > > `WildcardPermissionTest
> > > > > > > <https://github.com/apache/shiro/blob/master/core/src/
> > > > > > > test/java/org/apache/shiro/authz/permission/
> > > > > WildcardPermissionTest.java#
> > > > > > > L143>
> > > > > > > `
> > > > > > >
> > > > > > > Does that help clear things up? If not maybe a PR to that
test
> > > class
> > > > > > would
> > > > > > > help clarify the point?
> > > > > > >
> > > > > > > Thanks!
> > > > > > > -Brian
> > > > > > >
> > > > > > >
> > > > > > > On Tue, Jan 23, 2018 at 5:36 AM, claude.warren@wipro.com
<
> > > > > > > claude.warren@wipro.com> wrote:
> > > > > > >
> > > > > > > > Not seeing any discussion of this and seeing no tests
in the
> > test
> > > > > cases
> > > > > > > > that perform any tests of this issue.  I will open
a defect
> and
> > > > work
> > > > > on
> > > > > > > > that.
> > > > > > > >
> > > > > > > >
> > > > > > > > Claude
> > > > > > > >
> > > > > > > > ________________________________
> > > > > > > > From: Claude Warren (Product Engineering Service)
> > > > > > > > Sent: Friday, January 12, 2018 12:52:52 PM
> > > > > > > > To: dev@shiro.apache.org
> > > > > > > > Subject: Defect in WildcardPermission evaluation?
> > > > > > > >
> > > > > > > >
> > > > > > > > Currently the WildcardPermission.implies() method
contains
> the
> > > > > > following
> > > > > > > > code snippet and comment.
> > > > > > > >
> > > > > > > >
> > > > > > > > // If this permission has less parts than the other
> permission,
> > > > > > > everything
> > > > > > > > after the number of parts contained
> > > > > > > > // in this permission is automatically implied, so
return
> true
> > > > > > > >  ....
> > > > > > > >
> > > > > > > >
> > > > > > > > // If this permission has more parts than the other
parts,
> only
> > > > imply
> > > > > > it
> > > > > > > > if all of the other parts are wildcards
> > > > > > > >         for (; i < getParts().size(); i++) {
> > > > > > > >             Set<String> part = getParts().get(i);
> > > > > > > >             if (!part.contains(WILDCARD_TOKEN)) {
> > > > > > > >                 return false;
> > > > > > > >             }
> > > > > > > >         }
> > > > > > > >
> > > > > > > >
> > > > > > > > This means that If you have (User perms in first col,
testing
> > > > against
> > > > > > > > across columns)
> > > > > > > > {noformat}
> > > > > > > >
> > > > > > > >
> > > > > > > >         A       A:*     A:B     A:B:*   A:B:C   A:B:C:*
> > > > > > > > A       t       t       t       t       t       t
> > > > > > > > A:*     T
> > > > > > > >         t       t       t       t       t
> > > > > > > > A:B     f       f       t       t       t       t
> > > > > > > > A:B:*   f       f       T       t       t       t
> > > > > > > > A:B:C   f       f       f       f       t       t
> > > > > > > > A:B:C:* f       f       f       f       T       t
> > > > > > > >
> > > > > > > > {noformat}
> > > > > > > >
> > > > > > > > I think the issues are where the upper case  "T"s
are.   I
> > > believe
> > > > > that
> > > > > > > > those should be "F"
> > > > > > > >
> > > > > > > > The logic being that once a separator (:) is presented
it
> > should
> > > no
> > > > > > > longer
> > > > > > > > match anything shorter than that.
> > > > > > > >
> > > > > > > > Thoughts?
> > > > > > > > Claude
> > > > > > > >
> > > > > > > >
> > > > > > > > The information contained in this electronic message
and any
> > > > > > attachments
> > > > > > > > to this message are intended for the exclusive use
of the
> > > > > addressee(s)
> > > > > > > and
> > > > > > > > may contain proprietary, confidential or privileged
> > information.
> > > If
> > > > > you
> > > > > > > are
> > > > > > > > not the intended recipient, you should not disseminate,
> > > distribute
> > > > or
> > > > > > > copy
> > > > > > > > this e-mail. Please notify the sender immediately
and destroy
> > all
> > > > > > copies
> > > > > > > of
> > > > > > > > this message and any attachments. WARNING: Computer
viruses
> can
> > > be
> > > > > > > > transmitted via email. The recipient should check
this email
> > and
> > > > any
> > > > > > > > attachments for the presence of viruses. The company
accepts
> no
> > > > > > liability
> > > > > > > > for any damage caused by any virus transmitted by
this email.
> > > > > > > > www.wipro.com
> > > > > > > >
> > > > > > > > ____________________________________________________________
> > > > > __________
> > > > > > > > This email has been scanned by the Symantec Email
> > Security.cloud
> > > > > > service.
> > > > > > > > For more information please visit
> http://www.symanteccloud.com
> > > > > > > > ____________________________________________________________
> > > > > __________
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > I like: Like Like - The likeliest place on the web
> > > > > > <http://like-like.xenei.com>
> > > > > > LinkedIn: http://www.linkedin.com/in/claudewarren
> > > > > >
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > I like: Like Like - The likeliest place on the web
> > > > <http://like-like.xenei.com>
> > > > LinkedIn: http://www.linkedin.com/in/claudewarren
> > > >
> > >
> >
> >
> >
> > --
> > I like: Like Like - The likeliest place on the web
> > <http://like-like.xenei.com>
> > LinkedIn: http://www.linkedin.com/in/claudewarren
> >
>



-- 
I like: Like Like - The likeliest place on the web
<http://like-like.xenei.com>
LinkedIn: http://www.linkedin.com/in/claudewarren

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message