Search filters in roundup can be transitive.  From hyperdb.py:

  ... e.g. you can search
  for all issues where a message was added by a certain user in
  the last week with a filterspec of
  {'messages.author' : '42', 'messages.creation' : '.-1w;'}

so in theory I should be able to submit a rest call as:

  issue?messages.author=42&messages.creation=.-1w

however the current code balks at that. Stating "messages.author"
isn't a valid property of issue. I wonder if we should rework the code
in rest.py that does:

  try: 
    prop = class_obj.getprops()[key]
  except KeyError:
    raise UsageError("Field %s is not valid for %s class."%(
          key, class_name))

to something like:

  try:
    prop = class_obj.getprops()[key.split('.')[0]]
  except: ...

This should result in:

   obj_list = class_obj.filter(None, filter_props)

getting filter_props like:

  {'messages.author', [1]}

which I think is properly handled.

Two questions:

  1) Does this seem the right thing to do? Should we support
     transitive searching in the rest interface? I don't see any
     reason it's not restful. It certainly beats searching
     messages?author=42&creation=.-1w then searching
     issues?messages=123,345,678 etc. (speaking of which
      curl 'https://....net/demo/rest/data/issue/?messages=1,2'
      returns all messages for some reason, so we may have
      another bug.)

  2) Does this seem to be the right way to implement it? Given a
     filter of link.link.link.property, should we check the whole link
     chain and validate the property, or do we just rely on filter
     raising an error if the filter is nonsense?

On Thu, Jul 11, 2019 at 02:38:23AM +0000, John Rouillard wrote:
> 
> Search filters in roundup can be transitive.  From hyperdb.py:
> 
>   ... e.g. you can search
>   for all issues where a message was added by a certain user in
>   the last week with a filterspec of
>   {'messages.author' : '42', 'messages.creation' : '.-1w;'}
> 
> so in theory I should be able to submit a rest call as:
> 
>   issue?messages.author=42&messages.creation=.-1w
> 
> however the current code balks at that. Stating "messages.author"
> isn't a valid property of issue. I wonder if we should rework the code
> in rest.py that does:
> 
>   try: 
>     prop = class_obj.getprops()[key]
>   except KeyError:
>     raise UsageError("Field %s is not valid for %s class."%(
>           key, class_name))
> 
> to something like:
> 
>   try:
>     prop = class_obj.getprops()[key.split('.')[0]]
>   except: ...

Yes!
I think there is even something like get_transitive_prop that does the
right thing.

> Two questions:
> 
>   1) Does this seem the right thing to do? Should we support
>      transitive searching in the rest interface? I don't see any
>      reason it's not restful. It certainly beats searching
>      messages?author=42&creation=.-1w then searching
>      issues?messages=123,345,678 etc. (speaking of which
>       curl 'https://....net/demo/rest/data/issue/?messages=1,2'
>       returns all messages for some reason, so we may have
>       another bug.)

Yes!

>   2) Does this seem to be the right way to implement it? Given a
>      filter of link.link.link.property, should we check the whole link
>      chain and validate the property, or do we just rely on filter
>      raising an error if the filter is nonsense?
See above, I think there is already some method to check for a correct
transitive property. If it should be implemented (tm) :-)
Note that I implemented transitive properties a very long time ago :-)

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

with patch rev5872:1b91e3df3fd0 it looks like Ralf added code to fix 
the rejection of searches like:

  issue?messages.author=2

due to messages.author not being recognized. But the anonymous user
is user2. This returns 172 items, but there should be no messages 
generated by user2. I also seem to get the same response from:

     issue?messages.author=1

so I am not sure that this is actually working as intended.

Also this works (and seems to return the same 172 items)

     issue?messages.authors=2

but it shouldn't work as 'authors' is not a valid field for
the msg type.

Ralf ideas?

Hmm, if I use user admin, it works as I expected. If I use another user, 
it responds/fails as reported.....

Ok, figured it out I think.
If I add Search permission for msg class, it seems to work as
expected.

Ralf, is this ticket ok to close?

On Fri, Oct 04, 2019 at 12:30:02AM +0000, John Rouillard wrote:
> 
> John Rouillard added the comment:
> 
> with patch rev5872:1b91e3df3fd0 it looks like Ralf added code to fix 
> the rejection of searches like:
> 
>   issue?messages.author=2
> 
> due to messages.author not being recognized. But the anonymous user
> is user2. This returns 172 items, but there should be no messages 
> generated by user2. I also seem to get the same response from:
> 
>      issue?messages.author=1
> 
> so I am not sure that this is actually working as intended.

Do you have search permission for message.author, user.username,
user.id? If not the search will silently drop the attribute.

> 
> Also this works (and seems to return the same 172 items)
> 
>      issue?messages.authors=2
> 
> but it shouldn't work as 'authors' is not a valid field for
> the msg type.
Hmm, this should definitely raise an error when it encounters a field
that is not valid.

I'll look into this.

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

On Fri, Oct 04, 2019 at 01:22:19AM +0000, John Rouillard wrote:
> 
> John Rouillard added the comment:
> 
> Ok, figured it out I think.
> If I add Search permission for msg class, it seems to work as
> expected.
> 
> Ralf, is this ticket ok to close?

Hmm, the second problem when you're searching for "authors" should
probably be fixed. Does this only occur for missing search permission?

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

Hi Ralf:

In message <20191004085120.q2mb4qabwp7ujmpz@runtux.com>,
Ralf Schlatterbeck writes:
>On Fri, Oct 04, 2019 at 01:22:19AM +0000, John Rouillard wrote:
>> Ok, figured it out I think.
>> If I add Search permission for msg class, it seems to work as
>> expected.
>> 
>> Ralf, is this ticket ok to close?
>
>Hmm, the second problem when you're searching for "authors" should
>probably be fixed.

Yup. thought about that as I was heading to bed last night. It does
need to be fixed.

> Does this only occur for missing search permission?

AFAICT yes. I haven't tried setting a property restricted search
permission.  I tested with "Search" on the msg class. Not "Search" on
id, content, and author props of the msg class.

IIUC I can not use a check function in my permission definition. If I
do the search property will be dropped. If I can Search the msg class
but a View check function denies me view permission on the message
search return, I do not see the issue associated with the message in
the rest output. I claim this is fine.

Also did I miss docs on the transitive filtering in rest? I'll check
the rest.txt doc and if it's not documented, I'll add it along with
mentioning the Search perms.

docs include searching for transitive properties on collections. 
rev5898:be8524335bfa

Ralf, just a ping. This is still open to address the lack of error if the
user specifies an invalid transitive property.

I was thinking, should we return some 400 code if a user doesn't have
the ability to search by a property?

IIUC the current code ignores properties that can't be searched.  So
the search returns 200, but the search results are not for the
search that was requested.

Error code 403 says:

  403 Forbidden
    The request contained valid data and was understood by the server,
    but the server is refusing action. This may be due to the user not
    having the necessary permissions for a resource or needing an
    account of some sort, or attempting a prohibited action
    (e.g. creating a duplicate record where only one is allowed). This
    code is also typically used if the request provided authentication
    via the WWW-Authenticate header field, but the server did not
    accept that authentication. The request should not be repeated.

I am focusing on "This may be due to the user not having the necessary
permissions for a resource". The missing permission is search on the
transitive property.

For a user without search perms on messages.author, running:

    /rest/data/issue?messages.author=2

should return a 403 with:

  User does not have search permission on messages.author.

I claim this is a better response. It notifies the client that the
request can't be completed as written. It also allows the client and
the roundup admin to address a possible bug in the schema.

Validating this may also allow addressing the incorrect property
issue. So filtering by the non-existent messages.authors:

    /rest/data/issue?messages.authors=2

gets a 400 response with:

   messages.authors is not a valid property for issue.

Where 400 error is:

  400 Bad Request
    The server cannot or will not process the request due to an
    apparent client error (e.g., malformed request syntax, size too
    large, invalid request message framing, or deceptive request
    routing).  Also if we do this, I claim it should be done for
    non-transitive fields as well. E.G. if i don't have acess to the
    user address property, a request:

the request syntax is malformed due to using an incorrect property
name.

What does the underlying search code do if passed an invalid property
or a property that the user can't access? If they raise different
exceptions for no permission and bad property, the error handling code
can identify the offending property for reporting to the user.

Thoughts?

-- rouilj

Hi Ralf,

Have you looked at returning a 403 error if permissions
are not allowed for the transitive property or if the property is 
invalid?

-- rouilj

Hi John,
I've now pushed a change that returns a 403 -- but both, on non-searchable properties as well as non-existing props (e.g., your test with messages.authors (not the 's')). I think this is the correct way to do it as users not having permission on a property should not see it at all. Also I didn't want to dig deeper into the transitive search permission check in the framework which currently does not distinguish the two cases.

Let me know what you think!

Feel free to close this issue if it's ok for you.
Ralf

Hi Ralf:

I made the claim in other tickets that a user shouldn't be able to
discover the schema by probing it. I am not sure that's a valid
claim. I think different return codes for does not exist vs. doesn't
have access could be helpful.

For example a different code returned for the use of the alternate
spelling of organization (that didn't exist in the schema) for
organisation (that the users did have access to) in the roundup
issue tracker would have lead to the solving the issue more quickly.

However returning 403 in both cases is sufficient for me to close this
issue.

Thanks for the fix.

-- rouilj