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

proxy: Avoid sorting endpoints for every request #4514

Merged
merged 4 commits into from
Sep 7, 2022

Conversation

rhafer
Copy link
Contributor

@rhafer rhafer commented Sep 5, 2022

Description

The endpoints are no longer hashed by path name in the directors map since that made iterating over the endpoints unstable. They are now stored in a slice in the order in which the are defined in the configuration.

This also includes a fix to make the archiver work for public-links (#4461 (review))

Related Issue

}
}
}

// override default director with root. If any
if ri, ok := rt.directors[pol][config.PrefixRoute][method]["/"]; ok { // try specific method
if ri := rt.directors[pol][config.PrefixRoute][method][0]; ri.endpoint == "/" { // try specific method
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still not too happy with this. I guess I need something better here for the fallback/default routes.

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about my comment about the archiver again, and actually I'm confused here. oC Web always adds the public-token to the url query (on any public link), so the public token auth provider should be used in the authentication middleware... and fails? Might just be a bug in the auth provider. The route should not be unprotected, as I thought initially. Does that sound reasonable? 🤔

@ownclouders
Copy link
Contributor

ownclouders commented Sep 5, 2022

💥 Acceptance test localApiTests-apiArchiver-ocis failed. Further test are cancelled...

@rhafer
Copy link
Contributor Author

rhafer commented Sep 6, 2022

I thought about my comment about the archiver again, and actually I'm confused here. oC Web always adds the public-token to the url query (on any public link), so the public token auth provider should be used in the authentication middleware... and fails? Might just be a bug in the auth provider. The route should not be unprotected, as I thought initially. Does that sound reasonable?

Yes. That's what I also found out while looking at the code. And it's exactly what this commit (6bbdcc2) implements. IIUC the _publicPaths slice contains a list of paths which are enabled for the public token auth provider. I.e. "public token auth" only works for paths listed there. And /archiver was simply missing. (This change does not make the archiver unprotected)

@rhafer
Copy link
Contributor Author

rhafer commented Sep 6, 2022

Hm, this seems to break the archiver somehow:-( (https://drone.owncloud.com/owncloud/ocis/15224/42/8)

@kulmann
Copy link
Member

kulmann commented Sep 6, 2022

I thought about my comment about the archiver again, and actually I'm confused here. oC Web always adds the public-token to the url query (on any public link), so the public token auth provider should be used in the authentication middleware... and fails? Might just be a bug in the auth provider. The route should not be unprotected, as I thought initially. Does that sound reasonable?

Yes. That's what I also found out while looking at the code. And it's exactly what this commit (6bbdcc2) implements. IIUC the _publicPaths slice contains a list of paths which are enabled for the public token auth provider. I.e. "public token auth" only works for paths listed there. And /archiver was simply missing. (This change does not make the archiver unprotected)

Ah, makes sense, thanks for explaining ❤️

@rhafer rhafer marked this pull request as draft September 6, 2022 07:50
@rhafer rhafer force-pushed the issue/4497 branch 4 times, most recently from 7583d78 to 26cdbf8 Compare September 7, 2022 08:08
@rhafer rhafer marked this pull request as ready for review September 7, 2022 08:09
@C0rby
Copy link
Contributor

C0rby commented Sep 7, 2022

You need to rebase from master and then the go generate steps should work again.

Allows /archiver to be used the "public-token" auth middleware. The
archiver is a bit of a special case, because it can be uses in several
ways: using 'normal' authentication (basic, oidc), using signed-urls or
using sharetokens. As only the "sharetoken" part is handled by the
"PublicShareAuth" middleware, we needed to special-case it a bit.
The endpoints are no longer hashed by path name in the directors map since
that made iterating over the endpoints unstable. They are now stored in a
slice in the order in which the are defined in the configuration.

Closes: owncloud#4497
@sonarcloud
Copy link

sonarcloud bot commented Sep 7, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

80.0% 80.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@C0rby C0rby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the whole proxy code is not ideal but this is good for now. 👍

@rhafer rhafer merged commit 8ee8842 into owncloud:master Sep 7, 2022
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.

proxy unstable routing behavior for routes that match multiple rules
4 participants