Skip to content
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

Merged
merged 4 commits into from
Oct 25, 2021

Conversation

gmgigi96
Copy link
Member

No description provided.

@update-docs
Copy link

update-docs bot commented Oct 22, 2021

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.

@gmgigi96 gmgigi96 changed the title [WIP] Filter root paths according to user agent Filter root paths according to user agent Oct 22, 2021
@gmgigi96 gmgigi96 marked this pull request as ready for review October 22, 2021 15:47
@gmgigi96 gmgigi96 requested review from ishank011, labkode and a team as code owners October 22, 2021 15:47
@labkode labkode merged commit 1be4a24 into cs3org:master Oct 25, 2021
@refs
Copy link
Member

refs commented Oct 25, 2021

👍 nice changes!

Copy link
Contributor

@ishank011 ishank011 left a 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 {
Copy link
Contributor

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

Copy link
Member

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

Copy link
Contributor

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

Copy link
Member

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.

gmgigi96 added a commit to cernbox/reva that referenced this pull request Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants