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

Feature/myst-572 node rejects unregistered consumers #311

Merged
merged 9 commits into from Aug 14, 2018

Conversation

Projects
None yet
3 participants
@tadovas
Copy link
Member

commented Aug 9, 2018

Node checks consumer identity on each connect request and sends back error to consumer if its identity is unregistered

@tadovas tadovas requested review from donce and Waldz as code owners Aug 9, 2018

@tadovas tadovas requested a review from interro Aug 9, 2018

@@ -53,6 +53,7 @@ services:
--gasprice '1'
--unlock '0xd9786b6ee6caf5cd0ef88301fc40de83bfac5594'
--password node_acc_password.txt
--rpccorsdomain *

This comment has been minimized.

Copy link
@interro

interro Aug 10, 2018

Contributor

this parameter already exists

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 10, 2018

Author Member

Rebase failed me :)

tadovas added some commits Jul 18, 2018

@tadovas tadovas force-pushed the feature/MYST-572-node-rejects-unregistered-consumers branch from 8661883 to 516e044 Aug 13, 2018

@@ -70,10 +74,21 @@ func (waiter *dialogWaiter) ServeDialogs(dialogHandler communication.DialogHandl
if request.PeerID == "" {
return &responseInvalidIdentity, nil
}
registered, err := waiter.identityRegistry.IsRegistered(common.HexToAddress(request.PeerID))

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 13, 2018

Member

Idea was to have dialogWaiter thin and all business specific logic goes to DialogHandler.Handle(Dialog), which validates before handling

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 13, 2018

Author Member

Ok. let's see if I can improve that

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 13, 2018

Author Member

Moved to dialog handler. Well looks nicer

}

if !registered {
//TODO should we tell peer that id is not registered (i.e. need to report more specific error?)

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 13, 2018

Member

Just log specific error as in line 93, and we dont respond with exact errors

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 13, 2018

Author Member

Will do

err = registerIdentity(registrationData)
assert.NoError(t, err)

err = waitForCondition(func() (bool, error) {

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 13, 2018

Member

Nice +1

@@ -112,21 +112,21 @@ imports:
- leveldb/table
- leveldb/util
- name: golang.org/x/crypto
version: f027049dab0ad238e394a753dba2d14753473a04
version: de0752318171da717af4ce24d0a2e8626afaeb11

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 13, 2018

Member

Why do they change all the time?

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 13, 2018

Author Member

Because of glide update. If you add new dep - glide update must be run. If some deps uses master version - they are upgraded to latest commit at the moment

}

// Handle starts serving services in given Dialog instance
func (handler *handler) Handle(dialog communication.Dialog) error {

identity := dialog.PeerID()

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 13, 2018

Member

I would name it peerID to avoid mixing with package identity

@tadovas tadovas force-pushed the feature/MYST-572-node-rejects-unregistered-consumers branch from ed74663 to 9e31fed Aug 14, 2018

@Waldz

Waldz approved these changes Aug 14, 2018

@tadovas tadovas merged commit b1c6b68 into master Aug 14, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@tadovas tadovas deleted the feature/MYST-572-node-rejects-unregistered-consumers branch Aug 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.