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

Identity passphrase (Unlock endpoint for tequilapi, server command option, CLI) #100

Merged
merged 11 commits into from Jan 19, 2018

Conversation

Projects
None yet
3 participants
@donce
Copy link
Contributor

commented Jan 16, 2018

No description provided.

@Waldz

This comment has been minimized.

Copy link
Member

commented Jan 17, 2018

As You will be touching most of identity package, could You fix Linter problems there with one of PRs?

@@ -35,10 +37,12 @@ func NewCommandWith(
identityManager := identity.NewIdentityManager(keystoreInstance)

dialogEstablisherFactory := func(myIdentity identity.Identity) communication.DialogEstablisher {
identityManager.Unlock(myIdentity.Address, identityPassword)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 17, 2018

Member

mysterium_client command got Tequilapi endpoint, and shouldn't be auto-unlocking anymore

This comment has been minimized.

Copy link
@donce

donce Jan 18, 2018

Author Contributor

Done

@@ -28,6 +28,11 @@ type identityRegistrationDto struct {
Registered bool `json:"registered"`
}

type identityUnlockingDto struct {
Id string `json:"id"`

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 17, 2018

Member

Id defines REST resource address and should not be in payload

This comment has been minimized.

Copy link
@donce

donce Jan 18, 2018

Author Contributor

Done

id, err = ih.manager.GetIdentity(address)
if err != nil {
return
}
err = ih.manager.Unlock(address, passphrase)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 17, 2018

Member

This look like duplicated block. Better to run unlock after any of 3 cases in LoadIdentity()

This comment has been minimized.

Copy link
@donce

donce Jan 17, 2018

Author Contributor

Actually thought about that, but that method is untested, so I was hesitating to complicste it more.. But I'll try to add tests for that, using mock functions

handler := NewNodeIdentityHandler(identityManager, client, cache, fakeSignerFactory)

_, err := handler.UseLast("pass")
assert.NotNil(t, err)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 17, 2018

Member

Would be good to asset exact error with assert.EqualError()

This comment has been minimized.

Copy link
@donce

donce Jan 17, 2018

Author Contributor

The exact error is a mock error, so there is no much value in that.

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 18, 2018

Member

IMHO it's still better - because freezes more exact error

}

func Test_identityHandler_UseNewUnlockFails(t *testing.T) {
identityManager.CleanStatus()

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 17, 2018

Member

Avoid having global state by constructing identityManager in each test

This comment has been minimized.

Copy link
@donce

donce Jan 18, 2018

Author Contributor

Done


err = endpoint.idm.Unlock(id, unlockReq.Passphrase)
if err != nil {
utils.SendError(resp, err, http.StatusUnprocessableEntity)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 17, 2018

Member

It not validation error, but failed action e.g. 500

This comment has been minimized.

Copy link
@donce

donce Jan 17, 2018

Author Contributor

500 seems to harsh, becaus the most likely reason for this failure is user error - user passed us incorrect passphrase. Any other suitable http statuses?

This comment has been minimized.

Copy link
@shroomist

shroomist Jan 18, 2018

Contributor

403 Forbidden

This comment has been minimized.

Copy link
@donce

donce Jan 18, 2018

Author Contributor

Done 403

id := params.ByName("id")
unlockReq, err := toUnlockRequest(request)
if err != nil {
utils.SendError(resp, err, http.StatusBadRequest)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 17, 2018

Member

Should not validation errors be sent with utils.SendValidationErrorMessage()?

This comment has been minimized.

Copy link
@donce

donce Jan 18, 2018

Author Contributor

In most of endpoints, request parsing happens in two stages:

  • Mapping of json into go struct,
  • Validating go struct - whether required data is present and in correct format.

If 1st stage fails and json is not parsed into our struct, BadRequest is returned.
If 1st succeeds, than 2nd follow, which might return validation errors.

This error we're discussing is due to 1st - failing to parse json.

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 18, 2018

Member

1st stage 400 - got it
2nd stage - so I missed this stage, validation of DTO fields

This comment has been minimized.

Copy link
@donce

donce Jan 18, 2018

Author Contributor

There is only one field - password, which is optional. What validation are you missing?

@donce

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2018

@Waldz ok, will fix identity linter errors in a separate PR - created a subtask for that https://mysteriumnetwork.atlassian.net/browse/MYST-249

@donce donce force-pushed the feature/MYST-241-tequila-unlock-endpoint branch 5 times, most recently from 7658104 to 85c71a6 Jan 18, 2018

@donce donce changed the title Unlock endpoint for tequilapi Identity passphrase (Unlock endpoint for tequilapi, server command option) Jan 18, 2018

@donce donce force-pushed the feature/MYST-241-tequila-unlock-endpoint branch from b42c3e6 to d774a56 Jan 19, 2018

@@ -10,6 +10,7 @@ type CommandOptions struct {
DirectoryConfig string
DirectoryRuntime string
DirectoryKeystore string
Passphrase string

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 19, 2018

Member

passphrase -> password to be consistent with previous naming (i think in Tequilapi.CreateIdentity endpoint).
Or sync both places

This comment has been minimized.

@donce donce changed the title Identity passphrase (Unlock endpoint for tequilapi, server command option) Identity passphrase (Unlock endpoint for tequilapi, server command option, CLI) Jan 19, 2018

@donce donce force-pushed the feature/MYST-241-tequila-unlock-endpoint branch from 35bf73d to 5e36467 Jan 19, 2018

@@ -150,6 +146,37 @@ func (c *Command) connect(line string) {
success("Connected.")
}

func (c *Command) unlock(line string) {
connectionArgs := strings.TrimSpace(line[7:])

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 19, 2018

Member

unlockArgs

func (c *Command) unlock(line string) {
connectionArgs := strings.TrimSpace(line[7:])

unlockSignature := "Unlock <identity> <passphrase (optional)>"

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 19, 2018

Member

optional things are marked with []
unlock identity [passphrase]

@Waldz

Waldz approved these changes Jan 19, 2018

@donce donce merged commit b1c611c into master Jan 19, 2018

@donce donce deleted the feature/MYST-241-tequila-unlock-endpoint branch Jan 19, 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.