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

Use federatedCloudId instead of email for user identification between servers #102

Closed
wants to merge 3 commits into from

Conversation

MahdiBaghbani
Copy link
Collaborator

@MahdiBaghbani MahdiBaghbani commented Aug 31, 2024

This PR changes 2 things:

  1. Renames userID to userId to be consistent with camelCase across the whole specification eg. providerId
  2. Adds a field to the invite-flow called federatedCloudId.
  3. Removes recipientProvider since it is redundant and is repeated on federatedCloudId.

Reason behind the new field
Previously OCM used email as a way to identify remote users.

This is essential when a party tries to send a share or notification to another party because it is going to find the domain name of the recipient from their ID (which currently is set to email, look at Reva and ScienceMesh)

This becomes problematic when the user's email name or domain is different than the user's username or provider domain. for example:

  1. Alice is on provider apollo.space with username alice1998 and has email [email protected] -> OK
  2. Bob is on provider nasa.space with username bob and has email [email protected] -> Problem

Vendors can overcome this problem by not providing the user's email and just returning username@provider-domain in the email field.

I think it is a good idea to make a distinction between email and federatedCloudId.

@glpatcern glpatcern self-requested a review September 3, 2024 07:20
Copy link
Member

@glpatcern glpatcern left a comment

Choose a reason for hiding this comment

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

This is an interesting proposal as the net sum is to add the username of a remote user as known by the remote EFSS, on top of the email identifier that indeed may not be related to the remote EFSS' domain.

I think some comment from @labkode as originally involved in the conception, as well as @michielbdejong, is more than welcome prior to merging this enhancement.

Copy link
Contributor

@michielbdejong michielbdejong left a comment

Choose a reason for hiding this comment

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

Let's not merge this until we understand the relationship between the proposed federatedCloudId and the existing userId

@MahdiBaghbani
Copy link
Collaborator Author

It seems, that the invite is being accepted by [email protected]

As implemented in ScienceMesh

So no need for the new filed in this PR.

@MahdiBaghbani
Copy link
Collaborator Author

MahdiBaghbani commented Sep 4, 2024

I think this https:// (look at Test Suite) bug is originated from here.

The "recipientProvider": "https://receiver.org/" has the protocol schema instead of just the domain.

Proposal:
Merge both userId and recipientProvider into cloudId = userId@recipientProviderDomain

@michielbdejong
Copy link
Contributor

Proposal:
Merge both userId and recipientProvider into cloudId = userId@recipientProviderDomain

That would be a breaking spec change with costs to implementers. What if instead we just improve how the existing userId and recipientProvider fields are documented? In particular, make it clear that recipientProvider should be an FQDN and not a URL?

@MahdiBaghbani
Copy link
Collaborator Author

MahdiBaghbani commented Sep 4, 2024

That's right, that makes sense since it is already inside Reva and ScienceMesh, and they don't check if it is URL or FQDN.

PS. For future reference, this is the Reva code for accepting an invite.

@michielbdejong
Copy link
Contributor

So do we want to close this PR without merging then?

@MahdiBaghbani
Copy link
Collaborator Author

Yes!

we just improve how the existing userId and recipientProvider fields are documented.

that would be in another PR.

michielbdejong added a commit that referenced this pull request Sep 4, 2024
glpatcern pushed a commit that referenced this pull request Sep 6, 2024
@MahdiBaghbani MahdiBaghbani deleted the email-field branch September 29, 2024 16:17
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.

3 participants