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

Fix ocmd tutorial #2285

Closed
wants to merge 2 commits into from
Closed

Fix ocmd tutorial #2285

wants to merge 2 commits into from

Conversation

michielbdejong
Copy link
Contributor

@michielbdejong michielbdejong commented Nov 16, 2021

Reverts part of @butonic's commit here
Reverts part of @butonic's commit here
Fixes #2284

@michielbdejong
Copy link
Contributor Author

@butonic this may not be the desired fix - maybe it's better to leave users.demo.json as you changed it (including the 'https://') and then edit to providers.demo.json to match it?

Copy link
Contributor

@butonic butonic left a comment

Choose a reason for hiding this comment

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

Just commenting why I thought these were wrong. I don't want people to assume the examples are correct. Is this example config used in any tests?

@@ -87,6 +87,8 @@ driver = "json"
users = "users.demo.json"

[http]
enabled_services = ["ocmd"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm ok, I was under the impression that the ocmd service gets picked up by just having [http.services.ocmd] in the config...

Same for the providerauthorizer Middleware.

@@ -2,7 +2,7 @@
{
"id": {
"opaque_id": "4c510ada-c86b-4815-8820-42cdf82c3d51",
"idp": "https://cernbox.cern.ch",
"idp": "cernbox.cern.ch",
Copy link
Contributor

Choose a reason for hiding this comment

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

The id really should be a url. It may be compared to the oidc provider, which always is a url. Including the protocol.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By 'id' you mean the "idp" field in a UserId object? I agree that the cs3apis documentation is undecisive and seems to tend towards making it a URL, but reva's current providerauthorizer interceptor code clearly requires it to be a domain.

I'll propose a non-breaking change in the providerauthorizer code so that it strips the protocol (if present) from the user's idp string before comparing it to the provider's domain string.

I'll also create a PR on the c3apis repo to make this clearer.

michielbdejong added a commit to cs3org/cs3apis that referenced this pull request Nov 17, 2021
@michielbdejong
Copy link
Contributor Author

Closing this, let's discuss the desired string format in cs3org/cs3apis#159 and then we can adapt the code to that decision.

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.

provider not authorized when following https://reva.link/docs/tutorials/share-tutorial/
2 participants