TwilioDevEd / api-snippets

Code snippets for the Twilio API Documentation
MIT License
259 stars 468 forks source link

Sync examples #937

Closed deshartman closed 3 years ago

deshartman commented 3 years ago

Propose the below code snippet to add to the docs here: https://www.twilio.com/docs/iam/access-tokens#

I'll also submit the WagTail request once code has been merged

deshartman commented 3 years ago

Just realised I left "outgoingApplicationSid" in. Have updated it, but unsure how to update PR to the latest commit

bld010 commented 3 years ago

Thanks for adding this. It definitely seems necessary and helpful to get all of the Grants examples in one place!

I'm not super-familiar with Sync yet, but I notice that the JavaScript example that exists in docs right now has a lot of information about identity, and the identity is both attached to the token AND sent as part of the token response from the /token endpoint.

Specifically, it has these comments:


    // This is the most critical part of your backend code, as you must identify the user and (possibly)
    // challenge them with some authentication scheme. To determine the identity, you might use:
    //    * A session datum consistently identifying one anonymous visitor,
    //    * A session key identifying a logged-in user
    //    * OAuth credentials identifying a logged-in user
    //    * A random username for all comers.
    //

and we see it added to the token and sent in the response separate from the token itself:

token.identity = identity;

    // Serialize the token to a JWT string and include it in a JSON response
    response.send({
        identity: identity,
        token: token.toJwt()
    });

@anabenites Do you have any insight into whether we need to include these comments in the SyncGrant examples for this page? Is there more detailed information about how to authenticate a user's identity, or is this just a "you can do this if necessary" kind of thing?

Also, @anabenites the SyncGrant can also take an endpointId but I couldn't find any information on that. Do we need to include that in these examples? Thanks!

anabenites commented 3 years ago

hi @bld010 I asked and the endpointId is deprecated. I think there is no need to add the additional comments because the users should build what best suit their needs. Are we still using outgoingApplicationSid? In my case is throwing an error. I was also thinking that to create a SyncGrant we need to identify the Sync Service instance something like this:

const syncGrant = new SyncGrant({
  serviceSid: serviceSid,
});
deshartman commented 3 years ago

@anabenites "outgoingApplicationSid" was a mistake. If you look, I did an additional commit after the PR to remove it. My mistake

anabenites commented 3 years ago

@deshartman oh! thanks for the information, I didn't see it. A question why are you passing incomingAllow: true, // Optional: add to allow incoming calls to new SyncGrant? I saw it for Voice grants. I think Sync Access Token is more similar to Video or Conversations Access Token. Thanks,

deshartman commented 3 years ago

@anabenites I just realised, I checked in the wrong code. I had meant to delete all the voice related grants and only add the Sync ones. My mistake. Sorry about this. Have not done PRs before, so still learning.

I have completely reworked the examples, and re-uploaded. Hope my other language proposals are ok?

anabenites commented 3 years ago

Hi @deshartman Thanks for the changes 👍 . I added a couple of suggestions, once these are done, I will approve the PR. I tested the script locally and it is working. Thanks!

deshartman commented 3 years ago

@anabenites Changes done and pushed. Good suggestions!