Consensys / armlet

a MythX API client wrapper
MIT License
17 stars 7 forks source link

Fix bug in compatibility in using old API. #22

Closed rocky closed 5 years ago

rocky commented 5 years ago

Given the threat of a demo and the fact that over the weekend we saw that people are still using the old API_KEY, and the desire to be able to get people up to speed quickly (on prod), I see the wisdom of better compatibility with the old API_KEY. For now.

The meat of this PR is really in one little logic bug in index.js.

fgimenez commented 5 years ago

I think this change would break the checks for JWT authentication.

In API key authenticationemail is not required, it is enough to set the apiKey field (this has been always this way, it hasn't changed lately).

In JWT authentication, you can set either email or ethAddress. If one of them is defined, then you need to also set password, this is what the check enforces:

if ((auth.email !== undefined || auth.ethAddress !== undefined) && auth.password === undefined) {
    throw new TypeError('Please provide a password auth option.')
}

If you allow email to be undefined in this check then you could instantiate the client just with email, and you would not be able to authenticate. Let me know if I can help to make this more clear, thanks!

rocky commented 5 years ago

Ok. Without that, I wasn't able to get the older auth working. @daniyarchambylov will fix up my code and put in a PR. Thanks Daniyar!

fgimenez commented 5 years ago

Without that, I wasn't able to get the older auth working.

Yep, you probably were setting both email and apiKey, you only need to set apiKey for API key based auth.

birdofpreyru commented 5 years ago

Noticed the piece of code above in email updates. You really should write such things as

if ((auth.email || auth.ethAddress) && !auth.password) {
    throw new TypeError('Please provide a password auth option.')
}

it will save you from many potential problems going forward (e.g. it works as intended if fields are set to null rather than undefined).

rocky commented 5 years ago

@daniyarchambylov would you address @birdofpreyru concern?

@birdofpreyru I'd be interested to understand in more detail what kinds of problems this would address, or in what scenary we would find this. Thanks.

birdofpreyru commented 5 years ago

@rocky well, may be problems is too bold to say here. But when people want to "unset" a string field in JS, they have many choices: delete the object field, assign undefined to it, or assign null, or assign empty string. You write the code I propose, it will handle all these options the same and correctly; and also the condition itself looks more compact.

rocky commented 5 years ago

@birdofpreyru Thanks for the clarification. I think I understand now.

daniyarchambylov commented 5 years ago

Since @birdofpreyru raised issue of code readability and avoiding potential bugs I pushed new changes with rewritten Client constructor

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 157


Totals Coverage Status
Change from base Build 156: 0.01%
Covered Lines: 190
Relevant Lines: 191

💛 - Coveralls
daniyarchambylov commented 5 years ago

@fgimenez Done!

daniyarchambylov commented 5 years ago

@rocky @fgimenez @birdofpreyru - Coonflicts resolved.

rocky commented 5 years ago

@fgimenez @birdofpreyru what is the status of this PR?

birdofpreyru commented 5 years ago

I am not really following this one. I guess, you can merge it in.

fgimenez commented 5 years ago

Yes, I thought it was already in, sorry for not approving before.