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

[full-ci] [bump-reva] Replacement for TokenInfo endpoint #8926

Merged
merged 1 commit into from
May 28, 2024

Conversation

2403905
Copy link
Contributor

@2403905 2403905 commented Apr 23, 2024

Description

Related Issue

#8858

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Copy link

update-docs bot commented Apr 23, 2024

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.

@2403905 2403905 force-pushed the issue-8858 branch 2 times, most recently from 63aa938 to 3dc6701 Compare April 25, 2024 08:58
@2403905 2403905 changed the title POC Replacement for TokenInfo endpoint [full-ci] POC Replacement for TokenInfo endpoint Apr 25, 2024
@2403905 2403905 force-pushed the issue-8858 branch 2 times, most recently from 9190a95 to 10a35bd Compare April 25, 2024 11:01
@rhafer rhafer self-requested a review April 25, 2024 15:14
@@ -168,7 +172,7 @@ func (m OIDCAuthenticator) shouldServe(req *http.Request) bool {
// Authenticate implements the authenticator interface to authenticate requests via oidc auth.
func (m *OIDCAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) {
// there is no bearer token on the request,
if !m.shouldServe(r) || isPublicPath(r.URL.Path) {
if !m.shouldServe(r) || (isPublicPath(r.URL.Path) && !isInternal(r.URL.Path, m.RevaGatewaySelector)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, wouldn't it be much easier if we'd just remove the isPublicPath here. What is wrong with trying to verify the Bearer token on e.g. /dav/public-files if it's in the request? That way we wouldn't need the inInternal check here.

I think the same is true for the BasicAuthenticator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we remove the isPublicPath we hit the m.getClaims(token, r) for every public link which sounds bad
We can filter out the requests to public-files with the sharetoken

Copy link
Contributor

Choose a reason for hiding this comment

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

If we remove the isPublicPath we hit the m.getClaims(token, r) for every public link which sounds bad

Why is that bad? Also we can easily skip the call and error out when the Authorizatiion header does not carry a bearer token.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you mean to check the token after retrieving, it makes sense

token := strings.TrimPrefix(r.Header.Get(_headerAuthorization), _bearerPrefix)
if token == "" {
    return nil, false
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, some cases will be broken because the sequence of the authenticators has been changed. the public and oidc are swapped. The request below will be failed.

http://example.com/dav/public-files/
-H Authorization Bearer some_token
-H public-token some_tocken 

It was passed with the public_share_auth
But after we swapped authenticators, it will not reach the public_share_auth

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It means all cases that pass in a public_share_auth we should skip in an oidc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already changed to if !m.shouldServe(r) Let's see what impact this has.

@JammingBen
Copy link
Contributor

I'm not 100% sure here, but after a quick research I think we can't get rid of the browser auth popup when sending a 401 response code and the WWW-Authenticate header. So we might need to find a slightly different solution. Things that come to my mind:

  • Changing the status code to e.g. 400 (semantically not correct though).
  • Remove the WWW-Authenticate header so clients need differentiate based on the returned error message. I guess that would be okay if we don't plan to translate it.

@2403905
Copy link
Contributor Author

2403905 commented May 2, 2024

@JammingBen I removed the WWW-Authenticate Basic header
The popup authentication also made the webUITest faled
Please try without authentication popup

@JammingBen
Copy link
Contributor

@JammingBen I removed the WWW-Authenticate Basic header The popup authentication also made the webUITest faled Please try without authentication popup

Yup that works 👍 I'm simply checking the error message now: https://github.com/owncloud/web/pull/10869/files#diff-90681f8aba50079e5c32cf69d1f1c61af3dbf496f0adfd5fa45936a5647eb767R144. If @micbar or @rhafer don't have any objections I'm fine with that.

@2403905 2403905 mentioned this pull request May 2, 2024
14 tasks
@2403905 2403905 marked this pull request as ready for review May 3, 2024 08:42
@2403905 2403905 force-pushed the issue-8858 branch 2 times, most recently from 8c0af81 to 77220fd Compare May 7, 2024 08:26
@rhafer
Copy link
Contributor

rhafer commented May 7, 2024

@2403905 This doesn't build currently. I think you need to rebase you reva fork and update the vendor files.

@rhafer
Copy link
Contributor

rhafer commented May 14, 2024

@2403905 Now that the reva part is merged we can rebase and remove the reva replacement.

@2403905 2403905 changed the title [full-ci] POC Replacement for TokenInfo endpoint [full-ci] [bump-reva] Replacement for TokenInfo endpoint May 21, 2024
@2403905 2403905 enabled auto-merge May 21, 2024 09:47
@2403905 2403905 requested a review from rhafer May 21, 2024 09:47
Copy link

sonarcloud bot commented May 28, 2024

@2403905 2403905 merged commit 4fea772 into owncloud:master May 28, 2024
4 checks passed
ownclouders pushed a commit that referenced this pull request May 28, 2024
[full-ci] [bump-reva] Replacement for TokenInfo endpoint
@micbar micbar mentioned this pull request Jun 19, 2024
24 tasks
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.

None yet

3 participants