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

Check for read key or public key when adding Threads #370

Merged
merged 2 commits into from
Jun 3, 2020

Conversation

carsonfarmer
Copy link
Member

This is a teeny tiny PR to skip checking for a read key when AddThread is called to add a remote thread. This is a prelim PR to initiate a discussion, as this might not be the best approach.

Here is some context:

When calling AddThread from the net API, there is currently a check to see if the ThreadKey has Read access (https://github.com/textileio/go-threads/blob/master/net/net.go#L274). This seems like a reasonable check, however, this means no log is created. In settings where the net client does have the Read key, but isn't sharing it with the net service (which is the case with our JS client), this might not be ideal, because there is no log created to push even encrypted updates to. With CreateThread this is never an issue, because that check isn't performed, which makes sense because the client is creating the thread, so it clearly have write permission, even if in practice, it hasn't sent over the key...

To add some details here, this issue is cropping because of how CreateRecord works via the JS network client. The reason this is an issue is because, to avoid sending over the keys to the remote host, under the hood the JS client actually calls AddRecord instead of CreateRecord. Now, since AddRecord doesn't ever call getOrCreateOwnLog (which makes sense), we get to a point where we don't have a log to write to... and the remote net layer throws an error.

This PR is one solution to this problem, where the log is created, regardless of if the remote host has read permission or not. The nice thing here is that, even though the log has been created, it still doesn't have the required keys to do anything with the log data, so privacy is preserved.

Signed-off-by: Carson Farmer <carson.farmer@gmail.com>
@carsonfarmer carsonfarmer added enhancement New feature or request dependencies Pull requests that update a dependency file labels Jun 3, 2020
@carsonfarmer carsonfarmer added this to the Sprint 38 milestone Jun 3, 2020
@carsonfarmer carsonfarmer requested a review from sanderpick June 3, 2020 21:30
@carsonfarmer carsonfarmer self-assigned this Jun 3, 2020
@sanderpick
Copy link
Member

Comments from slack:

Hrm, good catch. What about changing the condition to:

if args.ThreadKey.CanRead() || args.LogKey != nil {

I think that would cover the different combinations of use cases. So, in your case, you just tell it what your log key is, either public or private.

Maybe then you catch the combination: can read but only has public key, that might be a useless state.

net/net.go Outdated
if err = n.store.AddLog(id, linfo); err != nil {
return
}
var linfo thread.LogInfo
Copy link
Member

Choose a reason for hiding this comment

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

We want to avoid creating a log if it won't be used ever, since it will get pushed to the other peers.

Signed-off-by: Carson Farmer <carson.farmer@gmail.com>
@carsonfarmer
Copy link
Member Author

As per offline convo, the read key with public key only state might be a valid state in which a peer can only read (not write)... essentially an observer. So no additional checks there required, adding check for public key so as to avoid creating a log when it won't ever be used (see comment from @sanderpick)

Copy link
Member

@sanderpick sanderpick left a comment

Choose a reason for hiding this comment

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

LGTM!

@carsonfarmer carsonfarmer changed the title Skip check for read key Check for read key or public key when adding Threads Jun 3, 2020
@carsonfarmer carsonfarmer merged commit 4638cb8 into master Jun 3, 2020
@carsonfarmer carsonfarmer deleted the carson/no-read branch June 3, 2020 21:49
dgtony pushed a commit to anyproto/go-threads that referenced this pull request Aug 24, 2020
* feat(net): skip check for read key

Signed-off-by: Carson Farmer <carson.farmer@gmail.com>

* fix: add check for logkey OR read perm

Signed-off-by: Carson Farmer <carson.farmer@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants