Roundup Tracker - Issues

Issue 2551051

classification
Title: Transitive filters don't work in rest interface
Type: behavior Severity: normal
Components: API Versions: devel
process
Status: open Resolution: remind
Dependencies: Superseder:
Assigned To: schlatterbeck Nosy List: rouilj, schlatterbeck
Priority: Keywords: rest

Created on 2019-07-11 02:38 by rouilj, last changed 2019-10-25 00:09 by rouilj.

Messages
msg6574 Author: [hidden] (rouilj) Date: 2019-07-11 02:38
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?
msg6582 Author: [hidden] (schlatterbeck) Date: 2019-07-22 14:55
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
msg6675 Author: [hidden] (rouilj) Date: 2019-10-04 00:30
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?
msg6676 Author: [hidden] (rouilj) Date: 2019-10-04 00:49
Hmm, if I use user admin, it works as I expected. If I use another user, 
it responds/fails as reported.....
msg6677 Author: [hidden] (rouilj) Date: 2019-10-04 01:22
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?
msg6678 Author: [hidden] (schlatterbeck) Date: 2019-10-04 08:50
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
msg6679 Author: [hidden] (schlatterbeck) Date: 2019-10-04 08:51
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
msg6680 Author: [hidden] (rouilj) Date: 2019-10-04 11:40
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.
msg6688 Author: [hidden] (rouilj) Date: 2019-10-05 16:48
docs include searching for transitive properties on collections. 
rev5898:be8524335bfa
msg6766 Author: [hidden] (rouilj) Date: 2019-10-25 00:09
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
History
Date User Action Args
2019-10-25 00:09:33rouiljsetmessages: + msg6766
2019-10-20 21:34:08rouiljsetresolution: remind
2019-10-05 16:48:40rouiljsetmessages: + msg6688
2019-10-05 02:35:45rouiljsetcomponents: + API
2019-10-04 11:40:19rouiljsetmessages: + msg6680
2019-10-04 08:51:24schlatterbecksetmessages: + msg6679
2019-10-04 08:50:37schlatterbecksetmessages: + msg6678
2019-10-04 01:22:31rouiljsetassignee: schlatterbeck
2019-10-04 01:22:19rouiljsetmessages: + msg6677
2019-10-04 00:49:34rouiljsetmessages: + msg6676
2019-10-04 00:30:02rouiljsetstatus: new -> open
messages: + msg6675
2019-07-22 14:55:12schlatterbecksetnosy: + schlatterbeck
messages: + msg6582
2019-07-11 02:39:16rouiljsettype: behavior
versions: + devel
2019-07-11 02:38:23rouiljcreate