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

Profiles #1453

Merged
merged 25 commits into from Sep 11, 2017
Merged

Profiles #1453

merged 25 commits into from Sep 11, 2017

Conversation

liliakai
Copy link
Contributor

@liliakai liliakai commented Sep 8, 2017

Adds support for downloading, decrypting, and displaying profile names and avatars, syncing profile keys from the master device, sending and receiving profile keys, and inferring profile whitelisting.

When AES-GCM authentication fails, webcrypto returns a very generic error. The
same error is thrown for invalid length inputs, but our earlier checks in
decryptProfile should rule out those failure modes and leave us safe to assume
that we either had bad ciphertext or the wrong key.

// FREEBIE
Use different ones for staging and production

// FREEBIE
Synced contact avatars will still override profile avatars.
Only if we don't have a synced contact name.

// FREEBIE
@liliakai liliakai force-pushed the profiles-electron branch 2 times, most recently from 68c7b85 to 46eb012 Compare September 8, 2017 14:30
For standalone accounts, generate a random profile key.

// FREEBIE
Android will use a contact sync message to sync a profile key from Android
clients who have just upgraded and generated their profile key. Normally we
should receive this data in a provisioning message.

// FREEBIE
Requires that `profileSharing` be set on the conversation.

// FREEBIE
When receiving a message with this flag, don't init a message record, just
process the profile key and move on.

// FREEBIE
Copy link
Contributor

@scottnonnenberg scottnonnenberg left a comment

Choose a reason for hiding this comment

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

Pretty exciting stuff! Really cool to see profiles show up in the app. :0)

There are a couple changes still necessary to fine-tune the experience: managing the cache, pulling down new profiles when we see them, and error handling.

js/background.js Outdated
console.log('Got sync message with our own profile key');
storage.put('profileKey', profileKey);
}
return ev.confirm();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should eliminate the return here, since it will prevent the creation of a local Conversation for yourself. And I'm pretty sure we rely on it for conversations with yourself, and display of the members in a group.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if (data.message.flags & textsecure.protobuf.DataMessage.Flags.PROFILE_KEY_UPDATE) {
var profileKey = data.message.profileKey.toArrayBuffer();
return ConversationController.getOrCreateAndWait(data.source, 'private').then(function(sender) {
return sender.setProfileKey(profileKey).then(ev.confirm);
Copy link
Contributor

Choose a reason for hiding this comment

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

Calling ev.confirm() at the end of this save is probably too aggressive. I think we want to wait until all message processing is complete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should avoid initializing a Whisper.Message that we don't want to save. Neither ios or android are saving message records for profile key updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, handleDataMessage will have side effects like bumping the conversation to the top of the list, triggering a notification, etc... which would be weird when no new message bubble is being added.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, again I'm mistaken about how flags are used. The design of the data structure suggests that any message can have multiple flags, and be a normal message as well. But you're telling me that a DataMessage is one of: 1) a normal message 2) a empty message with any single flag

if (data.message.flags & textsecure.protobuf.DataMessage.Flags.PROFILE_KEY_UPDATE) {
var id = data.message.group ? data.message.group.id : data.destination;
return ConversationController.getOrCreateAndWait(id, 'private').then(function(convo) {
return convo.save({profileSharing: true}).then(ev.confirm);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Wait until the confirm() call at the end of handleDataMessage()

c.setProfileAvatar(profile.avatar)
]).then(function() {
// success
c.save();
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably makes sense to surround this with a new Promise() to capture any errors generated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -765,6 +765,9 @@ MessageReceiver.prototype.extend({
} else if (decrypted.flags & textsecure.protobuf.DataMessage.Flags.EXPIRATION_TIMER_UPDATE ) {
decrypted.body = null;
decrypted.attachments = [];
} else if (decrypted.flags & textsecure.protobuf.DataMessage.Flags.PROFILE_KEY_UPDATE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised that our flags handling uses else if given that our flags bitpacking technique can result in multiple flags. Are we really sure that we'll only get one of these at a time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In practice this has always been the case, yes.

@@ -0,0 +1,54 @@
describe('encrypting and decrypting profile data', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we perhaps name this file profile_crypto_test.js if it's specific to profiles?

@@ -72,6 +72,7 @@
this.messageCollection.on('send-error', this.onMessageError, this);

this.on('change:avatar', this.updateAvatarUrl);
this.on('change:profileAvatar', this.updateAvatarUrl);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need a change:profileKey handler here which will go fetch the user's profile, thereby pulling new profile information. I just tried sharing my profile key via a message, and because the recipient already had the conversation open, nothing changed. Left the conversation and came back, still no change, because we throttle profile get. Had to restart to see the profile avatar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

scottnonnenberg added a commit that referenced this pull request Sep 8, 2017
When clicking on a notification in Windows when a window had been stuck
to one side of the screen using Snap, the window would be repositioned.

Fixes #1453

FREEBIE
Don't return early if we get a contact sync for our own number

// FREEBIE
Copy link
Contributor

@scottnonnenberg scottnonnenberg left a comment

Choose a reason for hiding this comment

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

Looking good!

scottnonnenberg added a commit that referenced this pull request Sep 11, 2017
When clicking on a notification in Windows when a window had been stuck
to one side of the screen using Snap, the window would be repositioned.

Fixes #1453

FREEBIE
@scottnonnenberg scottnonnenberg merged commit 33fa666 into electron Sep 11, 2017
@scottnonnenberg scottnonnenberg deleted the profiles-electron branch September 11, 2017 16:50
scottnonnenberg added a commit that referenced this pull request Sep 11, 2017
Contact profiles support - display avatars and names (#1453)

Audio notes can now be recorded on Windows (#1456)

When window is snap-positioned on windows, no longer automatically
re-position window (#1455)

Ensure that our sound setting applies for all notification types (#1445)

Fix loading screen hang with application is unlinked (#1440)

Fix error dialog on initial load (#1440)

Based on https://github.com/WhisperSystems/Signal-Desktop/releases/tag/v0.43.3

FREEBIE
scottnonnenberg added a commit that referenced this pull request Sep 15, 2017
When clicking on a notification in Windows when a window had been stuck
to one side of the screen using Snap, the window would be repositioned.

Fixes #1453

FREEBIE
scottnonnenberg pushed a commit that referenced this pull request Sep 15, 2017
* Add AES-GCM encryption for profiles

With tests.

* Add profileKey to DataMessage protobuf

// FREEBIE

* Decrypt and save profile names

// FREEBIE

* Save incoming profile keys

* Move pad/unpad to crypto module

// FREEBIE

* Support fetching avatars from the cdn

// FREEBIE

* Translate failed authentication errors

When AES-GCM authentication fails, webcrypto returns a very generic error. The
same error is thrown for invalid length inputs, but our earlier checks in
decryptProfile should rule out those failure modes and leave us safe to assume
that we either had bad ciphertext or the wrong key.

// FREEBIE

* Handle profile avatars (wip) and log decrypt errors

// FREEBIE

* Display profile avatars

Synced contact avatars will still override profile avatars.

* Display profile names in convo list

Only if we don't have a synced contact name.

// FREEBIE

* Make cdn url an environment config

Use different ones for staging and production

// FREEBIE

* Display profile name in conversation header

* Display profile name in group messages

* Update conversation header if profile avatar changes

// FREEBIE

* Style profile names small with ~

* Save profileKeys from contact sync messages

// FREEBIE

* Save profile keys from provisioning messages

For standalone accounts, generate a random profile key.

// FREEBIE

* Special case for one-time sync of our profile key

Android will use a contact sync message to sync a profile key from Android
clients who have just upgraded and generated their profile key. Normally we
should receive this data in a provisioning message.

// FREEBIE

* Infer profile sharing from synced data messages

* Populate profile keys on outgoing messages

Requires that `profileSharing` be set on the conversation.

// FREEBIE

* Support for the profile key update flag

When receiving a message with this flag, don't init a message record, just
process the profile key and move on.

// FREEBIE

* Display profile names in group member list

* Refresh contact's profile on profile key changes

// FREEBIE

* Catch errors on profile save

// FREEBIE

* Save our own synced contact info

Don't return early if we get a contact sync for our own number

// FREEBIE
scottnonnenberg added a commit that referenced this pull request Sep 15, 2017
Contact profiles support - display avatars and names (#1453)

Audio notes can now be recorded on Windows (#1456)

When window is snap-positioned on windows, no longer automatically
re-position window (#1455)

Ensure that our sound setting applies for all notification types (#1445)

Fix loading screen hang with application is unlinked (#1440)

Fix error dialog on initial load (#1440)

Based on https://github.com/WhisperSystems/Signal-Desktop/releases/tag/v0.43.3

FREEBIE
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

2 participants