-
Notifications
You must be signed in to change notification settings - Fork 113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Filter root paths according to user agent #2193
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. |
5f1230a
to
fdb0a60
Compare
6255fbb
to
893546f
Compare
👍 nice changes! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check also needs to be called when resource ID is set (the if condition just above) and in ListProviders
return true | ||
} | ||
case "desktop": | ||
if ua.Desktop { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ua.Desktop
will be true if the request comes from a desktop browser. We need to review the rules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can wait. The important bit is that we can differentiate desktop traffic from web traffic, the rest we don't care at this point.
From desktop traffic,
"Mozilla/5.0 (Linux) mirall/2.7.1 (build 2596) (cernboxcmd, centos-3.10.0-1160.36.2.el7.x86_64 ClientArchitecture: x86_64 OsArchitecture: x86_64)"
this should be flagged as Desktop: true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But Desktop will also be true for requests coming from desktop browsers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean by that, you're right.
I suggest we add "mirall" hard check in our rules like we do for basic auth support.
The rule should check if the user agent contains "mirall" keyword, this will match all sync client I think.
No description provided.