Normalize domains in mentix provider authorizer driver

[mirekys]

trafficstars

Fixes https://github.com/cs3org/reva/issues/3113

[update-docs[bot]]

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.

[redblom]

@mirekys This PR is to solve https://github.com/cs3org/reva/issues/3113 right? That issue was created because invites could not be accepted because the providers could not be authorized. Please take note of the following:

When the user idp of the sender of the forward invite is a domein rather than a url (eg. cesnet.cz vs. http://cesnet.cz) the provider authorization also fails at the receiving end (where the token has been generated). This is because from the bare domain the host name will not be resolved in the current implementation: https://github.com/cs3org/reva/blob/5eb6e708af1f7cdd4ae1f950610b0fceff96c28e/internal/http/services/ocmd/invites.go#L241-L248 It will result in an empty string (but no error!). So ProviderInfo.Domain will be an empty string. This will make authorization fail as we found out in the tests. Previously the recipientProvider parameter was actually normalized but this has been removed (https://github.com/cs3org/reva/pull/2751). See also https://github.com/cs3org/reva/issues/2288. So I guess this recipientProvider needs to be normalized once more. And solve https://github.com/cs3org/reva/issues/2288 in the process.

[mirekys]

@redblom You're right, it also needs normalization, thanks for pointing that out. If I understood the situation correctly, there is yet no exact specification on how the provider domains should look, and Reva should accept both the bare domain name & full URL, while the full URL is the preferable variant in the future? I'll update the PR accordingly

[mirekys]

I just simply removed the code and pass the provider as is, as it should be a responsibility of IsProviderAllowed endpoint to normalize value before checking

[redblom]

I just simply removed the code and pass the provider as is, as it should be a responsibility of IsProviderAllowed endpoint to normalize value before checking

Makes sense. Could IsProviderAllowed also log some meaningful err data like the actual input so it's easier to spot the root of an issue? I guess since we currently have no specification on eg. user idp domain vs. url we can not log suggestions in that regard. I mean reva/mesh configuration is rather complex so any logging hints that would guide you to where a configuration might be wrong would be really helpful.

[mirekys]

It already returns errors if domain is not found amongst mesh providers (together with input domain value) here, which then gets logged here, is this insufficient or something else needs to also be logged?

EDIT: udated other current mentix errors to provide more context

[redblom]

EDIT: udated other current mentix errors to provide more context

Yeah I like that ! (there's obviously some personal preference from me here but I like this better ;)

[redblom]

I just simply removed the code and pass the provider as is, as it should be a responsibility of IsProviderAllowed endpoint to normalize value before checking

@mirekys @labkode in the ForwardInvite request the recipientProvider that comes from (is) the context user idp still must be normalized also. This could be done here and here. But we can also let the user manager normalize the user Idp when setting it in the first place. What would be the preferred way?

[mirekys]

I just simply removed the code and pass the provider as is, as it should be a responsibility of IsProviderAllowed endpoint to normalize value before checking

@mirekys @labkode in the ForwardInvite request the recipientProvider that comes from (is) the context user idp still must be normalized also. This could be done here and here. But we can also let the user manager normalize the user Idp when setting it in the first place. What would be the preferred way?

I would vote for handling it in a single place (user manager), rather than doing it in all places idp is being used, but not sure whether it won't break something somewhere

[redblom]

~~I'm also for that. Best to do this in a separate PR though. I can take that one, in 2 weeks as I'm currently on holidays. But would be good to get an opinion from @labkode also, he's back from holidays in 2 weeks as well.~~

[redblom]

@mirekys

@mirekys @labkode in the ForwardInvite request the recipientProvider that comes from (is) the context user idp still must be normalized also. This could be done here and here. But we can also let the user manager normalize the user Idp when setting it in the first place. What would be the preferred way?

I would vote for handling it in a single place (user manager), rather than doing it in all places idp is being used, but not sure whether it won't break something somewhere

I realized, and tested, that this is not needed after all. So simply removing as you've already done in internal/http/services/ocmd/invites.go will suffice indeed !

Next however the getOCMHost method currently also fails. Here ocmHost.Host will return an empty string after the host field is parsed into a url. So we must be sure to create a proper url here once more from the input (similar to normalizeDomain) in order to be able to return the host.

[mirekys]

@antoonp I must admit that I'm getting kinda confused about acceptable formats of values of each different provider properties. As I understood from this PR and https://github.com/cs3org/cs3apis/pull/159#discussion_r766662560, we have to deal with the following provider-related properties:

• Provider Domain
• Provider service Host
• User's IDP

that can have 3 possible values:

• FQDN (without protocol)
• URI without path
• full URI

I start thinking it would be cleaner to determine the required formats on each property, and then validate/convert to a single given format, when setting the property (rather than forcing normalization from multiple formats to full URLs everywhere such property is used). So it would be safer to expect from everywhere, that if a given property is set on a provider, it is in an expected format.

I think Domain should be just FQDNs, IDP should be a full URI, and Host is defined as https://github.com/cs3org/cs3apis/blob/main/cs3/ocm/provider/v1beta1/resources.proto#L64, so it should probably also be required to always have a full valid URL there.

But I see a Host as the most specific part of a Domain name (e.g. sciencemesh for a domain of sciencemesh.cesnet.cz, or sciencemesh.cesnet.cz for a domain of cesnet.cz), I wouldn't exactly expect an URL to be there. I believe that if we need URLs in service hosts, the terminology used should be updated to not be confusing

[michielbdejong]

Oops, I used the GitHub online merge tool to merge this branch into my debug branch, and apparently it somehow allowed me to add commits to https://github.com/mirekys/reva/commits/fix-3113 - that's scary!

[michielbdejong]

Force push to someone else's GitHub account is also scary but looks like it did put the branch back in its correct state :/

[micbar]

@michielbdejong The Ci has changed on the master branch. You need to rebase this PR.

[michielbdejong]

I'm still not sure why I'm allowed to push to @mirekys's branch, but I merged origin/master into it as @micbar requested.

[michielbdejong]

While testing this code, I think there is one more change missing: In this block in the mentix driver for ocm provider authorizer, service.Host is converted from a URL to a FQDN, but looking at for instance this gocdb edit screen:

I think the "service host" is already a FQDN, and parsing it in this way changes it to "". So I think it's better to just return s.Host, nil here (and same for the json and open drivers).

[michielbdejong]

@labkode I think we should merge this PR asap