node-slack-sdk icon indicating copy to clipboard operation
node-slack-sdk copied to clipboard

Add better support for App Level Tokens

Open stevengill opened this issue 3 years ago • 9 comments

Description

Related to #1132, We need to add better support for App Level Token in @slack/web-api.

I'm thinking we either pass in App Level Token in the token property (this works today), or we introduce a new appToken property. If we introduce a new appToken property, we could add code to make certain api calls use appToken over token (which is usually a bot token). apps.event.authorizations.list & apps.connections.open currently require an App Level Token. Not sure if there are any other web-api methods that currently do.

What type of issue is this? (place an x in one of the [ ])

  • [ ] bug
  • [x] enhancement (feature request)
  • [ ] question
  • [ ] documentation related
  • [ ] testing related
  • [ ] discussion

Requirements (place an x in each of the [ ])

  • [x] I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • [x] I've read and agree to the Code of Conduct.
  • [x] I've searched for any related issues and avoided creating a duplicate issue.

stevengill avatar Jan 12 '21 07:01 stevengill

Not sure if there are any other web-api methods that currently do.

As you mentioned, apps.event.authorizations.list & apps.connections.open are the only ones that require an app-level token.

@stevengill Do we want to add appToken in the next minor release(v6.2)? This sounds like a minor improvement to me but it may be helpful for some use cases.

seratch avatar Apr 09 '21 04:04 seratch

We moved this task to v6.3 or later.

seratch avatar Apr 13 '21 21:04 seratch

Is there a way to differentiate between app-level tokens and other tokens? It wasn't clear in the token types documentation.

filmaj avatar Sep 13 '21 23:09 filmaj

@filmaj - App-level tokens are prefaced with xapp-. That's one way to distinguish. That ought to be included in the docs, and I can see it isn't currently.

srajiang avatar Sep 13 '21 23:09 srajiang

@srajiang cool, thanks for the confirmation.

So, it should be possible for the SDK to hide away the details of the kind of token and handle checking it, figuring the type out, possibly setting some internal flags on the token type? This way we can keep the API the same for the user (a single token property), though we would have to write and maintain logic for token type detection, along with e.g. preventing invocation of the app-level-token-required APIs by the user if the user did not provide an app level token.

Ideally, whatever approach we decide on (single token property vs. separate appToken property) would be consistent across Python and Java implementations.

filmaj avatar Sep 13 '21 23:09 filmaj

As the server-side returns not_allowed_token_type error if you use a different type of token, I don't think we should have custom logic on the client side.

seratch avatar Sep 17 '21 00:09 seratch

@seratch do you mean you prefer to surface the error returned by the server-side API instead of complicating the token handling logic in the SDK?

From my perspective, I think having two separate properties for the user to leverage, token vs. appToken, is confusing and puts the responsibility of understanding when and how each token type should be used on the user. I see this as an additional barrier to usage, especially for first-time users.

filmaj avatar Sep 17 '21 15:09 filmaj

@filmaj If you are thinking about the following update (see the pattern 3 in the code below), I agree that it can be a great improvement in terms of developer experience.

const appLevelToken = "xapp-...";
const botToken = "xoxb-...";

// The following two ways are currently supported

// 1) WebClient tries to use the token anyway
const client = new WebClient(appLevelToken);
const response = await client.apps.connections.open({});

// 2) Developers can pass any type of tokens in the method arguments
const client = new WebClient();
const response = await client.apps.connections.open({token: appLevelToken});

// 3) We may want to add a new option appToken here
// The first argument can be absent (undefined) if a developer does not want to have
const client = new WebClient(botToken, { appToken: appLevelToken });
// WebClient chooses appToken over the token for you
const response = await client.apps.connections.open({});

If you are thinking of some approach to check the prefix of token string values, in general, I don't recommend using it to determine when to use and how. I know that our platform never changed the prefix for tokens. However, we've changed ID prefixes for channels, users, and so on in the past (e.g., U/W for users, C/G for channels (now we don't have G prefix)). Taking place the assumption that there will be only single pattern for the token string prefix can be a cause of unexpected issues in the future.

Also, I do understand that the suggested argument addition in the above code 3) is not intuitive ... but I cannot think of any alternatives. This is because the constructor arguments of WebClient has one token and others as an object. If we can add other structure without breaking changes, that'd be a nice improvement, though.

seratch avatar Sep 21 '21 02:09 seratch

If you are thinking of some approach to check the prefix of token string values, in general, I don't recommend using it to determine when to use and how.

@seratch yes the above approach is what I was considering, but I trust your view on this and will follow your recommendation. Your suggested approach (3) would be an OK alternative, but I don't believe it addresses the need to educate users (especially first-timers) about when and how the app token is used - so we will need to make sure to clearly document this difference.

filmaj avatar Sep 21 '21 16:09 filmaj