From 96c961e0ccf31676c7fd48c37342eac8a8a93a98 Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Fri, 11 Dec 2020 00:52:58 -0800 Subject: [PATCH 1/9] Add message event subtypes to SlackEvent union Moved all the subtypes into a new file to keep them more organized. Wrote a test file that should theoretically pass when this issue is fixed. Does not yet pass TS compiler, more changes to come. --- src/types/events/base-events.ts | 248 ++++++++--------------------- src/types/events/message-events.ts | 128 +++++++++++++++ types-tests/message.test-d.ts | 16 ++ 3 files changed, 209 insertions(+), 183 deletions(-) create mode 100644 src/types/events/message-events.ts create mode 100644 types-tests/message.test-d.ts diff --git a/src/types/events/base-events.ts b/src/types/events/base-events.ts index dfabcc185..365e04c93 100644 --- a/src/types/events/base-events.ts +++ b/src/types/events/base-events.ts @@ -1,5 +1,5 @@ -import { MessageAttachment, KnownBlock, Block, View } from '@slack/types'; -import { StringIndexed } from '../helpers'; +import { View } from '@slack/types'; +import { MessageEvent as AllMessageEvents } from './message-events'; /** * All known event types in Slack's Events API @@ -76,16 +76,13 @@ export type SlackEvent = * this interface. That condition isn't enforced, since we're not interested in factoring out common properties from the * known event types. */ -export interface BasicSlackEvent extends StringIndexed { +export interface BasicSlackEvent { type: Type; } /* ------- TODO: Generate these interfaces ------- */ -// TODO: why are these all StringIndexed? who does that really help when going more than one level deep means you have -// to start coercing types anyway? - -export interface AppRequestedEvent extends StringIndexed { +export interface AppRequestedEvent { type: 'app_requested'; app_request: { id: string; @@ -131,7 +128,7 @@ export interface AppRequestedEvent extends StringIndexed { date_created: number; } -export interface AppHomeOpenedEvent extends StringIndexed { +export interface AppHomeOpenedEvent { type: 'app_home_opened'; user: string; channel: string; @@ -142,7 +139,7 @@ export interface AppHomeOpenedEvent extends StringIndexed { // NOTE: this is essentially the same as the `message` event, except for the type and that this uses `event_ts` instead // of `ts` -export interface AppMentionEvent extends StringIndexed { +export interface AppMentionEvent { type: 'app_mention'; user: string; text: string; @@ -154,14 +151,14 @@ export interface AppMentionEvent extends StringIndexed { // TODO: this event doesn't use the envelope. write test cases to make sure its works without breaking, and figure out // what exceptions need to be made to the related types to make this work // https://api.slack.com/events/app_rate_limited -// export interface AppRateLimitedEvent extends StringIndexed { +// export interface AppRateLimitedEvent { // } -export interface AppUninstalledEvent extends StringIndexed { +export interface AppUninstalledEvent { type: 'app_uninstalled'; } -export interface CallRejectedEvent extends StringIndexed { +export interface CallRejectedEvent { type: 'call_rejected'; call_id: string; user_id: string; @@ -169,13 +166,13 @@ export interface CallRejectedEvent extends StringIndexed { external_unique_id: string; } -export interface ChannelArchiveEvent extends StringIndexed { +export interface ChannelArchiveEvent { type: 'channel_archive'; channel: string; user: string; } -export interface ChannelCreatedEvent extends StringIndexed { +export interface ChannelCreatedEvent { type: 'channel_created'; channel: { id: string; @@ -185,24 +182,24 @@ export interface ChannelCreatedEvent extends StringIndexed { }; } -export interface ChannelDeletedEvent extends StringIndexed { +export interface ChannelDeletedEvent { type: 'channel_deleted'; channel: string; } -export interface ChannelHistoryChangedEvent extends StringIndexed { +export interface ChannelHistoryChangedEvent { type: 'channel_history_changed'; latest: string; ts: string; event_ts: string; } -export interface ChannelLeftEvent extends StringIndexed { +export interface ChannelLeftEvent { type: 'channel_left'; channel: string; } -export interface ChannelRenameEvent extends StringIndexed { +export interface ChannelRenameEvent { type: 'channel_rename'; channel: { id: string; @@ -211,20 +208,20 @@ export interface ChannelRenameEvent extends StringIndexed { }; } -export interface ChannelSharedEvent extends StringIndexed { +export interface ChannelSharedEvent { type: 'channel_shared'; connected_team_id: string; channel: string; event_ts: string; } -export interface ChannelUnarchiveEvent extends StringIndexed { +export interface ChannelUnarchiveEvent { type: 'channel_unarchive'; channel: string; user: string; } -export interface ChannelUnsharedEvent extends StringIndexed { +export interface ChannelUnsharedEvent { type: 'channel_unshared'; previously_connected_team_id: string; channel: string; @@ -232,7 +229,7 @@ export interface ChannelUnsharedEvent extends StringIndexed { event_ts: string; } -export interface DNDUpdatedEvent extends StringIndexed { +export interface DNDUpdatedEvent { type: 'dnd_updated'; user: string; dnd_status: { @@ -245,7 +242,7 @@ export interface DNDUpdatedEvent extends StringIndexed { }; } -export interface DNDUpdatedUserEvent extends StringIndexed { +export interface DNDUpdatedUserEvent { type: 'dnd_updated_user'; user: string; dnd_status: { @@ -255,14 +252,14 @@ export interface DNDUpdatedUserEvent extends StringIndexed { }; } -export interface EmailDomainChangedEvent extends StringIndexed { +export interface EmailDomainChangedEvent { type: 'email_domain_changed'; email_domain: string; event_ts: string; } // NOTE: this should probably be broken into its two subtypes -export interface EmojiChangedEvent extends StringIndexed { +export interface EmojiChangedEvent { type: 'emoji_changed'; subtype: 'add' | 'remove'; names?: string[]; // only for remove @@ -271,7 +268,7 @@ export interface EmojiChangedEvent extends StringIndexed { event_ts: string; } -export interface FileChangeEvent extends StringIndexed { +export interface FileChangeEvent { type: 'file_change'; file_id: string; // TODO: incomplete, this should be a reference to a File shape from @slack/types @@ -283,7 +280,7 @@ export interface FileChangeEvent extends StringIndexed { // NOTE: `file_comment_added` and `file_comment_edited` are left out because they are discontinued -export interface FileCommentDeletedEvent extends StringIndexed { +export interface FileCommentDeletedEvent { type: 'file_comment_deleted'; comment: string; // this is an ID file_id: string; @@ -294,7 +291,7 @@ export interface FileCommentDeletedEvent extends StringIndexed { }; } -export interface FileCreatedEvent extends StringIndexed { +export interface FileCreatedEvent { type: 'file_created'; file_id: string; // TODO: incomplete, this should be a reference to a File shape from @slack/types @@ -304,13 +301,13 @@ export interface FileCreatedEvent extends StringIndexed { }; } -export interface FileDeletedEvent extends StringIndexed { +export interface FileDeletedEvent { type: 'file_deleted'; file_id: string; event_ts: string; } -export interface FilePublicEvent extends StringIndexed { +export interface FilePublicEvent { type: 'file_public'; file_id: string; // TODO: incomplete, this should be a reference to a File shape from @slack/types @@ -320,7 +317,7 @@ export interface FilePublicEvent extends StringIndexed { }; } -export interface FileSharedEvent extends StringIndexed { +export interface FileSharedEvent { type: 'file_shared'; file_id: string; // TODO: incomplete, this should be a reference to a File shape from @slack/types @@ -330,7 +327,7 @@ export interface FileSharedEvent extends StringIndexed { }; } -export interface FileUnsharedEvent extends StringIndexed { +export interface FileUnsharedEvent { type: 'file_unshared'; file_id: string; // TODO: incomplete, this should be a reference to a File shape from @slack/types @@ -340,51 +337,51 @@ export interface FileUnsharedEvent extends StringIndexed { }; } -export interface GridMigrationFinishedEvent extends StringIndexed { +export interface GridMigrationFinishedEvent { type: 'grid_migration_finished'; enterprise_id: string; } -export interface GridMigrationStartedEvent extends StringIndexed { +export interface GridMigrationStartedEvent { type: 'grid_migration_started'; enterprise_id: string; } -export interface GroupArchiveEvent extends StringIndexed { +export interface GroupArchiveEvent { type: 'group_archive'; channel: string; } -export interface GroupCloseEvent extends StringIndexed { +export interface GroupCloseEvent { type: 'group_close'; user: string; channel: string; } -export interface GroupDeletedEvent extends StringIndexed { +export interface GroupDeletedEvent { type: 'group_deleted'; channel: string; } -export interface GroupHistoryChangedEvent extends StringIndexed { +export interface GroupHistoryChangedEvent { type: 'group_history_changed'; latest: string; ts: string; event_ts: string; } -export interface GroupLeftEvent extends StringIndexed { +export interface GroupLeftEvent { type: 'group_left'; channel: string; } -export interface GroupOpenEvent extends StringIndexed { +export interface GroupOpenEvent { type: 'group_open'; user: string; channel: string; } -export interface GroupRenameEvent extends StringIndexed { +export interface GroupRenameEvent { type: 'group_rename'; channel: { id: string; @@ -393,18 +390,18 @@ export interface GroupRenameEvent extends StringIndexed { }; } -export interface GroupUnarchiveEvent extends StringIndexed { +export interface GroupUnarchiveEvent { type: 'group_unarchive'; channel: string; } -export interface IMCloseEvent extends StringIndexed { +export interface IMCloseEvent { type: 'im_close'; user: string; channel: string; } -export interface IMCreatedEvent extends StringIndexed { +export interface IMCreatedEvent { type: 'im_created'; user: string; // TODO: incomplete, this should probably be a reference to a IM shape from @slack/types. can it just be a @@ -415,20 +412,20 @@ export interface IMCreatedEvent extends StringIndexed { }; } -export interface IMHistoryChangedEvent extends StringIndexed { +export interface IMHistoryChangedEvent { type: 'im_history_changed'; latest: string; ts: string; event_ts: string; } -export interface IMOpenEvent extends StringIndexed { +export interface IMOpenEvent { type: 'im_open'; user: string; channel: string; } -export interface InviteRequestedEvent extends StringIndexed { +export interface InviteRequestedEvent { type: 'invite_requested'; invite_request: { id: string; @@ -448,7 +445,7 @@ export interface InviteRequestedEvent extends StringIndexed { }; } -export interface LinkSharedEvent extends StringIndexed { +export interface LinkSharedEvent { type: 'link_shared'; channel: string; user: string; @@ -460,7 +457,7 @@ export interface LinkSharedEvent extends StringIndexed { }[]; } -export interface MemberJoinedChannelEvent extends StringIndexed { +export interface MemberJoinedChannelEvent { type: 'member_joined_channel'; user: string; channel: string; @@ -469,7 +466,7 @@ export interface MemberJoinedChannelEvent extends StringIndexed { inviter?: string; } -export interface MemberLeftChannelEvent extends StringIndexed { +export interface MemberLeftChannelEvent { type: 'member_left_channel'; user: string; channel: string; @@ -477,124 +474,9 @@ export interface MemberLeftChannelEvent extends StringIndexed { team: string; } -// TODO: this is just a draft of the actual message event -export interface MessageEvent extends StringIndexed { - type: 'message'; - channel: string; - user: string; - text?: string; - ts: string; - attachments?: MessageAttachment[]; - blocks?: (KnownBlock | Block)[]; - edited?: { - user: string; - ts: string; - }; - - // TODO: optional types that maybe should flow into other subtypes? - is_starred?: boolean; - pinned_to?: string[]; - reactions?: { - name: string; - count: number; - users: string[]; - }[]; -} - -export interface BotMessageEvent extends StringIndexed { - type: 'message'; - subtype: 'bot_message'; - ts: string; - text: string; - bot_id: string; - username?: string; - icons?: { - [size: string]: string; - }; - - // copied from MessageEvent - // TODO: is a user really optional? likely for things like IncomingWebhook authored messages - user?: string; - attachments?: MessageAttachment[]; - blocks?: (KnownBlock | Block)[]; - edited?: { - user: string; - ts: string; - }; -} - -export interface EKMAccessDeniedMessageEvent extends StringIndexed { - type: 'message'; - subtype: 'ekm_access_denied'; - ts: string; - text: string; // This will not have any meaningful content within - user: 'UREVOKEDU'; -} - -export interface MeMessageEvent extends StringIndexed { - type: 'message'; - subtype: 'me_message'; - channel: string; - user: string; - text: string; - ts: string; -} - -export interface MessageChangedEvent extends StringIndexed { - type: 'message'; - subtype: 'message_changed'; - hidden: true; - channel: string; - ts: string; - message: MessageEvent; // TODO: should this be the union of all message events with type 'message'? -} - -export interface MessageDeletedEvent extends StringIndexed { - type: 'message'; - subtype: 'message_deleted'; - hidden: true; - channel: string; - ts: string; - deleted_ts: string; -} - -export interface MessageRepliedEvent extends StringIndexed { - type: 'message'; - subtype: 'message_replied'; - hidden: true; - channel: string; - event_ts: string; - ts: string; - message: MessageEvent & { - // TODO: should this be the union of all message events with type 'message'? - thread_ts: string; - reply_count: number; - replies: MessageEvent[]; // TODO: should this be the union of all message events with type 'message'? - }; -} - -// the `reply_broadcast` message subtype is omitted because it is discontinued - -export interface ThreadBroadcastMessageEvent extends StringIndexed { - type: 'message'; - message: { - type: 'message'; - subtype: 'thread_broadcast'; - thread_ts: string; - user: string; - ts: string; - root: MessageEvent & { - // TODO: should this be the union of all message events with type 'message'? - thread_ts: string; - reply_count: number; - replies: MessageEvent[]; // TODO: should this be the union of all message events with type 'message'? - // TODO: unread_count doesn't appear in any other message event types, is this really the only place its included? - unread_count?: number; - }; - }; -} +export type MessageEvent = AllMessageEvents; -export interface PinAddedEvent extends StringIndexed { +export interface PinAddedEvent { type: 'pin_added'; user: string; channel_id: string; @@ -602,7 +484,7 @@ export interface PinAddedEvent extends StringIndexed { item: {}; } -export interface PinRemovedEvent extends StringIndexed { +export interface PinRemovedEvent { type: 'pin_removed'; user: string; channel_id: string; @@ -631,7 +513,7 @@ interface ReactionFileCommentItem { file: string; } -export interface ReactionAddedEvent extends StringIndexed { +export interface ReactionAddedEvent { type: 'reaction_added'; user: string; reaction: string; @@ -640,7 +522,7 @@ export interface ReactionAddedEvent extends StringIndexed { event_ts: string; } -export interface ReactionRemovedEvent extends StringIndexed { +export interface ReactionRemovedEvent { type: 'reaction_removed'; user: string; reaction: string; @@ -654,7 +536,7 @@ export interface ReactionRemovedEvent extends StringIndexed { // NOTE: `resources_added`, `resources_removed`, `scope_denied`, `scope_granted`, are left out because they are // deprecated as part of the Workspace Apps Developer Preview -export interface StarAddedEvent extends StringIndexed { +export interface StarAddedEvent { type: 'star_added'; user: string; // TODO: incomplete, items are of type message | file | file comment (deprecated) | channel | im | group @@ -663,7 +545,7 @@ export interface StarAddedEvent extends StringIndexed { event_ts: string; } -export interface StarRemovedEvent extends StringIndexed { +export interface StarRemovedEvent { type: 'star_removed'; user: string; // TODO: incomplete, items are of type message | file | file comment (deprecated) | channel | im | group @@ -672,7 +554,7 @@ export interface StarRemovedEvent extends StringIndexed { event_ts: string; } -export interface SubteamCreated extends StringIndexed { +export interface SubteamCreated { type: 'subteam_created'; // TODO: incomplete, this should probably be a reference to a Usergroup shape from @slack/types. // https://api.slack.com/types/usergroup @@ -681,7 +563,7 @@ export interface SubteamCreated extends StringIndexed { }; } -export interface SubteamMembersChanged extends StringIndexed { +export interface SubteamMembersChanged { type: 'subteam_members_changed'; subteam_id: string; team_id: string; @@ -693,17 +575,17 @@ export interface SubteamMembersChanged extends StringIndexed { removed_users_count: number; } -export interface SubteamSelfAddedEvent extends StringIndexed { +export interface SubteamSelfAddedEvent { type: 'subteam_self_added'; subteam_id: string; } -export interface SubteamSelfRemovedEvent extends StringIndexed { +export interface SubteamSelfRemovedEvent { type: 'subteam_self_removed'; subteam_id: string; } -export interface SubteamUpdatedEvent extends StringIndexed { +export interface SubteamUpdatedEvent { type: 'subteam_updated'; // TODO: incomplete, this should probably be a reference to a Usergroup shape from @slack/types. // https://api.slack.com/types/usergroup @@ -712,25 +594,25 @@ export interface SubteamUpdatedEvent extends StringIndexed { }; } -export interface TeamDomainChangedEvent extends StringIndexed { +export interface TeamDomainChangedEvent { type: 'team_domain_changed'; url: string; domain: string; } -export interface TeamJoinEvent extends StringIndexed { +export interface TeamJoinEvent { type: 'team_join'; // TODO: incomplete, this should probably be a reference to a User shape from @slack/types. // https://api.slack.com/types/user user: {}; } -export interface TeamRenameEvent extends StringIndexed { +export interface TeamRenameEvent { type: 'team_rename'; name: string; } -export interface TokensRevokedEvent extends StringIndexed { +export interface TokensRevokedEvent { type: 'tokens_revoked'; tokens: { // TODO: are either or both of these optional? @@ -741,14 +623,14 @@ export interface TokensRevokedEvent extends StringIndexed { // NOTE: url_verification does not use the envelope, but its also not interesting for an app developer. its omitted. -export interface UserChangeEvent extends StringIndexed { +export interface UserChangeEvent { type: 'user_change'; // TODO: incomplete, this should probably be a reference to a User shape from @slack/types. // https://api.slack.com/types/user user: {}; } -export interface WorkflowStepExecuteEvent extends StringIndexed { +export interface WorkflowStepExecuteEvent { type: 'workflow_step_execute'; callback_id: string; workflow_step: { diff --git a/src/types/events/message-events.ts b/src/types/events/message-events.ts new file mode 100644 index 000000000..71ebfe4dd --- /dev/null +++ b/src/types/events/message-events.ts @@ -0,0 +1,128 @@ +import { MessageAttachment, KnownBlock, Block } from '@slack/types'; + +export type MessageEvent = + | GenericMessageEvent + | BotMessageEvent + | EKMAccessDeniedMessageEvent + | MeMessageEvent + | MessageChangedEvent + | MessageDeletedEvent + | MessageRepliedEvent + | ThreadBroadcastMessageEvent; + +export interface GenericMessageEvent { + type: 'message'; + subtype: undefined; + channel: string; + user: string; + text?: string; + ts: string; + attachments?: MessageAttachment[]; + blocks?: (KnownBlock | Block)[]; + edited?: { + user: string; + ts: string; + }; + + // TODO: optional types that maybe should flow into other subtypes? + is_starred?: boolean; + pinned_to?: string[]; + reactions?: { + name: string; + count: number; + users: string[]; + }[]; +} + +export interface BotMessageEvent { + type: 'message'; + subtype: 'bot_message'; + ts: string; + text: string; + bot_id: string; + username?: string; + icons?: { + [size: string]: string; + }; + + // copied from MessageEvent + // TODO: is a user really optional? likely for things like IncomingWebhook authored messages + user?: string; + attachments?: MessageAttachment[]; + blocks?: (KnownBlock | Block)[]; + edited?: { + user: string; + ts: string; + }; +} + +export interface EKMAccessDeniedMessageEvent { + type: 'message'; + subtype: 'ekm_access_denied'; + ts: string; + text: string; // This will not have any meaningful content within + user: 'UREVOKEDU'; +} + +export interface MeMessageEvent { + type: 'message'; + subtype: 'me_message'; + channel: string; + user: string; + text: string; + ts: string; +} + +export interface MessageChangedEvent { + type: 'message'; + subtype: 'message_changed'; + hidden: true; + channel: string; + ts: string; + message: MessageEvent; // TODO: should this be the union of all message events with type 'message'? +} + +export interface MessageDeletedEvent { + type: 'message'; + subtype: 'message_deleted'; + hidden: true; + channel: string; + ts: string; + deleted_ts: string; +} + +export interface MessageRepliedEvent { + type: 'message'; + subtype: 'message_replied'; + hidden: true; + channel: string; + event_ts: string; + ts: string; + message: MessageEvent & { + // TODO: should this be the union of all message events with type 'message'? + thread_ts: string; + reply_count: number; + replies: MessageEvent[]; // TODO: should this be the union of all message events with type 'message'? + }; +} + +// the `reply_broadcast` message subtype is omitted because it is discontinued + +export interface ThreadBroadcastMessageEvent { + type: 'message'; + subtype: undefined; + message: { + type: 'message'; + subtype: 'thread_broadcast'; + thread_ts: string; + user: string; + ts: string; + root: (GenericMessageEvent | BotMessageEvent) & { + thread_ts: string; + reply_count: number; + replies: { user: string; ts: string; }[]; + // TODO: unread_count doesn't appear in any other message event types, is this really the only place its included? + unread_count?: number; + }; + }; +} diff --git a/types-tests/message.test-d.ts b/types-tests/message.test-d.ts new file mode 100644 index 000000000..97351cde1 --- /dev/null +++ b/types-tests/message.test-d.ts @@ -0,0 +1,16 @@ +import { expectError, expectType } from 'tsd'; +import { App, MessageEvent, BotMessageEvent } from '../'; + +const app = new App({ token: 'TOKEN', signingSecret: 'Signing Secret' }); + +expectType(app.message(async ({ message }) => { + expectType(message); + + if (message.subtype === undefined) { + expectError(message.user); + } else if (message.subtype === 'bot_message') { + expectType(message); + } + + await Promise.resolve(message); +})); From fe87175287d9dd397c5796c05ac6bf3cea4effff Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Fri, 11 Dec 2020 01:26:06 -0800 Subject: [PATCH 2/9] Refactor buildSource to resolve typechecking issues Once we removed StringIndexed from SlackEvent types, many property lookups became typechecking errors. I borrowed this refactor mostly from a WIP PR to improve our style/linting in a proactive effort to redue conflicts when that PR lands. The only changes I needed to add were the Org Apps changes that had landed on main. https://github.com/slackapi/bolt-js/pull/669 In a few cases, the event types had other issues that became apparent only after removing the aggressive casting we had before. Those are fixed. It's now evident that there's more type related problems with respect to the Authorize, its related types, especially the OrgApps variants. I'll try to fix these next. It's also not clear if buildSource needs the orgInstall argument. Will seek advice in code review for that. --- src/App.ts | 300 +++++++++++++++----------------- src/types/events/base-events.ts | 10 +- 2 files changed, 148 insertions(+), 162 deletions(-) diff --git a/src/App.ts b/src/App.ts index 1237ab68a..fc7c680f1 100644 --- a/src/App.ts +++ b/src/App.ts @@ -794,6 +794,7 @@ const validViewTypes = ['view_closed', 'view_submission']; /** * Helper which builds the data structure the authorize hook uses to provide tokens for the context. + * TODO: update signature for being able to return OrgAuthorizeSourceData as well */ function buildSource( type: IncomingEventType, @@ -803,166 +804,145 @@ function buildSource( ): AuthorizeSourceData | OrgAuthorizeSourceData { // NOTE: potentially something that can be optimized, so that each of these conditions isn't evaluated more than once. // if this makes it prettier, great! but we should probably check perf before committing to any specific optimization. - let source: AuthorizeSourceData | OrgAuthorizeSourceData; - // tslint:disable:max-line-length - if (orgInstall) { - source = { - teamId: - type === IncomingEventType.Event && - (body as SlackEventMiddlewareArgs['body']).authorizations !== undefined && - (body as SlackEventMiddlewareArgs['body']).authorizations!.length > 0 && - (body as SlackEventMiddlewareArgs['body']).authorizations![0].team_id !== null - ? ((body as SlackEventMiddlewareArgs['body']).authorizations![0].team_id as string) - : type === IncomingEventType.Event || type === IncomingEventType.Command - ? ((body as (SlackEventMiddlewareArgs | SlackCommandMiddlewareArgs)['body']).team_id as string) - : (type === IncomingEventType.Action || - type === IncomingEventType.Options || - type === IncomingEventType.ViewAction || - type === IncomingEventType.Shortcut) && - body.team !== null - ? ((body as ( - | SlackActionMiddlewareArgs - | SlackOptionsMiddlewareArgs - | SlackViewMiddlewareArgs - | SlackShortcutMiddlewareArgs - )['body']).team!.id as string) - : (type === IncomingEventType.Action || - type === IncomingEventType.Options || - type === IncomingEventType.ViewAction || - type === IncomingEventType.Shortcut) && - body.user !== undefined - ? ((body as ( - | SlackActionMiddlewareArgs - | SlackOptionsMiddlewareArgs - | SlackViewMiddlewareArgs - | SlackShortcutMiddlewareArgs - )['body']).user.team_id as string) - : undefined, - // TODO: double check payloads for event and command below - enterpriseId: - type === IncomingEventType.Event && - (body as SlackEventMiddlewareArgs['body']).authorizations !== undefined && - (body as SlackEventMiddlewareArgs['body']).authorizations!.length > 0 && - (body as SlackEventMiddlewareArgs['body']).authorizations![0].enterprise_id !== null - ? ((body as SlackEventMiddlewareArgs['body']).authorizations![0].enterprise_id as string) - : type === IncomingEventType.Event || type === IncomingEventType.Command - ? ((body as (SlackEventMiddlewareArgs | SlackCommandMiddlewareArgs)['body']).enterprise_id as string) - : type === IncomingEventType.Action || - type === IncomingEventType.Options || - type === IncomingEventType.ViewAction || - type === IncomingEventType.Shortcut - ? ((body as ( - | SlackActionMiddlewareArgs - | SlackOptionsMiddlewareArgs - | SlackViewMiddlewareArgs - | SlackShortcutMiddlewareArgs - )['body']).enterprise!.id as string) - : assertNever(type), - userId: - type === IncomingEventType.Event - ? typeof (body as SlackEventMiddlewareArgs['body']).event.user === 'string' - ? ((body as SlackEventMiddlewareArgs['body']).event.user as string) - : typeof (body as SlackEventMiddlewareArgs['body']).event.user === 'object' - ? ((body as SlackEventMiddlewareArgs['body']).event.user.id as string) - : (body as SlackEventMiddlewareArgs['body']).event.channel !== undefined && - (body as SlackEventMiddlewareArgs['body']).event.channel.creator !== undefined - ? ((body as SlackEventMiddlewareArgs['body']).event.channel.creator as string) - : (body as SlackEventMiddlewareArgs['body']).event.subteam !== undefined && - (body as SlackEventMiddlewareArgs['body']).event.subteam.created_by !== undefined - ? ((body as SlackEventMiddlewareArgs['body']).event.subteam.created_by as string) - : undefined - : type === IncomingEventType.Action || - type === IncomingEventType.Options || - type === IncomingEventType.ViewAction || - type === IncomingEventType.Shortcut - ? ((body as (SlackActionMiddlewareArgs | SlackOptionsMiddlewareArgs | SlackViewMiddlewareArgs)['body']).user - .id as string) - : type === IncomingEventType.Command - ? ((body as SlackCommandMiddlewareArgs['body']).user_id as string) - : undefined, - conversationId: channelId, - isEnterpriseInstall: true, - }; - } else { - source = { - teamId: - type === IncomingEventType.Event && - (body as SlackEventMiddlewareArgs['body']).authorizations !== undefined && - (body as SlackEventMiddlewareArgs['body']).authorizations!.length > 0 && - (body as SlackEventMiddlewareArgs['body']).authorizations![0].team_id !== null - ? ((body as SlackEventMiddlewareArgs['body']).authorizations![0].team_id as string) - : type === IncomingEventType.Event || type === IncomingEventType.Command - ? ((body as (SlackEventMiddlewareArgs | SlackCommandMiddlewareArgs)['body']).team_id as string) - : type === IncomingEventType.Action || - type === IncomingEventType.Options || - type === IncomingEventType.ViewAction || - type === IncomingEventType.Shortcut - ? ((body as ( - | SlackActionMiddlewareArgs - | SlackOptionsMiddlewareArgs - | SlackViewMiddlewareArgs - | SlackShortcutMiddlewareArgs - )['body']).team!.id as string) - : assertNever(type), - enterpriseId: - type === IncomingEventType.Event && - (body as SlackEventMiddlewareArgs['body']).authorizations !== undefined && - (body as SlackEventMiddlewareArgs['body']).authorizations!.length > 0 && - (body as SlackEventMiddlewareArgs['body']).authorizations![0].enterprise_id !== null - ? ((body as SlackEventMiddlewareArgs['body']).authorizations![0].enterprise_id as string) - : type === IncomingEventType.Event || type === IncomingEventType.Command - ? ((body as (SlackEventMiddlewareArgs | SlackCommandMiddlewareArgs)['body']).enterprise_id as string) - : (type === IncomingEventType.Action || - type === IncomingEventType.Options || - type === IncomingEventType.ViewAction || - type === IncomingEventType.Shortcut) && - body.team !== null - ? ((body as ( - | SlackActionMiddlewareArgs - | SlackOptionsMiddlewareArgs - | SlackViewMiddlewareArgs - | SlackShortcutMiddlewareArgs - )['body']).team!.enterprise_id as string) - : (type === IncomingEventType.Action || - type === IncomingEventType.Options || - type === IncomingEventType.ViewAction || - type === IncomingEventType.Shortcut) && - body.enterprise !== undefined - ? ((body as ( - | SlackActionMiddlewareArgs - | SlackOptionsMiddlewareArgs - | SlackViewMiddlewareArgs - | SlackShortcutMiddlewareArgs - )['body']).enterprise!.id as string) - : undefined, - userId: - type === IncomingEventType.Event - ? typeof (body as SlackEventMiddlewareArgs['body']).event.user === 'string' - ? ((body as SlackEventMiddlewareArgs['body']).event.user as string) - : typeof (body as SlackEventMiddlewareArgs['body']).event.user === 'object' - ? ((body as SlackEventMiddlewareArgs['body']).event.user.id as string) - : (body as SlackEventMiddlewareArgs['body']).event.channel !== undefined && - (body as SlackEventMiddlewareArgs['body']).event.channel.creator !== undefined - ? ((body as SlackEventMiddlewareArgs['body']).event.channel.creator as string) - : (body as SlackEventMiddlewareArgs['body']).event.subteam !== undefined && - (body as SlackEventMiddlewareArgs['body']).event.subteam.created_by !== undefined - ? ((body as SlackEventMiddlewareArgs['body']).event.subteam.created_by as string) - : undefined - : type === IncomingEventType.Action || - type === IncomingEventType.Options || - type === IncomingEventType.ViewAction || - type === IncomingEventType.Shortcut - ? ((body as (SlackActionMiddlewareArgs | SlackOptionsMiddlewareArgs | SlackViewMiddlewareArgs)['body']).user - .id as string) - : type === IncomingEventType.Command - ? ((body as SlackCommandMiddlewareArgs['body']).user_id as string) - : undefined, - conversationId: channelId, - isEnterpriseInstall: false, - }; - } - // tslint:enable:max-line-length - return source; + + const teamId: string | undefined = (() => { + if (type === IncomingEventType.Event) { + const bodyAsEvent = body as SlackEventMiddlewareArgs['body']; + if (Array.isArray(bodyAsEvent.authorizations) + && bodyAsEvent.authorizations[0] !== undefined + && bodyAsEvent.authorizations[0].team_id !== null + ) { + return bodyAsEvent.authorizations[0].team_id; + } + return bodyAsEvent.team_id; + } + + if (type === IncomingEventType.Command) { + return (body as SlackCommandMiddlewareArgs['body']).team_id; + } + + if (type === IncomingEventType.Action + || type === IncomingEventType.Options + || type === IncomingEventType.ViewAction + || type === IncomingEventType.Shortcut + ) { + const bodyAsActionOrOptionsOrViewActionOrShortcut = body as ( + | SlackActionMiddlewareArgs + | SlackOptionsMiddlewareArgs + | SlackViewMiddlewareArgs + | SlackShortcutMiddlewareArgs + )['body']; + + // When the app is installed using org-wide deployment, team property will be null + // TODO: is the orgInstall property necessary? + if (bodyAsActionOrOptionsOrViewActionOrShortcut.team !== null) { + return bodyAsActionOrOptionsOrViewActionOrShortcut.team.id; + } + + // This is the only place where this function might return undefined + return bodyAsActionOrOptionsOrViewActionOrShortcut.user.team_id; + } + + return assertNever(type); + })(); + + + const enterpriseId: string | undefined = (() => { + if (type === IncomingEventType.Event) { + const bodyAsEvent = body as SlackEventMiddlewareArgs['body']; + if (Array.isArray(bodyAsEvent.authorizations) + && bodyAsEvent.authorizations[0] !== undefined + && bodyAsEvent.authorizations[0].enterprise_id !== null + ) { + return bodyAsEvent.authorizations[0].enterprise_id; + } + return bodyAsEvent.team_id; + } + + if (type === IncomingEventType.Command) { + return (body as SlackCommandMiddlewareArgs['body']).enterprise_id; + } + + if (type === IncomingEventType.Action + || type === IncomingEventType.Options + || type === IncomingEventType.ViewAction + || type === IncomingEventType.Shortcut + ) { + // NOTE: no type system backed exhaustiveness check within this group of incoming event types + const bodyAsActionOrOptionsOrViewActionOrShortcut = body as ( + | SlackActionMiddlewareArgs + | SlackOptionsMiddlewareArgs + | SlackViewMiddlewareArgs + | SlackShortcutMiddlewareArgs + )['body']; + + // When the app is installed using org-wide deployment, team property will be null + // TODO: is the orgInstall property necessary? + if (bodyAsActionOrOptionsOrViewActionOrShortcut.team !== null) { + return bodyAsActionOrOptionsOrViewActionOrShortcut.team.enterprise_id; + } + + if (bodyAsActionOrOptionsOrViewActionOrShortcut.enterprise !== undefined) { + return bodyAsActionOrOptionsOrViewActionOrShortcut.enterprise.id; + } + + return undefined; + } + + return assertNever(type); + })(); + + + const userId: string | undefined = (() => { + if (type === IncomingEventType.Event) { + // NOTE: no type system backed exhaustiveness check within this incoming event type + const { event } = (body as SlackEventMiddlewareArgs['body']); + if ('user' in event) { + if (typeof event.user === 'string') { + return event.user; + } + if (typeof event.user === 'object') { + return event.user.id; + } + } + if ('channel' in event && typeof event.channel !== 'string' && 'creator' in event.channel) { + return event.channel.creator; + } + if ('subteam' in event && event.subteam.created_by !== undefined) { + return event.subteam.created_by; + } + return undefined; + } + + if (type === IncomingEventType.Action + || type === IncomingEventType.Options + || type === IncomingEventType.ViewAction + || type === IncomingEventType.Shortcut + ) { + // NOTE: no type system backed exhaustiveness check within this incoming event type + const bodyAsActionOrOptionsOrViewActionOrShortcut = body as ( + | SlackActionMiddlewareArgs + | SlackOptionsMiddlewareArgs + | SlackViewMiddlewareArgs + | SlackShortcutMiddlewareArgs + )['body']; + return bodyAsActionOrOptionsOrViewActionOrShortcut.user.id; + } + + if (type === IncomingEventType.Command) { + return (body as SlackCommandMiddlewareArgs['body']).user_id; + } + + return assertNever(type); + })(); + + return { + teamId, + enterpriseId, + userId, + conversationId: channelId, + isEnterpriseInstall: orgInstall, + }; } function isBlockActionOrInteractiveMessageBody( diff --git a/src/types/events/base-events.ts b/src/types/events/base-events.ts index 365e04c93..56dc96a33 100644 --- a/src/types/events/base-events.ts +++ b/src/types/events/base-events.ts @@ -560,6 +560,7 @@ export interface SubteamCreated { // https://api.slack.com/types/usergroup subteam: { id: string; + created_by: string; }; } @@ -591,6 +592,7 @@ export interface SubteamUpdatedEvent { // https://api.slack.com/types/usergroup subteam: { id: string; + created_by: string; }; } @@ -604,7 +606,9 @@ export interface TeamJoinEvent { type: 'team_join'; // TODO: incomplete, this should probably be a reference to a User shape from @slack/types. // https://api.slack.com/types/user - user: {}; + user: { + id: string; + }; } export interface TeamRenameEvent { @@ -627,7 +631,9 @@ export interface UserChangeEvent { type: 'user_change'; // TODO: incomplete, this should probably be a reference to a User shape from @slack/types. // https://api.slack.com/types/user - user: {}; + user: { + id: string; + }; } export interface WorkflowStepExecuteEvent { From d8d344e6b191b158e78197d7e451f60fc6e642e4 Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Fri, 11 Dec 2020 02:37:30 -0800 Subject: [PATCH 3/9] Generalize Authorize and AuthorizeSourceData types By making these two types generic, we can resolve the typing issue in buildSource and a couple other places. This code still doesn't compile. May need to update singleTeamAuthorization. Getting close. --- src/App.ts | 48 ++++++++++++++++++------------------------------ 1 file changed, 18 insertions(+), 30 deletions(-) diff --git a/src/App.ts b/src/App.ts index fc7c680f1..9237f974c 100644 --- a/src/App.ts +++ b/src/App.ts @@ -69,8 +69,8 @@ export interface AppOptions { token?: AuthorizeResult['botToken']; // either token or authorize botId?: AuthorizeResult['botId']; // only used when authorize is not defined, shortcut for fetching botUserId?: AuthorizeResult['botUserId']; // only used when authorize is not defined, shortcut for fetching - authorize?: Authorize; // either token or authorize - orgAuthorize?: Authorize; + authorize?: Authorize; // either token or authorize + orgAuthorize?: Authorize; // either token or orgAuthorize receiver?: Receiver; logger?: Logger; logLevel?: LogLevel; @@ -81,26 +81,17 @@ export interface AppOptions { export { LogLevel, Logger } from '@slack/logger'; /** Authorization function - seeds the middleware processing and listeners with an authorization context */ -export interface Authorize { - (source: AuthorizeSourceData | OrgAuthorizeSourceData, body?: AnyMiddlewareArgs['body']): Promise; +export interface Authorize { + (source: AuthorizeSourceData, body?: AnyMiddlewareArgs['body']): Promise; } /** Authorization function inputs - authenticated data about an event for the authorization function */ -export interface AuthorizeSourceData { - teamId: string; - enterpriseId?: string; - userId?: string; - conversationId?: string; - isEnterpriseInstall?: boolean; -} - -/** Authorization function inputs - authenticated data about an event for the authorization function */ -export interface OrgAuthorizeSourceData { - enterpriseId: string; - teamId?: string; +export interface AuthorizeSourceData { + teamId: IsEnterpriseInstall extends true ? (string | undefined) : string; + enterpriseId: IsEnterpriseInstall extends true ? string : (string | undefined); userId?: string; conversationId?: string; - isEnterpriseInstall?: boolean; + isEnterpriseInstall: IsEnterpriseInstall; } /** Authorization function outputs - data that will be available as part of event processing */ @@ -169,10 +160,10 @@ export default class App { private logger: Logger; /** Authorize */ - private authorize!: Authorize; + private authorize!: Authorize; /** Org Authorize */ - private orgAuthorize!: Authorize; + private orgAuthorize!: Authorize; /** Global middleware chain */ private middleware: Middleware[]; @@ -298,8 +289,8 @@ export default class App { } else if ((authorize !== undefined || orgAuthorize !== undefined) && usingOauth) { throw new AppInitializationError(`Both authorize options and oauth installer options provided. ${tokenUsage}`); } else if (authorize === undefined && orgAuthorize === undefined && usingOauth) { - this.authorize = (this.receiver as ExpressReceiver).installer!.authorize as Authorize; - this.orgAuthorize = (this.receiver as ExpressReceiver).installer!.authorize as Authorize; + this.authorize = (this.receiver as ExpressReceiver).installer!.authorize; + this.orgAuthorize = (this.receiver as ExpressReceiver).installer!.authorize; } else if (authorize === undefined && orgAuthorize !== undefined && !usingOauth) { // only supporting org installs this.orgAuthorize = orgAuthorize; @@ -794,14 +785,13 @@ const validViewTypes = ['view_closed', 'view_submission']; /** * Helper which builds the data structure the authorize hook uses to provide tokens for the context. - * TODO: update signature for being able to return OrgAuthorizeSourceData as well */ -function buildSource( +function buildSource( type: IncomingEventType, channelId: string | undefined, body: AnyMiddlewareArgs['body'], - orgInstall: boolean, -): AuthorizeSourceData | OrgAuthorizeSourceData { + isEnterpriseInstall: IsEnterpriseInstall, +): AuthorizeSourceData { // NOTE: potentially something that can be optimized, so that each of these conditions isn't evaluated more than once. // if this makes it prettier, great! but we should probably check perf before committing to any specific optimization. @@ -834,7 +824,6 @@ function buildSource( )['body']; // When the app is installed using org-wide deployment, team property will be null - // TODO: is the orgInstall property necessary? if (bodyAsActionOrOptionsOrViewActionOrShortcut.team !== null) { return bodyAsActionOrOptionsOrViewActionOrShortcut.team.id; } @@ -877,7 +866,6 @@ function buildSource( )['body']; // When the app is installed using org-wide deployment, team property will be null - // TODO: is the orgInstall property necessary? if (bodyAsActionOrOptionsOrViewActionOrShortcut.team !== null) { return bodyAsActionOrOptionsOrViewActionOrShortcut.team.enterprise_id; } @@ -937,11 +925,11 @@ function buildSource( })(); return { - teamId, - enterpriseId, userId, + isEnterpriseInstall, + teamId: teamId as (IsEnterpriseInstall extends true ? (string | undefined) : string), + enterpriseId: enterpriseId as (IsEnterpriseInstall extends true ? string : (string | undefined)), conversationId: channelId, - isEnterpriseInstall: orgInstall, }; } From c1e4b2d52672a78faffb17ae4ae3296ed0c07472 Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Sun, 13 Dec 2020 21:21:18 -0800 Subject: [PATCH 4/9] Resolves TS compiler errors and fixes bugs. In both App.ts and builtin.ts, issues were resolve simply by refactoring to an equivalent form that is more type-safe. In App.ts specifically, processEvent() was refactored to reduce some code repetition as well. In helpers.ts this actually fixes bugs. The getTypeAndConversation() would have failed conversation lookup for the following event types: pin_added, pin_removed, call_rejected, channel_created, channel_rename, group_rename, im_created. --- src/App.ts | 129 ++++++++++++++++++-------------------- src/helpers.ts | 33 +++++++--- src/middleware/builtin.ts | 6 +- 3 files changed, 90 insertions(+), 78 deletions(-) diff --git a/src/App.ts b/src/App.ts index 9237f974c..7bb1a6fcc 100644 --- a/src/App.ts +++ b/src/App.ts @@ -279,9 +279,8 @@ export default class App { `token as well as authorize, orgAuthorize, or oauth installer options were provided. ${tokenUsage}`, ); } - this.authorize = singleTeamAuthorization(this.client, { botId, botUserId, botToken: token }); - // Todo: what should we do with orgAuthorize in a singeTeamAuthorize/token provided world? - this.orgAuthorize = singleTeamAuthorization(this.client, { botId, botUserId, botToken: token }); + this.authorize = singleAuthorization(this.client, { botId, botUserId, botToken: token }); + this.orgAuthorize = singleAuthorization(this.client, { botId, botUserId, botToken: token }); } else if (authorize === undefined && orgAuthorize === undefined && !usingOauth) { throw new AppInitializationError( `No token, no authorize, no orgAuthorize, and no oauth installer options provided. ${tokenUsage}`, @@ -549,40 +548,31 @@ export default class App { // From this point on, we assume that body is not just a key-value map, but one of the types of bodies we expect const bodyArg = body as AnyMiddlewareArgs['body']; - let authorizeResult; - let source; // Check if type event with the authorizations object or if it has a top level is_enterprise_install property - if ( - (type === IncomingEventType.Event && - (bodyArg as SlackEventMiddlewareArgs['body']).authorizations !== undefined && - (bodyArg as SlackEventMiddlewareArgs['body']).authorizations![0] !== undefined && - (bodyArg as SlackEventMiddlewareArgs['body']).authorizations![0].is_enterprise_install) || - bodyArg.is_enterprise_install === true || - bodyArg.is_enterprise_install === 'true' // command payloads have this as a string - ) { - // This is an org app - // Initialize context (shallow copy to enforce object identity separation) - source = buildSource(type, conversationId, bodyArg, true); - - try { - authorizeResult = await this.orgAuthorize(source, bodyArg); - } catch (error) { - this.logger.warn('Authorization of incoming event did not succeed. No listeners will be called.'); - error.code = 'slack_bolt_authorization_error'; - return this.handleError(error); - } - } else { - // This is not an org app - // Initialize context (shallow copy to enforce object identity separation) - source = buildSource(type, conversationId, bodyArg, false); - - try { - authorizeResult = await this.authorize(source, bodyArg); - } catch (error) { - this.logger.warn('Authorization of incoming event did not succeed. No listeners will be called.'); - error.code = 'slack_bolt_authorization_error'; - return this.handleError(error); + const isEnterpriseInstall = isBodyWithTypeEnterpriseInstall(bodyArg, type); + const source = buildSource(type, conversationId, bodyArg, isEnterpriseInstall); + + let authorizeResult: AuthorizeResult; + try { + if (source.isEnterpriseInstall) { + authorizeResult = this.orgAuthorize(source as AuthorizeSourceData, bodyArg); + } else { + authorizeResult = this.authorize(source as AuthorizeSourceData, bodyArg); } + } catch (error) { + this.logger.warn('Authorization of incoming event did not succeed. No listeners will be called.'); + error.code = 'slack_bolt_authorization_error'; + return this.handleError(error); + } + + // Try to set teamId from AuthorizeResult before using one from source + if (authorizeResult.teamId === undefined && source.teamId !== undefined) { + authorizeResult.teamId = source.teamId; + } + + // Try to set enterpriseId from AuthorizeResult before using one from source + if (authorizeResult.enterpriseId === undefined && source.enterpriseId !== undefined) { + authorizeResult.enterpriseId = source.enterpriseId; } const context: Context = { ...authorizeResult }; @@ -681,39 +671,23 @@ export default class App { // Get the client arg let { client } = this; const token = selectToken(context); - let teamId; - let enterpriseId; - - // Try to set teamId from AuthorizeResult before using one from source - if (authorizeResult.teamId !== undefined) { - teamId = authorizeResult.teamId; - } else if (source.teamId !== undefined) { - teamId = source.teamId; - } - - // Try to set enterpriseId from AuthorizeResult before using one from source - if (authorizeResult.enterpriseId !== undefined) { - enterpriseId = authorizeResult.enterpriseId; - } else if (source.enterpriseId !== undefined) { - enterpriseId = source.enterpriseId; - } if (token !== undefined) { let pool; const clientOptionsCopy = { ...this.clientOptions }; - if (teamId !== undefined) { - pool = this.clients[teamId]; + if (authorizeResult.teamId !== undefined) { + pool = this.clients[authorizeResult.teamId]; if (pool === undefined) { // eslint-disable-next-line no-multi-assign - pool = this.clients[teamId] = new WebClientPool(); + pool = this.clients[authorizeResult.teamId] = new WebClientPool(); } // Add teamId to clientOptions so it can be automatically added to web-api calls - clientOptionsCopy.teamId = teamId; - } else if (enterpriseId !== undefined) { - pool = this.clients[enterpriseId]; + clientOptionsCopy.teamId = authorizeResult.teamId; + } else if (authorizeResult.enterpriseId !== undefined) { + pool = this.clients[authorizeResult.enterpriseId]; if (pool === undefined) { // eslint-disable-next-line no-multi-assign - pool = this.clients[enterpriseId] = new WebClientPool(); + pool = this.clients[authorizeResult.enterpriseId] = new WebClientPool(); } } if (pool !== undefined) { @@ -933,6 +907,25 @@ function buildSource( }; } +function isBodyWithTypeEnterpriseInstall(body: AnyMiddlewareArgs['body'], type: IncomingEventType): boolean { + if (type === IncomingEventType.Event) { + const bodyAsEvent = body as SlackEventMiddlewareArgs['body']; + if (Array.isArray(bodyAsEvent.authorizations) && bodyAsEvent.authorizations[0] !== undefined) { + return !!bodyAsEvent.authorizations[0].is_enterprise_install; + } + } + // command payloads have this property set as a string + if (body.is_enterprise_install === 'true') { + return true; + } + // all remaining types have a boolean property + if (body.is_enterprise_install !== undefined) { + return body.is_enterprise_install; + } + // as a fallback we assume it's a single team installation (but this should never happen) + return false; +} + function isBlockActionOrInteractiveMessageBody( body: SlackActionMiddlewareArgs['body'], ): body is SlackActionMiddlewareArgs['body'] { @@ -947,23 +940,23 @@ function defaultErrorHandler(logger: Logger): ErrorHandler { }; } -function singleTeamAuthorization( +function singleAuthorization( client: WebClient, authorization: Partial & { botToken: Required['botToken'] }, -): Authorize { +): Authorize { // TODO: warn when something needed isn't found const identifiers: Promise<{ botUserId: string; botId: string }> = authorization.botUserId !== undefined && authorization.botId !== undefined ? Promise.resolve({ botUserId: authorization.botUserId, botId: authorization.botId }) : client.auth.test({ token: authorization.botToken }).then((result) => { - return { - botUserId: result.user_id as string, - botId: result.bot_id as string, - }; - }); - - return async () => { - return { botToken: authorization.botToken, ...(await identifiers) }; + return { + botUserId: result.user_id as string, + botId: result.bot_id as string, + }; + }); + + return async ({ isEnterpriseInstall }) => { + return { isEnterpriseInstall, botToken: authorization.botToken, ...(await identifiers) }; }; } diff --git a/src/helpers.ts b/src/helpers.ts index eddba945c..7b82301c2 100644 --- a/src/helpers.ts +++ b/src/helpers.ts @@ -29,15 +29,34 @@ export enum IncomingEventType { */ export function getTypeAndConversation(body: any): { type?: IncomingEventType; conversationId?: string } { if (body.event !== undefined) { - const eventBody = body as SlackEventMiddlewareArgs['body']; + const { event } = body as SlackEventMiddlewareArgs['body']; + + // Find conversationId + const conversationId: string | undefined = (() => { + let foundConversationId: string; + if ('channel' in event) { + if (typeof event.channel === 'string') { + foundConversationId = event.channel; + } else if ('id' in event.channel) { + foundConversationId = event.channel.id; + } + } + if ('channel_id' in event) { + foundConversationId = event.channel_id; + } + if ('item' in event && 'channel' in event.item) { + // no channel for reaction_added, reaction_removed, star_added, or star_removed with file or file_comment items + foundConversationId = event.item.channel; + } + // Using non-null assertion (!) because the alternative is to use `foundConversation: (string | undefined)`, which + // impedes the very useful type checker help above that ensures the value is only defined to strings, not + // undefined. This is safe when used in combination with the || operator with a default value. + return foundConversationId! || undefined; + })(); + return { + conversationId, type: IncomingEventType.Event, - conversationId: - eventBody.event.channel !== undefined - ? eventBody.event.channel - : eventBody.event.item !== undefined - ? eventBody.event.item.channel - : undefined, }; } if (body.command !== undefined) { diff --git a/src/middleware/builtin.ts b/src/middleware/builtin.ts index 466ebcfe6..91f921714 100644 --- a/src/middleware/builtin.ts +++ b/src/middleware/builtin.ts @@ -215,7 +215,7 @@ export function matchMessage( return async ({ event, context, next }) => { let tempMatches: RegExpMatchArray | null; - if (event.text === undefined) { + if (!('text' in event) || event.text === undefined) { return; } @@ -301,7 +301,7 @@ export function ignoreSelf(): Middleware { const eventsWhichShouldBeKept = ['member_joined_channel', 'member_left_channel']; const isEventShouldBeKept = eventsWhichShouldBeKept.includes(args.event.type); - if (botUserId !== undefined && args.event.user === botUserId && !isEventShouldBeKept) { + if (botUserId !== undefined && 'user' in args.event && args.event.user === botUserId && !isEventShouldBeKept) { return; } } @@ -334,7 +334,7 @@ export function directMention(): Middleware> ); } - if (message.text === undefined) { + if (!('text' in message) || message.text === undefined) { return; } From c1da2d7ca8bb30e13cd93bf7bc943c2e08e5723f Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Sun, 13 Dec 2020 21:29:09 -0800 Subject: [PATCH 5/9] prettier --- src/App.ts | 63 ++++++++++++++++-------------- src/types/events/message-events.ts | 2 +- 2 files changed, 34 insertions(+), 31 deletions(-) diff --git a/src/App.ts b/src/App.ts index 7bb1a6fcc..d44132d60 100644 --- a/src/App.ts +++ b/src/App.ts @@ -87,8 +87,8 @@ export interface Authorize { /** Authorization function inputs - authenticated data about an event for the authorization function */ export interface AuthorizeSourceData { - teamId: IsEnterpriseInstall extends true ? (string | undefined) : string; - enterpriseId: IsEnterpriseInstall extends true ? string : (string | undefined); + teamId: IsEnterpriseInstall extends true ? string | undefined : string; + enterpriseId: IsEnterpriseInstall extends true ? string : string | undefined; userId?: string; conversationId?: string; isEnterpriseInstall: IsEnterpriseInstall; @@ -772,9 +772,10 @@ function buildSource( const teamId: string | undefined = (() => { if (type === IncomingEventType.Event) { const bodyAsEvent = body as SlackEventMiddlewareArgs['body']; - if (Array.isArray(bodyAsEvent.authorizations) - && bodyAsEvent.authorizations[0] !== undefined - && bodyAsEvent.authorizations[0].team_id !== null + if ( + Array.isArray(bodyAsEvent.authorizations) && + bodyAsEvent.authorizations[0] !== undefined && + bodyAsEvent.authorizations[0].team_id !== null ) { return bodyAsEvent.authorizations[0].team_id; } @@ -785,10 +786,11 @@ function buildSource( return (body as SlackCommandMiddlewareArgs['body']).team_id; } - if (type === IncomingEventType.Action - || type === IncomingEventType.Options - || type === IncomingEventType.ViewAction - || type === IncomingEventType.Shortcut + if ( + type === IncomingEventType.Action || + type === IncomingEventType.Options || + type === IncomingEventType.ViewAction || + type === IncomingEventType.Shortcut ) { const bodyAsActionOrOptionsOrViewActionOrShortcut = body as ( | SlackActionMiddlewareArgs @@ -809,13 +811,13 @@ function buildSource( return assertNever(type); })(); - const enterpriseId: string | undefined = (() => { if (type === IncomingEventType.Event) { const bodyAsEvent = body as SlackEventMiddlewareArgs['body']; - if (Array.isArray(bodyAsEvent.authorizations) - && bodyAsEvent.authorizations[0] !== undefined - && bodyAsEvent.authorizations[0].enterprise_id !== null + if ( + Array.isArray(bodyAsEvent.authorizations) && + bodyAsEvent.authorizations[0] !== undefined && + bodyAsEvent.authorizations[0].enterprise_id !== null ) { return bodyAsEvent.authorizations[0].enterprise_id; } @@ -826,10 +828,11 @@ function buildSource( return (body as SlackCommandMiddlewareArgs['body']).enterprise_id; } - if (type === IncomingEventType.Action - || type === IncomingEventType.Options - || type === IncomingEventType.ViewAction - || type === IncomingEventType.Shortcut + if ( + type === IncomingEventType.Action || + type === IncomingEventType.Options || + type === IncomingEventType.ViewAction || + type === IncomingEventType.Shortcut ) { // NOTE: no type system backed exhaustiveness check within this group of incoming event types const bodyAsActionOrOptionsOrViewActionOrShortcut = body as ( @@ -854,11 +857,10 @@ function buildSource( return assertNever(type); })(); - const userId: string | undefined = (() => { if (type === IncomingEventType.Event) { // NOTE: no type system backed exhaustiveness check within this incoming event type - const { event } = (body as SlackEventMiddlewareArgs['body']); + const { event } = body as SlackEventMiddlewareArgs['body']; if ('user' in event) { if (typeof event.user === 'string') { return event.user; @@ -876,10 +878,11 @@ function buildSource( return undefined; } - if (type === IncomingEventType.Action - || type === IncomingEventType.Options - || type === IncomingEventType.ViewAction - || type === IncomingEventType.Shortcut + if ( + type === IncomingEventType.Action || + type === IncomingEventType.Options || + type === IncomingEventType.ViewAction || + type === IncomingEventType.Shortcut ) { // NOTE: no type system backed exhaustiveness check within this incoming event type const bodyAsActionOrOptionsOrViewActionOrShortcut = body as ( @@ -901,8 +904,8 @@ function buildSource( return { userId, isEnterpriseInstall, - teamId: teamId as (IsEnterpriseInstall extends true ? (string | undefined) : string), - enterpriseId: enterpriseId as (IsEnterpriseInstall extends true ? string : (string | undefined)), + teamId: teamId as IsEnterpriseInstall extends true ? string | undefined : string, + enterpriseId: enterpriseId as IsEnterpriseInstall extends true ? string : string | undefined, conversationId: channelId, }; } @@ -949,11 +952,11 @@ function singleAuthorization( authorization.botUserId !== undefined && authorization.botId !== undefined ? Promise.resolve({ botUserId: authorization.botUserId, botId: authorization.botId }) : client.auth.test({ token: authorization.botToken }).then((result) => { - return { - botUserId: result.user_id as string, - botId: result.bot_id as string, - }; - }); + return { + botUserId: result.user_id as string, + botId: result.bot_id as string, + }; + }); return async ({ isEnterpriseInstall }) => { return { isEnterpriseInstall, botToken: authorization.botToken, ...(await identifiers) }; diff --git a/src/types/events/message-events.ts b/src/types/events/message-events.ts index 71ebfe4dd..e13fab476 100644 --- a/src/types/events/message-events.ts +++ b/src/types/events/message-events.ts @@ -120,7 +120,7 @@ export interface ThreadBroadcastMessageEvent { root: (GenericMessageEvent | BotMessageEvent) & { thread_ts: string; reply_count: number; - replies: { user: string; ts: string; }[]; + replies: { user: string; ts: string }[]; // TODO: unread_count doesn't appear in any other message event types, is this really the only place its included? unread_count?: number; }; From aa6234ce5fbcf26e0936561ac811959ba60f6adf Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Sun, 13 Dec 2020 23:02:23 -0800 Subject: [PATCH 6/9] Fix up test for changes --- src/App.ts | 4 ++-- src/middleware/builtin.spec.ts | 19 +++++++++++++------ src/types/events/index.ts | 1 + 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/src/App.ts b/src/App.ts index d44132d60..1d2090f85 100644 --- a/src/App.ts +++ b/src/App.ts @@ -555,9 +555,9 @@ export default class App { let authorizeResult: AuthorizeResult; try { if (source.isEnterpriseInstall) { - authorizeResult = this.orgAuthorize(source as AuthorizeSourceData, bodyArg); + authorizeResult = await this.orgAuthorize(source as AuthorizeSourceData, bodyArg); } else { - authorizeResult = this.authorize(source as AuthorizeSourceData, bodyArg); + authorizeResult = await this.authorize(source as AuthorizeSourceData, bodyArg); } } catch (error) { this.logger.warn('Authorization of incoming event did not succeed. No listeners will be called.'); diff --git a/src/middleware/builtin.spec.ts b/src/middleware/builtin.spec.ts index 739acdb19..ecd7fbc77 100644 --- a/src/middleware/builtin.spec.ts +++ b/src/middleware/builtin.spec.ts @@ -5,10 +5,18 @@ import sinon from 'sinon'; import { ErrorCode, ContextMissingPropertyError } from '../errors'; import { Override, createFakeLogger } from '../test-helpers'; import rewiremock from 'rewiremock'; -import { SlackEventMiddlewareArgs, NextFn, Context, MessageEvent, SlackCommandMiddlewareArgs } from '../types'; +import { + SlackEventMiddlewareArgs, + NextFn, + Context, + SlackEvent, + MessageEvent, + SlackCommandMiddlewareArgs, +} from '../types'; import { onlyCommands, onlyEvents, matchCommandName, matchEventType, subtype } from './builtin'; import { SlashCommand } from '../types/command'; -import { SlackEvent, AppMentionEvent, BotMessageEvent } from '../types/events'; +import { AppMentionEvent } from '../types/events'; +import { GenericMessageEvent } from '../types/events/message-events'; import { WebClient } from '@slack/web-api'; import { Logger } from '@slack/logger'; @@ -688,8 +696,8 @@ async function importBuiltin(overrides: Override = {}): Promise import('./builtin'), overrides); } -function createFakeMessageEvent(content: string | MessageEvent['blocks'] = ''): MessageEvent { - const event: Partial = { +function createFakeMessageEvent(content: string | GenericMessageEvent['blocks'] = ''): MessageEvent { + const event: Partial = { type: 'message', channel: 'CHANNEL_ID', user: 'USER_ID', @@ -739,10 +747,9 @@ const appMentionEvent: AppMentionEvent = { event_ts: '123.123', }; -const botMessageEvent: BotMessageEvent & MessageEvent = { +const botMessageEvent: MessageEvent = { type: 'message', subtype: 'bot_message', - channel: 'C1234567', user: 'U1234567', ts: '123.123', text: 'this is my message', diff --git a/src/types/events/index.ts b/src/types/events/index.ts index 9ff239a83..94b8556b0 100644 --- a/src/types/events/index.ts +++ b/src/types/events/index.ts @@ -1,4 +1,5 @@ export * from './base-events'; +export { BotMessageEvent } from './message-events'; import { SlackEvent, BasicSlackEvent } from './base-events'; import { StringIndexed } from '../helpers'; import { SayFn } from '../utilities'; From 39b8fd2094c2e3fa4b9c3fdbf3ba50d8de4c7ec4 Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Sun, 13 Dec 2020 23:59:52 -0800 Subject: [PATCH 7/9] expose GenericMessageEvent to ease fixing broken builds --- src/types/events/index.ts | 2 +- src/types/events/message-events.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/types/events/index.ts b/src/types/events/index.ts index 94b8556b0..167829b8c 100644 --- a/src/types/events/index.ts +++ b/src/types/events/index.ts @@ -1,5 +1,5 @@ export * from './base-events'; -export { BotMessageEvent } from './message-events'; +export { BotMessageEvent, GenericMessageEvent } from './message-events'; import { SlackEvent, BasicSlackEvent } from './base-events'; import { StringIndexed } from '../helpers'; import { SayFn } from '../utilities'; diff --git a/src/types/events/message-events.ts b/src/types/events/message-events.ts index e13fab476..dc5120d4d 100644 --- a/src/types/events/message-events.ts +++ b/src/types/events/message-events.ts @@ -79,7 +79,7 @@ export interface MessageChangedEvent { hidden: true; channel: string; ts: string; - message: MessageEvent; // TODO: should this be the union of all message events with type 'message'? + message: MessageEvent; } export interface MessageDeletedEvent { From 8266e6857f1602d9f98ff29e5b780cab4de2899e Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Thu, 17 Dec 2020 04:55:17 -0800 Subject: [PATCH 8/9] Update src/App.ts Co-authored-by: Steve Gill --- src/App.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/App.ts b/src/App.ts index 1d2090f85..ae056a99c 100644 --- a/src/App.ts +++ b/src/App.ts @@ -821,7 +821,7 @@ function buildSource( ) { return bodyAsEvent.authorizations[0].enterprise_id; } - return bodyAsEvent.team_id; + return bodyAsEvent.enterprise_id; } if (type === IncomingEventType.Command) { From 86340c9d64dd7e3178a1613edbac6eada6ae40ed Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Mon, 4 Jan 2021 12:30:57 -0800 Subject: [PATCH 9/9] Update src/App.ts Co-authored-by: Steve Gill --- src/App.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/App.ts b/src/App.ts index ae056a99c..1197dce9e 100644 --- a/src/App.ts +++ b/src/App.ts @@ -842,15 +842,15 @@ function buildSource( | SlackShortcutMiddlewareArgs )['body']; + if (bodyAsActionOrOptionsOrViewActionOrShortcut.enterprise !== undefined) { + return bodyAsActionOrOptionsOrViewActionOrShortcut.enterprise.id; + } + // When the app is installed using org-wide deployment, team property will be null if (bodyAsActionOrOptionsOrViewActionOrShortcut.team !== null) { return bodyAsActionOrOptionsOrViewActionOrShortcut.team.enterprise_id; } - if (bodyAsActionOrOptionsOrViewActionOrShortcut.enterprise !== undefined) { - return bodyAsActionOrOptionsOrViewActionOrShortcut.enterprise.id; - } - return undefined; }