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

Make tokeninfo endpoint unprotected #4715

Merged
merged 2 commits into from
Oct 4, 2022

Conversation

JammingBen
Copy link
Contributor

@JammingBen JammingBen commented Sep 29, 2022

Description

Make the tokeninfo endpoint unprotected as it is supposed to be available to the public.

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)

@JammingBen JammingBen self-assigned this Sep 29, 2022
@JammingBen
Copy link
Contributor Author

Is is necessary to add the endpoint in the proxy.yaml files as well?

  • tests/parallelDeployAcceptance/drone/ocis/proxy.yaml
  • deployments/examples/oc10_ocis_parallel/config/ocis/proxy.yaml
  • deployments/examples/ocis_individual_services/config/proxy/proxy.yaml
  • deployments/examples/ocis_hello/config/ocis/proxy.yaml

@C0rby
Copy link
Contributor

C0rby commented Sep 29, 2022

Is is necessary to add the endpoint in the proxy.yaml files as well?

* tests/parallelDeployAcceptance/drone/ocis/proxy.yaml

* deployments/examples/oc10_ocis_parallel/config/ocis/proxy.yaml

* deployments/examples/ocis_individual_services/config/proxy/proxy.yaml

* deployments/examples/ocis_hello/config/ocis/proxy.yaml

Yeah, I think you need to add them there.

Copy link
Contributor

@rhafer rhafer left a comment

Choose a reason for hiding this comment

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

@JammingBen Would you mind sharing (preferably in the commit message), why this is needed?

@rhafer
Copy link
Contributor

rhafer commented Sep 29, 2022

Sorry for adding more confusion here 🤦‍♂️ , but I took a bit of a deeper look into the code now. I think the correct fix is to add /ocs/v1.php/apps/files_sharing/api/v1/tokeninfo/unprotected and /ocs/v2.php/apps/files_sharing/api/v1/tokeninfo/unprotected to the _public_path in the authentication middleware. That way that get should be handled by the public_share_auth middleware.

You can also remove /remote.php/ocs/apps/files_sharing/api/v1/tokeninfo/unprotected from that list, which I think is just wrong.

This way you shouldn't need to touch the defaultconfig.go, neither the docker-compose examples.

@C0rby Feel free to correct me if I am wrong.

@C0rby
Copy link
Contributor

C0rby commented Sep 29, 2022

@rhafer is correct. This is the place where you need to add the lines: https://github.com/owncloud/ocis/blob/master/services/proxy/pkg/middleware/authentication.go#L24-L29

There is one already but it is prefixed with /remote.php which is wrong.

@JammingBen JammingBen force-pushed the fix-unprotected-tokeninfo-endpoint branch from 37809a9 to a323cf3 Compare September 30, 2022 07:07
@JammingBen
Copy link
Contributor Author

JammingBen commented Sep 30, 2022

I've adjusted the PR, thx for your help!

Copy link
Collaborator

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarcloud
Copy link

sonarcloud bot commented Sep 30, 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 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@rhafer rhafer merged commit e053f70 into master Oct 4, 2022
@delete-merged-branch delete-merged-branch bot deleted the fix-unprotected-tokeninfo-endpoint branch October 4, 2022 07:18
@micbar micbar mentioned this pull request Nov 25, 2022
73 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.

4 participants