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

MYST-114 Extract Authentificator interface #121

Merged
merged 16 commits into from Jan 30, 2018

Conversation

Projects
None yet
4 participants
@Waldz
Copy link
Member

commented Jan 29, 2018

No description provided.

@Waldz Waldz force-pushed the feature/MYST-114-com-signer-tests branch from 911b045 to b206f49 Jan 29, 2018

)

// Authenticator extracts identity
type Authenticator interface {

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 29, 2018

Member

Strange name. Maybe if it extracts identity it should be called IdentityExtractor with method ExtractIdentity ? Authenticate usually means check credentials

This comment has been minimized.

Copy link
@donce

donce Jan 29, 2018

Contributor

Since it's in identity package, it could be called just identity.Extractor.

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 29, 2018

Author Member

Renamed to identity.Extractor

@@ -39,9 +39,11 @@ func NewManager(mysteriumClient server.Client, dialogEstablisherFactory DialogEs
}
}

func (manager *connectionManager) Connect(id identity.Identity, nodeKey string) error {
func (manager *connectionManager) Connect(myId identity.Identity, nodeKey string) error {

This comment has been minimized.

Copy link
@zolia

zolia Jan 29, 2018

Member

shouldn't we name abbreviations up-cased? myId -> myID?

This comment has been minimized.

Copy link
@donce

donce Jan 29, 2018

Contributor

Also, there are more occurrences of same thing in this PR.

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 29, 2018

Author Member

Renamed myId, peerId -> to ..ID

) communication.Sender {

return nats.NewSender(
peerAddress.GetConnection(),

This comment has been minimized.

Copy link
@zolia

zolia Jan 29, 2018

Member

IMHO AddressNATS structure should be renamed, you usually cannot get a connection from an address. It should be either ContextNATS, EndpointNATS or DescriptorNATS. Out of scope for current PR, but for future reference.

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 29, 2018

Author Member

maybe

nats.NewSender(connection, expectedCodec, "provider1.customer1"),
dialog.Sender,
nats.NewSender(connection, expectedCodec, "my-topic.0x28bf83df144ab7a566bc8509d1fff5d5470bd4ea"),
dialogNats.Sender,

This comment has been minimized.

Copy link
@zolia

zolia Jan 29, 2018

Member

should be dialogNATS

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 29, 2018

Author Member

Fixed

authenticator := &authenticator{}
signerId, err := authenticator.Authenticate(message, signature)
assert.NoError(t, err)
assert.Exactly(t, FromAddress("0xded9913d38bfe94845b9e21fd32f43d0240e2f34"), signerId)

This comment has been minimized.

Copy link
@zolia

zolia Jan 29, 2018

Member

I would expect here something to fail since test name is WhenMessageIsChanged

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 29, 2018

Author Member

Idea is that middleman's changed message extracts another identity with sane format.
But I improved naming

"github.com/mysterium/node/communication"
)

// NewDialogHandler constructs handler which get all incoming dialogs does starts handling them

This comment has been minimized.

Copy link
@zolia

zolia Jan 29, 2018

Member

// NewDialogHandler constructs handler which gets all incoming dialogs and starts handling them

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 29, 2018

Author Member

Fixed

func (handler *handler) Handle(dialog communication.Dialog) error {
subscribeError := dialog.Respond(handler.sessionCreateConsumer)
if subscribeError != nil {
return nil

This comment has been minimized.

Copy link
@zolia

zolia Jan 29, 2018

Member

error is swallowed here..

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 29, 2018

Author Member

Fixed

@@ -39,9 +39,11 @@ func NewManager(mysteriumClient server.Client, dialogEstablisherFactory DialogEs
}
}

func (manager *connectionManager) Connect(id identity.Identity, nodeKey string) error {
func (manager *connectionManager) Connect(myId identity.Identity, nodeKey string) error {

This comment has been minimized.

Copy link
@donce

donce Jan 29, 2018

Contributor

Also, there are more occurrences of same thing in this PR.


return dialog, nil
func (establisher *dialogEstablisher) newCodecToPeer(peerId identity.Identity) *codecSecured {

This comment has been minimized.

Copy link
@donce

donce Jan 29, 2018

Contributor

Name is confusing - newCodecForPeer ?

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 29, 2018

Author Member

Fixed

Authenticate(message []byte, signature Signature) (Identity, error)
}

// NewAuthenticator extracts identity which was used to sign message

This comment has been minimized.

Copy link
@donce

donce Jan 29, 2018

Contributor

This describes Authenticator, not NewAuthenticator.
extracts identity which was used to sign message - this is a useful information and could be moved to Authenticator or Authenticate method comment itself.

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 29, 2018

Author Member

Fixed


type authenticator struct{}

func (authenticator *authenticator) Authenticate(message []byte, signature Signature) (Identity, error) {

This comment has been minimized.

Copy link
@donce

donce Jan 29, 2018

Contributor

Comment missing.

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 29, 2018

Author Member

Fixed

)

// Authenticator extracts identity
type Authenticator interface {

This comment has been minimized.

Copy link
@donce

donce Jan 29, 2018

Contributor

Since it's in identity package, it could be called just identity.Extractor.

@Waldz Waldz force-pushed the feature/MYST-114-com-signer-tests branch from b206f49 to 6880792 Jan 29, 2018

@Waldz

This comment has been minimized.

Copy link
Member Author

commented Jan 29, 2018

All comments was fixed

manager.status = statusConnecting()

providerId := identity.FromAddress(nodeKey)

This comment has been minimized.

Copy link
@donce

donce Jan 29, 2018

Contributor

providerID

@@ -22,12 +21,19 @@ type testContext struct {
fakeOpenVpn *fakeOpenvpnClient
}

var (
activeProviderId = "vpn-node-1"

This comment has been minimized.

Copy link
@donce

donce Jan 29, 2018

Contributor

activeProviderID, also, there still are other such names.

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 30, 2018

Author Member

Fixed

Stop() error
ServeDialogs(dialogManager DialogHandler) error

This comment has been minimized.

Copy link
@donce

donce Jan 29, 2018

Contributor

we can omit parameter name in interface:

ServeDialogs(dialogManager DialogHandler) error

DialogHandler type is already as descriptive as dialogManager, naming this in interface doesn't give more clarification but add more confusion (handler == manager?)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 30, 2018

Author Member

Fixed


// DialogHandler defines how to handle incoming Dialog
type DialogHandler interface {
Handle(Dialog) error

This comment has been minimized.

Copy link
@donce

donce Jan 29, 2018

Contributor

Maybe type DialogHandler func(Dialog) error?

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 30, 2018

Author Member

Saw this possibility, but this handler will have Start/Stop interface

@@ -13,4 +13,4 @@ if [ -z "$ARGUMENTS" ]; then
ARGUMENTS=`go list ./...`
fi

go run vendor/github.com/golang/lint/golint/*.go -set_exit_status ${ARGUMENTS}
go run vendor/github.com/golang/lint/golint/*.go --set_exit_status --min_confidence=1 ${ARGUMENTS}

This comment has been minimized.

Copy link
@donce

donce Jan 30, 2018

Contributor

Wasn't this already increased before? :O

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 30, 2018

Author Member

no, thats why we are clearing too many errors now

)
}

func (establisher *dialogEstablisher) newSenderToPeer(

This comment has been minimized.

Copy link
@donce

donce Jan 30, 2018

Contributor

newSenderForPeer

func (establisher *dialogEstablisher) newDialogToContact(
contactAddress *discovery.AddressNATS,
contactCodec communication.Codec,
func (establisher *dialogEstablisher) newDialogToPeer(

This comment has been minimized.

Copy link
@donce

donce Jan 30, 2018

Contributor

newDialogForPeer

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 30, 2018

Author Member

But I want to send messages to another peer

This comment has been minimized.

Copy link
@donce

donce Jan 30, 2018

Contributor

ok, makes sense

}

func (establisher *dialogEstablisher) CreateDialog(contact dto_discovery.Contact) (communication.Dialog, error) {
func (establisher *dialogEstablisher) CreateDialog(

This comment has been minimized.

Copy link
@donce

donce Jan 30, 2018

Contributor

This method is doing a lot of different things - it's tests are hard to understand, because it requires a lot of different mocking.
Have you though about splitting it up?

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 30, 2018

Author Member

Yes. I started splitting to smaller functions like newCodecForPeer() and newSenderToPeer.
Later they will be injectable factories

assert.Nil(t, dialogInstance)

_, ok := dialogInstance.(*dialog)
assert.True(t, ok)

This comment has been minimized.

Copy link
@donce

donce Jan 30, 2018

Contributor

Since you already assert that dialogInstance is nil and this is error is present, what is the purpose of this type check?

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 30, 2018

Author Member

Fixed

sessionCreateConsumer communication.RequestConsumer
}

func (handler *handler) Handle(dialog communication.Dialog) error {

This comment has been minimized.

Copy link
@donce

donce Jan 30, 2018

Contributor

Missing comment.

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 30, 2018

Author Member

Fixed

@Waldz Waldz force-pushed the feature/MYST-114-com-signer-tests branch from 6880792 to 415bfab Jan 30, 2018

@donce

donce approved these changes Jan 30, 2018

func (establisher *dialogEstablisher) newDialogToContact(
contactAddress *discovery.AddressNATS,
contactCodec communication.Codec,
func (establisher *dialogEstablisher) newDialogToPeer(

This comment has been minimized.

Copy link
@donce

donce Jan 30, 2018

Contributor

ok, makes sense

@Waldz Waldz merged commit 1b74faa into master Jan 30, 2018

@Waldz Waldz deleted the feature/MYST-114-com-signer-tests branch Jan 30, 2018

@Waldz Waldz restored the feature/MYST-114-com-signer-tests branch Jan 31, 2018

@Waldz Waldz deleted the feature/MYST-114-com-signer-tests branch Feb 1, 2018

@Waldz Waldz restored the feature/MYST-114-com-signer-tests branch Feb 1, 2018

@donce donce deleted the feature/MYST-114-com-signer-tests branch Mar 5, 2018

@Waldz Waldz restored the feature/MYST-114-com-signer-tests branch Mar 14, 2018

@Waldz Waldz deleted the feature/MYST-114-com-signer-tests branch Mar 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.