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

Chrome export fixes: too-long names, redact group ids #1402

Merged
merged 4 commits into from Aug 30, 2017

Conversation

scottnonnenberg
Copy link
Contributor

This user reported a failing export: #1398

Ultimately we discovered that it was due to an extremely long group name. So now we limit group and contact names to 30 characters when we go to the filesystem.

But in the process we realized that the group id is not something we wanted in clear-text. So, as with phone numbers, we only log the last three characters of the group id.

@liliakai
Copy link
Contributor

These changes look fine, but tangentially, it occurs that we might need to worry about conversation name collisions...

@scottnonnenberg
Copy link
Contributor Author

@liliakai What kind of collisions are you concerned about? In our logging? On disk? Both?

We do have one technique in place already to avoid this - we include the conversation's timestamp in logging and on disk. So to have a collision the conversations would need have been modified at the same time, and share the last couple characters in their ID.

Even so, it won't be a huge problem because we operate on conversations in order and we print out log entries for the initial scheduling, so I think we'll be able to figure out what's going on.

As far as on disk, we don't use the group ID, we use the name of the conversation to make it more human readable.

@liliakai
Copy link
Contributor

I was thinking specifically about the directory name. I have a lot of groups called "test", for instance.

@scottnonnenberg
Copy link
Contributor Author

As long as those 'test' groups were last modified at different times, we'll be fine.

Though I can imagine one scenario that might make a lot of groups modified at the same time: a safety number change of a group member in every group. Maybe the right thing is to include both the human-readable name and the ID on disk? Same for individuals - both name and phone number?

@liliakai
Copy link
Contributor

I didn't see any references to timestamp, but it looks like active_at is used to prefix directory names. 👍

@liliakai
Copy link
Contributor

Hmm I don't think safety number changes modify active_at, but timestamps are somewhat flimsy as unique identifiers... Also, I don't know that human-readability should be a priority, at least not over reliability.

@scottnonnenberg
Copy link
Contributor Author

Alright, I'll merge this and assemble another PR.

@scottnonnenberg scottnonnenberg merged commit 548586b into master Aug 30, 2017
@scottnonnenberg scottnonnenberg deleted the redact-groups branch August 30, 2017 16:30
scottnonnenberg added a commit that referenced this pull request Aug 30, 2017
Properly handle update of blocked numbers sync'd from mobile
device (#1411)

Fix some bugs with migration to Electron (still behind a flag)
  - Dark Theme: Increase banner text contrast for legibility (#1415)
  - Better disambiguate conversation directory names (#1409)
  - Handle long group or contact names (#1402)
  - Redact group ids in export logging (#1402)

Better logging
  - Don't log expiration if queued task threw an error (#1412)
  - Additional error handling/logging during contact sync (#1395)

Remove unknown group messages from cache - no need to retry! (#1414)

Update a large number of strings via transifex (#1403)

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