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

Javascript tutorial for Encryption API #67

Merged
merged 4 commits into from
Sep 13, 2019

Conversation

example.html Outdated
}

Ably.Realtime.Crypto.generateRandomKey(function (err, key) {
var cipherParams = Ably.Realtime.Crypto.getDefaultParams({key: key});
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why you're using the long way here (getDefaultParams when you used the short way ({ cipher: { key: key } }) in the nodejs tutorial? We should use the short way in both.

Copy link
Author

Choose a reason for hiding this comment

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

Ah - sorry for the inconsistency; my intention was to see which one would be preferable in a tutorial.
One has the benefit of being less code and succinct, while other let's the reader know about Ably.Realtime.Crypto.getDefaultParams()

I'll make the code consistent by keeping the short version

example.html Outdated

function createChannel(channelName, channelOpts) {
var realtime = new Ably.Realtime(apiKey);
return realtime.channels.get(channelName, channelOpts);
Copy link
Member

Choose a reason for hiding this comment

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

I see why you've done it this way (so the publisher and subscriber get different Realtime instances so it's more of a real test), but still, I think this could well mislead someone into believing they should be creating a new Realtime instance for each channel they use (especially given the method name), which would be a big problem. I think the way you did it in the nodejs tutorial is better (just creating one Realtime instance at the top and using that).

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense. I was on the fence doing it this way too.
I'll update this.

example.html Outdated
</body>

<!-- Include the latest Ably Library -->
<script src="https://cdn.ably.io/lib/ably.min.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

That is not the latest ably library, that's 0.8. The latest is at ably.min-1.js.
(Did you copy this from another tutorial? If so, please let us know so @tomczoink or @Srushtika can fix it)

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Thanks - @tomczoink could you make sure that tutorial is updated, and check for any others that use ably.min.js or ably.js?

main.js Outdated
resultArea.value += ('Presence Subscribers: ' + occupancyMetrics.presenceSubscribers + ' \n')
resultArea.scrollTop = resultArea.scrollHeight;
}
})
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to include this file? This isn't a tutorial on channel occupancy events

Copy link
Author

Choose a reason for hiding this comment

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

no I did not. I'll remove this.

@vivekmore vivekmore force-pushed the encryption-javascript branch 2 times, most recently from 4022523 to cb04240 Compare August 22, 2019 12:07
@vivekmore
Copy link
Author

vivekmore commented Aug 22, 2019

I believe I have addressed comments;, Kindly let me know if I missed anything.

subscriber.html Outdated
key: [
1554178354, 192577227, 772660949, -2115679804,
1890741037, 2046217291, 703998164, -644134120
]
Copy link
Member

Choose a reason for hiding this comment

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

The key here should be either an ArrayBuffer (e.g. created using new Uint32Array([1554178354, ...]).buffer) or a base64 string (e.g. "W29iamVjdCBBcnJheUJ1ZmZlcl0="). An array of ints may happen to work at the moment due to how ably-js browserside works with buffers right now, but isn't a supported type and isn't guaranteed to work in future.

For a tutorial probably best to use a base64 string, it's easy and works in all browsers.

Copy link
Author

Choose a reason for hiding this comment

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

I've updated the code to use a base64 string as the cipher key

@vivekmore vivekmore force-pushed the encryption-javascript branch 2 times, most recently from 9490df4 to be96da4 Compare September 9, 2019 21:08
@Srushtika Srushtika self-requested a review September 12, 2019 11:20
Copy link
Member

@Srushtika Srushtika left a comment

Choose a reason for hiding this comment

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

Two small comments:

  1. Could you please add a comment in both files to say that you can generate the key with the utility function provided by ably, but we have embedded one explicitly here for simplicity?
  2. Retain the README file and add this project specific info please + mention that other tutorials are available in the same repo but in different branches.

Apart from that, it looks good to be merged in, thanks!

@vivekmore
Copy link
Author

Will do.

Just for my understanding how would one use the Ably's generateRandomKey() utility in a real world application (and share a key between publisher and subscriber)

@Srushtika
Copy link
Member

Srushtika commented Sep 12, 2019

Will do.

Just for my understanding how would one use the Ably's generateRandomKey() utility in a real world application (and share a key between publisher and subscriber)

We can't say, it's really upto them (our customers). They could set up any kind of secure system to share their keys internally.

@vivekmore
Copy link
Author

understood

@vivekmore
Copy link
Author

  • Could you please add a comment in both files to say that you can generate the key with the utility function provided by ably, but we have embedded one explicitly here for simplicity?
  • Retain the README file and add this project specific info please + mention that other tutorials are available in the same repo but in different branches.

I've addressed the review comments in javascript PR.
Please let me know and I'll update the nodejs PR right away.

publisher.html Outdated

/* Create options object with cipher(encryption) configuration */
// Note: A sample cipher key is provided here for simplicity.
// Alternatively, one could use Ably.Realtime.Crypto.generateRandomKey()
Copy link
Member

Choose a reason for hiding this comment

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

// Note: A sample cipher key is provided here for simplicity.
// Alternatively, one could use Ably.Realtime.Crypto.generateRandomKey()
=>
/* Note: A sample cipher key is provided here for simplicity. Alternatively, one could use the Ably.Realtime.Crypto.generateRandomKey() utility method to generate a valid key */

README.md Outdated
@@ -6,6 +6,8 @@

This repository contains the working code for many of the [Ably tutorials](https://www.ably.io/tutorials).

This branch contains the working code for the [Encryption Tutorial](https://www.ably.io/tutorials/encryption).
Copy link
Member

Choose a reason for hiding this comment

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

This branch contains a sample app for implementing encryption in an Ably app. There is also a detailed step-by-step tutorial to follow along.

publisher.html Outdated
var channelName = 'encrypted:messages';

/* Create options object with cipher(encryption) configuration */
/* Note: A sample cipher key is provided here for simplicity.
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this isn't updated.

Copy link
Author

Choose a reason for hiding this comment

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

updated now, (also updated it in subscriber.html)

@Srushtika Srushtika merged commit 718ae34 into ably:encryption-javascript Sep 13, 2019
@vivekmore vivekmore deleted the encryption-javascript branch September 13, 2019 11:31
@vivekmore
Copy link
Author

Kindly create tags as below

encryption-javascript-step2
encryption-javascript-step3
encryption-javascript-step4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants