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

Add better support for App Level Tokens #1153

Open
4 of 9 tasks
stevengill opened this issue Jan 12, 2021 · 9 comments
Open
4 of 9 tasks

Add better support for App Level Tokens #1153

stevengill opened this issue Jan 12, 2021 · 9 comments
Labels
enhancement M-T: A feature request for new functionality pkg:web-api applies to `@slack/web-api`
Milestone

Comments

@stevengill
Copy link
Member

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
  • enhancement (feature request)
  • question
  • documentation related
  • testing related
  • discussion

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

  • I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've searched for any related issues and avoided creating a duplicate issue.
@stevengill stevengill added enhancement M-T: A feature request for new functionality pkg:web-api applies to `@slack/web-api` labels Jan 12, 2021
@stevengill stevengill self-assigned this Mar 15, 2021
@seratch seratch added this to the web-api@6.2 milestone Mar 24, 2021
@seratch
Copy link
Member

seratch commented Apr 9, 2021

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 seratch modified the milestones: web-api@6.2, web-api@6.x Apr 13, 2021
@seratch
Copy link
Member

seratch commented Apr 13, 2021

We moved this task to v6.3 or later.

@seratch seratch modified the milestones: web-api@6.x, web-api@6.3 Apr 13, 2021
@stevengill stevengill removed their assignment Jun 1, 2021
@seratch seratch modified the milestones: web-api@6.3, web-api@6.4 Jul 13, 2021
@srajiang srajiang modified the milestones: web-api@6.4, web-api@6.5.0 Aug 19, 2021
@filmaj
Copy link
Contributor

filmaj commented Sep 13, 2021

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

@srajiang
Copy link
Member

@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.

@filmaj
Copy link
Contributor

filmaj commented Sep 13, 2021

@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.

@seratch
Copy link
Member

seratch commented Sep 17, 2021

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.

@filmaj
Copy link
Contributor

filmaj commented Sep 17, 2021

@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.

@seratch
Copy link
Member

seratch commented Sep 21, 2021

@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 seratch modified the milestones: web-api@6.5.0, web-api@6.6.0 Sep 21, 2021
@filmaj
Copy link
Contributor

filmaj commented Sep 21, 2021

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 filmaj modified the milestones: web-api@6.6.0, web-api@6.7.0 Jan 20, 2022
@seratch seratch modified the milestones: web-api@6.7.0, web-api@6.x Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality pkg:web-api applies to `@slack/web-api`
Projects
None yet
Development

No branches or pull requests

4 participants