search

Nexmo / nexmo-cli

Nexmo CLI (Command Line Interface)
https://nexmo.com
MIT License
78 stars 52 forks source link

Update request.js #135

Closed sammachin closed 3 years ago

sammachin commented 7 years ago

make the API Key all lower case to work around bug on applications api

cbetta commented 7 years ago

Hmm shouldn't we fix this in the node library?

On Fri, 24 Mar 2017, 16:19 Sam Machin, notifications@github.com wrote:

make the API Key all lower case to work around bug on applications api

You can view, comment on, or merge this pull request online at:

https://github.com/Nexmo/nexmo-cli/pull/135 Commit Summary

  • Update request.js

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Nexmo/nexmo-cli/pull/135, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAeJpazn-8kkvIcoPqGz-5zVN9Y6s47ks5ro-0LgaJpZM4MoeJ- .

sammachin commented 7 years ago

Not really it's kinda a temporary workaround anyway, API Keys shouldn't be case sensitive but the application API is only accepting lower case keys, this should be fixed in the API soon but this is to hide the error for application management which is mainly done via the CLI. If we put the fix into the node library then we should really put it into all the libraries. I'm not 100% convinced we should be 'fixing' this at all as the user could just re-setup their CLI with the key in the same format as it is in the dashboard (eg lower case) but this helps to smooth the experience along.

cbetta commented 7 years ago

What is the ETA for the API to have this fix? If we fix it I'd rather do some in the node library personally, as the same bug will affect anyone using that. If the fix is out soon, then let's not fix it, otherwise I think we should cater to as many people as we can.

leggetter commented 7 years ago

I've created a test for this fef1e2d0b5ca364467fe647cc5471af6c2e0ba7f

As an alternative to the toLowerCase option, how about we do some client-side validation of the key and error if it contains any uppercase characters? That way the user is informed that keys have to be lowercase and we're not adding a "hack" to anything.

sammachin commented 7 years ago

@leggetter thats a nice option although its more work? The problem is that the keys don't HAVE to be lower case most of the APIs are case insensitive just there's a bug in the Application API that means it enforces case, and our convention has been to always use lowercase.....