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.

Sign up for GitHub

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

Jump to bottom

[HERMES-5354] Add userScopes to ISlackManifestShared #189

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[HERMES-5354] Add userScopes to ISlackManifestShared #189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
124a54f
Add user scopes to shared manifest interface
srajiang Jul 10, 2023
76d64f9
Add test
srajiang Jul 10, 2023
0b34cfd
Add test for empty scopes array
srajiang Jul 10, 2023
8eda211
Do not set token_management_enabled=true
srajiang Jul 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/manifest/manifest_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,6 +745,26 @@ Deno.test("Manifest() correctly assigns other app features", () => {
);
});

Deno.test("Manifest() ensures that runOnSlack apps can define user-level scopes", () => {
const definition: SlackManifestType = {
runOnSlack: true,
name: "fear and loathing in las vegas",
description:
"fear and loathing in las vegas: a savage journey to the heart of the american dream",
displayName: "fear and loathing",
icon: "icon.png",
botScopes: ["channels:history", "chat:write", "commands"],
userScopes: ["users:read"],
};
const manifest = Manifest(definition);

// ensure user scopes are set on the manifest
assertStrictEquals(
manifest.oauth_config.scopes.user,
definition.userScopes,
);
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this file defines botScopes: [] in many tests 🤔
Should we also define userScopes:[] in those tests as well @filmaj ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Deno.test("SlackManifest() registration functions don't allow duplicates", () => {
const functionId = "test_function";
const arrayTypeId = "test_array_type";
Expand Down
9 changes: 5 additions & 4 deletions src/manifest/mod.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export class SlackManifest {

export() {
const def = this.definition;

const manifest: ManifestSchema = {
_metadata: {
// todo: is there a more idiomatic way of defining this? constant file?
Expand All @@ -50,6 +51,7 @@ export class SlackManifest {
oauth_config: {
scopes: {
bot: this.ensureBotScopes(),
user: def.userScopes,
srajiang marked this conversation as resolved.
Show resolved Hide resolved
},
},
features: {
Expand Down Expand Up @@ -256,10 +258,9 @@ export class SlackManifest {
manifest.app_directory = def.appDirectory;

//OauthConfig
manifest.oauth_config.scopes.user = def.userScopes;
manifest.oauth_config.redirect_urls = def.redirectUrls;

// Remote-hosted Slack apps manage their own tokens
// Remote-hosted Slack apps always manage their own tokens
manifest.oauth_config.token_management_enabled = true;

// Remote Features
Expand All @@ -274,8 +275,8 @@ export class SlackManifest {
private assignRunOnSlackManifestProperties(manifest: ManifestSchema) {
const def = this.definition as ISlackManifestRunOnSlack;

// Run on Slack Apps do not manage access tokens
// This is set by default as false
// Oauth Config
// Run On Slack don't manage their own tokens
manifest.oauth_config.token_management_enabled = false;

// Required App Settings for run on slack apps
Expand Down
14 changes: 7 additions & 7 deletions src/manifest/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,26 +60,26 @@ export interface ISlackManifestRemote extends ISlackManifestShared {
socketModeEnabled?: boolean;
tokenRotationEnabled?: boolean;
appDirectory?: ManifestAppDirectorySchema;
userScopes?: Array<string>;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer needed here since we're moving it up to the shared interface.

redirectUrls?: Array<string>;
features?: ISlackManifestRemoteFeaturesSchema;
}

/* Shared app manifest properties */
interface ISlackManifestShared {
name: string;
backgroundColor?: string;
botScopes: Array<string>;
datastores?: ManifestDatastore[];
description: string;
displayName?: string;
events?: ICustomEvent[];
functions?: ManifestFunction[];
icon: string;
longDescription?: string;
botScopes: Array<string>;
functions?: ManifestFunction[];
workflows?: ManifestWorkflow[];
name: string;
outgoingDomains?: Array<string>;
events?: ICustomEvent[];
types?: ICustomType[];
datastores?: ManifestDatastore[];
userScopes?: Array<string>;
workflows?: ManifestWorkflow[];
}

interface ISlackManifestRunOnSlackFeaturesSchema {
Expand Down