MM-15340 Interactive dialogs without TriggerId

[scottleedavis]

Summary

These changes allow a trigger id to be omitted from an OpenDialogRequest, if a UserId is used instead.

Ticket Link

Fixes #10900

Related Pull Requests

• Has webapp changes MM-15340 provide InteractiveDialog hook to webapp plugins mattermost-webapp#2838

[hanzei]

We need to enforce request.TriggerId != "" on the API layer. Otherwise all integrations can open a dialog without a trigger ID.

[scottleedavis]

@hanzei Added the api filter I believe as requested.

[hanzei]

I'm 3/5 that this is not needed and we should not confuse "normal" integrations with this field.

When taking a broad look at this changes it seams to be unneeded to include this here. To me the plugin API method signature should be (userid string, request model.OpenDialogRequest) *model.AppError. What do you think about adding a new API method called e.g. OpenInteractiveDialogWithoutTriggerId and depreciate the old one? This seams to me the clean approach. @scottleedavis @jwilander Thoughts?

[scottleedavis]

I chose adding UserId to OpenDialogRequest to have as little impact as possible on the API definitions, however if it is preferred to have the new API method and the deprecated method, I'd be happy to do that. Though I would prefer less of a lengthy name than OpenInteractiveDialogWithoutTriggerId. Maybe OpenInteractiveDialogAsUser or... ? I don't have a better recommendation currently.

[hanzei]

My idea is, to consolidate the two methods when the next version of the plugin API is released and only have a method that doesn't need a trigger.

My problem with OpenInteractiveDialogAsUser is that it suggests that OpenInteractiveDialog doesn't opens the dialog as a user.

@levb @cpoile would like to here your thoughts on this discussion if you have the time.

[jwilander]

I think we need to think more carefully about this. As is these changes would allow anyone (including unauthed requests) to open a dialog for any user on the system. The reason that a trigger ID is used to is make sure that the user took an action in the app before opening a dialog for them.

[scottleedavis]

@jwilander The intent was to require trigger ID for API requests, but allow plugins to forgo it so that webapp generated events could also trigger the dialog. Besides a rogue plugin, I don't see how this opens up additional vulnerability though certainly don't have the knowledge of the entire system to make that call.

Would it be advisable to allow webapp plugins to generate triggerIds? If so we may be able to keep infrastructure intact, and pass the trigger id along.

[jwilander]

Ok I overlooked the change in api4/integration_action.go that would require the trigger ID at the API layer.

Regardless, we shouldn't need any server changes to let a web app plugin open the dialog. We should just expose a function to web app plugins that allows for them to open the dialog, providing whatever fields they want plus a response URL that presumably points to a server endpoint of the plugin.

[scottleedavis]

Sounds good @jwilander . I'll close this PR, and continue this conversation/work on mattermost/mattermost-webapp#2838 .

[hanzei]

I would like to discuss for a moment, why this shouldn't be fixed on the server part. Plugins usually just skip any authentication of there API requests. What is the difference between "normal" plugin API request and the request to open an interactive dialog?

[jwilander]

I'm ok with letting server plugins open interactive dialogs but is there really a need for that? I'd think it's the web app plugins that are going to want to open the dialog and it would just be annoying (and unnecessary traffic) to have to trigger it from the server.

[hanzei]

After think a bit longer about this, I didn't came up with a use case where a server plugin wants to open a dialog without a trigger.

Let's see if one comes up in the future.