fix: double quotes will be trimmed from the search token #8035
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
services/graph/pkg/identity/odata.go
Outdated
| searchValue := req.Search.Tree.Token.Value | ||
| if strings.HasPrefix(searchValue, "\"") && strings.HasSuffix(searchValue, "\"") { | ||
| searchValue = strings.Trim(searchValue, "\"") | ||
| } |
There was a problem hiding this comment.
Nitpick: I guess in this case you can just call strings.Trim unconditionally. There is no need for the HasPrefix and HasSuffix calls. Or am I missing something?
The OData parser will already choke on strings that only contain a single double quote.
There was a problem hiding this comment.
The OData parser will already choke on strings that only contain a single double quote.
I thought it was still possible. I'll remove the condition.
There was a problem hiding this comment.
How are we dealing the same with single quotes?
There was a problem hiding this comment.
I think single quotes have a completely different meaning here and are handled by the OData parser. In this case, as far as I understand the odata specs, using single quotes here won't even work. (our parsers doesn't allow it here at least https://github.com/CiscoM31/godata)
There was a problem hiding this comment.
does a user (the one who is on the keyboard...) know this difference...?
There was a problem hiding this comment.
The user shouldn't need to worry about quoting at all.
The UI/client which is constructing the query should know the difference, yes.
There was a problem hiding this comment.
Hm, actually I think I overlooked something. We still have minimum search length check in https://github.com/owncloud/ocis/blob/master/services/graph/pkg/service/v0/users.go#L226.
Where check of the length of the possibly quoted string. I think we need to adjust that. Otherwise it would be simple to circumvent that limit (or at least reduce it by 2) by just adding quotes.
jvillafanez
force-pushed
the
fix/graph_double_quote_handling
branch
from
cb78ef4 to
70f2e75
Compare
|
Refactored the code a bit. The tests should pass now. |
|
Checked. Looks good now. |
|
I would love it to see a unit test for that 😍 |
They are part of #8050 now. |
Description
Search terms from the graph API can be enclosed with double quotes. The double quotes are intended to prevent the interpretation of some tokens from the parser. However, the parser doesn't remove the double quotes.
This PR will remove those double quotes.
As a practical example, searching by email (such as
[email protected]) can give problems because the@char is interpreted as a different token (likely as a parameter alias, although I'm not fully sure). With this PR, you can search it by enclosing the email in double quotes, like$search="[email protected]"(note that url-encoding might be needed)Related Issue
#7990
Motivation and Context
Allow searching users by email
How Has This Been Tested?
Manually tested via API, running something like
curl -vk -ueinstein:relativity https://localhost:9200/graph/v1.0/users?%24search=%22marie%40example.org%22Screenshots (if appropriate):
Types of changes
Checklist: