-
Notifications
You must be signed in to change notification settings - Fork 12
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
Invitation workflow for OCM #41
Invitation workflow for OCM #41
Conversation
e6bc7e6
to
dcdc02e
Compare
ocm: enhance invitation flow
@ishank011 and @labkode Do you see a reason for using both "invite" and "invitation" in the API description? If we use both I think we should be clear that they either mean the same (no need to use both in that case), or what the difference is. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Lovisa. Looks good - just a couple of comments.
* add more services to gateway * fix ocm * clean * add expose field to InititiateFileUpload/Download * extend download/upload responses for metadata gateway * add user provider service * fix userprovier api * fix format * fix types * fix is in logic for userprovider * add userprovider to the gatewayg * add userId to GenerateAcccessTokenResponse * add authregistry and authprovider * move user type to user manager package * fix user namespace
Add name and email to invites/accept request
spec.yaml
Outdated
/invites/forward: | ||
post: | ||
summary: Forward an invitation to start sharing | ||
description: Forwards an invitation received by an out-of-band channel, email for example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "through an out-of-band channel", or "via" would be easier to understand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a nice improvement, I'll update.
@@ -355,6 +355,47 @@ paths: | |||
type: string | |||
schema: | |||
$ref: "#/definitions/Error" | |||
/invites/forward: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who forwards the invitation when this end-point is called, and from where to where?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC from https://www.youtube.com/watch?v=TnlUOG3lOu4, the flow would be as follows, if Alice at server A invites Bob at server B:
- Alice instructs A to send an invite to "Bob@B"
- A changes state, "invite sent from Alice to Bob@B"
- A calls B/invites/forward
- B changes state, "invite received from Alice@A to Bob"
- B notifies Bob (this is the "forwarding", right?)
- Bob instructs B to accept the invitation
- B changes state, "invite from Alice@A to Bob accepted"
- B calls A/invites/accept
- A changes state, "invite from Alice to Bob@B accepted"
If this is correct, then I think the summary and description should reflect that it is B who forwards the invite from A (or effectively from Alice@A) to Bob?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The summary "Forward an invitation to start sharing" suggests that the agent doing the forwarding (B) is also the agent who starts sharing, but I think it's more accurate to say Alice@A starts sharing.
Also, since B forwards from the inner network to the last mile end-point, "deliver" might have been a better verb than "forward".
The delivery of the notification is not the main functionality exposed by this API endpoint; rather, if and how B notifies Bob is not even the concern of A or of Alice@A. So given that there is also a state change in B, maybe "create" (as in "create an entry in your list of received invites") would have been even better than "forward" or "deliver".
The description "Forwards an invitation received through an out-of-band channel, email for example." describes how the invitation was received, but I think the intention was to describe how the invitation gets forwarded.
So maybe "Forwards a received invitation through an out-of-band channel, email for example." is clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And for the summary I would suggest "Notify the recipient of a new invite"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah wait, I ran through https://reva.link/docs/tutorials/share-tutorial/#4-2-accept-the-token again and this actually looks like accepting the invite is not a server-to-server interaction, but a client-to-server one?
Should the OCM API not only define those, or leave that to each implementer?
In the end it doesn't matter to Alice or to A how Bob and B communicate with each other, right?
What is the difference between ForwardInvite and AcceptInvite? |
I think we should only add Looking at the reva code there is a But the |
I'll see if I can create a PR to supersede this one (which is now 18 months old and hasn't been getting much movement recently). |
I think we should also define this end point as |
I see revad additionally has a http endpoint where an authenticated user can cause an invite to be generated and emailed out. |
Superseded by #54, please let's resume the discussion there. |
This PR contains an invitation workflow for OCM to discover users in remote system while preserving privacy.