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-41 Session generation and exchange #47

Merged
merged 7 commits into from Nov 28, 2017

Conversation

Projects
None yet
3 participants
@ignasbernotas
Copy link
Member

commented Nov 21, 2017

No description provided.

@ignasbernotas ignasbernotas requested review from tadovas, interro and Waldz Nov 21, 2017

return err
}

session.Id = string(vpnSession.Id)

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 21, 2017

Member
  • Dont mutate variable in runtime
  • I guess session stats sending started to fail
if err != nil {
return err
}

vpnSession, err := getVpnSession(vpnSessionJson)

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 21, 2017

Member

getVpnSession() could be a wrapper: make request + parse response

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 21, 2017

Member

I am missing test, which freezes response of SESSION_CREATE

type SessionId string

func GenerateSessionId() (sid SessionId, err error) {
b, err := generateRandomBytes(16)

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 21, 2017

Member

Did You consider using UniqueID library, which has randomness issues solved?
I used https://github.com/satori/go.uuid:

requestId := uuid.NewV4().String()
return sid, err
}

for !manager.sessionIsUnique(sid) {

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 21, 2017

Member

What if SID will have collision with another node's session?
Maybe let add ticket for future improvement

manager := Manager{}
length := 10

for i := 0; i < length; i++ {

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 21, 2017

Member

Looks like loop with overhead.
You can:

  • create SID
  • test private function sessionIsUnique()
func NewVpnSession(config string) (session VpnSession, err error) {
id, err := manager.Create()
if err != nil {
return VpnSession{}, err

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 21, 2017

Member

You can just return:

return

or

return session, err
return VpnSession{}, err
}

vpnSession := VpnSession{

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 21, 2017

Member
session = VpnSession{...}
return "Failed to serialize VPN session."
}

return string(session)

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 22, 2017

Member

Missing acceptance:
Node responds “session_rejected” with “reasonType”, “reasonMessage” (when Client's proposalId mismatches with Node's current proposal)

@shroomist
Copy link
Contributor

left a comment

we need something more complex than strings for RequestType and MessageType for them to be able to encapsulate proposalId, serviceStartParams (Request) and “session_rejected” with “reasonType”, “reasonMessage" for MessageType (is it ResponseType?)

DIALOG_CREATE = RequestType("dialog-create")
GET_CONNECTION_CONFIG = RequestType("get-connection-config")
DIALOG_CREATE = RequestType("dialog-create")
SESSION_CREATE = RequestType("session-create")

This comment has been minimized.

Copy link
@shroomist

shroomist Nov 25, 2017

Contributor

I suggest defining structs for message types

type SessionCreate struct {
proposalId, serviceStartParams
}

we also need response types defined here

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Nov 27, 2017

Author Member

Message types do have their own structs:) These constants are basically topics for the communication channel, so we still need them.

@@ -38,13 +42,13 @@ func (cmd *CommandRun) Run(options CommandOptions) (err error) {
return err
}

vpnConfigString, err := sender.Request(communication.GET_CONNECTION_CONFIG, "")
vpnSession, err := getVpnSession(sender, strconv.Itoa(proposal.Id))

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 27, 2017

Member

Why casting in controller level

}

if response.Success == false {
return session, errors.New(response.Message)

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 27, 2017

Member

Error might return empty. I suggest adding prefix string

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Nov 27, 2017

Author Member

Don't see how that could happen. All edge cases are handled. What did I miss?:)

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 28, 2017

Member

Because it's external message, can come from another node's code

config, _ := buildVpnClientConfig(vpnServerIp, options.DirectoryConfig)
return config
})
receiver.Respond(communication.SESSION_CREATE, sessionResponseHandler.Handle)

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 27, 2017

Member

spacy spaces

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Nov 27, 2017

Author Member

It seems so on github, IDE shows it fine.

return serializeCreateResponse(response)
}

str := serializeCreateResponse(scr.buildResponse())

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 27, 2017

Member

Nice segragation

}

func TestHandler_Success(t *testing.T) {
handler := CreateResponseHandler{

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 27, 2017

Member

You could reuse same handler|code-block in all tests

"testing"
)

func TestManagerHasSessionsStored(t *testing.T) {

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 27, 2017

Member

Does not look like unit.
Is it like integration test?

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Nov 27, 2017

Author Member

Yeah, testing the manager is kind of pointless in this case.. the only use case for unit tests would be to freeze the interface methods, does that make sense though?

@Waldz
Copy link
Member

left a comment

Questioned some places

return id
}

func (manager *Manager) Get(sid SessionId) (sessionId SessionId) {

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 27, 2017

Member

Impossible to distinguish if session was found/not-found.
Test would help You to prove usability.

@Waldz

Waldz approved these changes Nov 28, 2017

@Waldz Waldz merged commit c381b55 into master Nov 28, 2017

@Waldz Waldz deleted the feature/MYST-41-session-exchange branch Nov 28, 2017

@Waldz Waldz changed the title [MYST-41] Session generation and exchange MYST-41 Session generation and exchange Nov 28, 2017

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.