balena-io / balena-sdk

The SDK to make balena powered JavaScript applications.
https://www.balena.io/
Apache License 2.0
145 stars 46 forks source link

Add support for user-level API keys to be used instead of JWTs #364

Closed flesler closed 6 years ago

flesler commented 7 years ago

HQ: https://github.com/resin-io/hq/pull/861

Discussions:

  1. https://www.flowdock.com/app/rulemotion/resin-tech/threads/Tp6Up7Zu2U9qAnOLfqsqY3bYqv0
  2. https://www.flowdock.com/app/rulemotion/resin-frontend/threads/_g5ZBIGqmsKYzk5YG_cDx5B07-h
  3. https://www.flowdock.com/app/rulemotion/platform/threads/pUlR_TwVuF5RywMqsgYCxYe-QVL
pimterry commented 7 years ago

Just to summarize the current state and plan: the goal here is that as well as the currently available login methods, you should also be able to log in with an api key.

The API work for this is done (ask @flesler if you have any specific questions on the backend part), and there's a replacement for resin-token called resin-auth that should give you a single interface that will work for both JWTs and api keys.

To do this, we're going to need to:

There's going to be a few breaking changes in here, please make PRs with breaking changes to the v8 branch, with a no-checks label!. We don't want to put this on master, or for versionbot to build and release this immediately - instead we want to batch up our breaking changes together to make life easier for users, and then we'll merge the whole breaking branch into master once it's all ready.

flesler commented 7 years ago

One thing I'd like to add is. Historically, JWT have been sent on the Authorization header while API Keys where included as apikey in the querystring (most of the time). With this PR that is close to production, due to a security concern, we're moving towards sending both JWTs and API Keys in the same place (Authorization header). This actually simplifies a lot the integration of resin-auth into resin-request. Because you just need to put whatever is in auth.getKey() in the header, regardless of what it is

MoranF commented 7 years ago

@flesler , @pimterry , the SDK is using token.getUserId, token.getUserName, token.getEmail. How should I use these methods using resin-auth ?

MoranF commented 7 years ago

@pimterry , I also have a lot of these errors:

17) Application Model given no applications resin.models.application.create() should be able to create a child application:
     TypeError: token.has is not a function
      at ResinPine._request (node_modules/resin-pine/build/pine.js:86:20)
      at ResinPine.PinejsClientCore.request (node_modules/pinejs-client/core.js:615:25)

that seems to come from pine.

pimterry commented 7 years ago

the SDK is using token.getUserId, token.getUserName, token.getEmail. How should I use these methods using resin-auth ?

@MoranF see my comment above:

There are some places that depend on the internal data of the token (jwt.io can be helpful to see this data for yourself), those will need to change (because API keys contain no data), and should probably all be making requests to our API instead.

Those token.has errors you're seeing are errors with resin-token usage inside resin-pine - you'll need to upgrade that to use resin-auth too.

MoranF commented 7 years ago

@pimterry , the SDK already uses auth = require('../auth')(deps, opts) and resin.auth. I can change also token to auth, but I'm afraid it can be confusing. What do you say?

pimterry commented 7 years ago

@MoranF we're going to stop exposing the instance directly, so resin.auth will still be the SDK's current auth module, and resin.token will just go away. I don't have an easy answer for variable naming inside the SDK code itself though, you'll need to come up with something sensible there. auth/authSdk?

MoranF commented 7 years ago

@pimterry , how do I start using the API in the SDK project?

pimterry commented 7 years ago

@MoranF sorry, I have no idea what that means. Can you be more specific?

MoranF commented 7 years ago

@pimterry , about Stop exposing resin.token directly to end users, and maybe expose resin.auth.getKey() instead (this is mainly just used by the UI, so just hunt down whatever it needs it for) it seems that the UI uses token.set , token.get and token.remove (and maybe will need token.hasKey also). Do you prefer that I will expose all these methods instead the whole object?

pimterry commented 7 years ago

@MoranF something like that, yes. It depends a bit on how the UI uses these methods, since maybe there are changes we should make in the UI instead, but if we can't then making them standalone methods here instead is the right choice.

For example, why does the UI use token.remove instead of the already existing sdk.auth.logout()? Could the UI use the already existing sdk.auth.isLoggedIn() instead of hasKey, and auth.loginWithToken() instead of token.set? We should add extra methods where it's necessary to support things that the SDK can't already do, but we should first fix the UI to simplify things as much as we can.

MoranF commented 7 years ago

Depends-On: https://github.com/resin-io/resin-api/pull/661

MoranF commented 7 years ago

@flesler , is the method token.setKey should work also with an api-key as a parameter?

MoranF commented 7 years ago

@pimterry , about Add a resin.auth.loginWithApiKey method, auth.setKey should work with api keys also, so the method sdk.auth.loginWithAuth should work with JWT and with api keys, isn't it?

I also had some failed tests, but they failed also when I ran these tests on master.

pimterry commented 7 years ago

sdk.auth.loginWithAuth should work with JWT and with api keys

Sure, if that works that's great :+1:

is the method token.setKey should work also with an api-key as a parameter?

It's not really clear what this sentence means. Are you asking if setKey should work with both API keys and JWTs? If so, it sounds like you've already found the answer, but let me know if not :smiley:.

thgreasi commented 6 years ago

@MoranF should this be closed now that https://github.com/resin-io/resin-sdk/pull/449 is merged and published as sdk v8?

MoranF commented 6 years ago

@thgreasi , yes