[HERMES-5354] Add userScopes to ISlackManifestShared#189
Conversation
| socketModeEnabled?: boolean; | ||
| tokenRotationEnabled?: boolean; | ||
| appDirectory?: ManifestAppDirectorySchema; | ||
| userScopes?: Array<string>; |
There was a problem hiding this comment.
No longer needed here since we're moving it up to the shared interface.
filmaj
left a comment
filmaj
left a comment
There was a problem hiding this comment.
Left a couple of questions.
WilliamBergamin
left a comment
WilliamBergamin
left a comment
There was a problem hiding this comment.
Good changes 💯 just had one query
| true, | ||
| ); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Seems like this file defines botScopes: [] in many tests 🤔
Should we also define userScopes:[] in those tests as well @filmaj ?
There was a problem hiding this comment.
I'd like for the tests to use definition variables that reflect how end-users would craft their own manifest definitions. In my own experience creating manifest definitions, I would omit the property altogether if I'm not using it. However, devs do the darndest things! I think we should guard against common JS-y/TS-y pitfalls such as what counts as truthy vs. falsy, like empty arrays and empty strings. Therefore, I would recommend the Manifest() constructor clean user-provided definitions up into a state that the Slack manifest APIs expect as a baseline.
Codecov Report
@@ Coverage Diff @@
## main #189 +/- ##
=======================================
Coverage 99.23% 99.23%
=======================================
Files 55 55
Lines 2084 2084
Branches 115 115
=======================================
Hits 2068 2068
Misses 15 15
Partials 1 1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| manifest.oauth_config.token_management_enabled = false; | ||
| // Oauth Config | ||
| // Run On Slack manage their own tokens | ||
| // if and only if at least one user scope is included in the manifest |
There was a problem hiding this comment.
I'm not sure I understand this-- Adding scopes in the manifest should not require apps to use token management, right? In general I don't think any ROSI apps needs token management, right? Having token management enabled would mean the app needs to provide an oauth token server, which ROSI apps cannot do. Is there an issue we're trying to solve with this change?
There was a problem hiding this comment.
@jpbrabec - This was also something I'm also unsure about, so thanks for weighing in.
Adding scopes in the manifest should not require apps to use token management.
I get this error when adding user scopes to the manifest and leaving token_management_enabled: false:
Is this error expected?
There was a problem hiding this comment.
Ah, that's for user scopes specifically (not bot scopes). Hermes apps do not support user scopes at all right now, so this error is basically here to say "You can't be a hermes ROSI app and use user scopes at the same time". We can chat in slack about future plans for this area.
There was a problem hiding this comment.
@jpbrabec are you open to me changing the error to be able to bypass it if Hermes User Delegated Auth Houston FF is in place
5 participants
Summary
Adds the
userScopesproperty to the ISlackManifestShared interface.Ask from BE folks to enable authoring app manifest that use built-ins with user-delegated auth.
Context thread
Changes:
userScopesproperty to theISlackManifestSharedinterface and sets the value on Manifest if not emptytoken_management_enableddefaulted to false on behalf of the developer. But in the case where user scopes are requested, the backend requirestoken_management_enabledto be set totrue. So I have modified the SDK behavior to default to setting the value to true only when there's at least one user scope defined.Requirements (place an
xin each[ ])