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

Widgets, the spec #2764

Draft
wants to merge 46 commits into
base: old_master
Choose a base branch
from
Draft

Widgets, the spec #2764

wants to merge 46 commits into from

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Sep 2, 2020

Specifies MSC1236 - Widget API v2
Specifies MSC1354 - Always on-screen action
Specifies MSC1960 - OpenID Connect information exchange
Specifies MSC2765 - Widget avatars
Specifies MSC2774 - Widget ID template variable
To be reviewed with "Widgets, the book": matrix-org/matrix.org#825

For a rendered version, see the 'docs' status check near the merge button.

A brief history of widgets

Prior to MSC1236 there was a v1 widget API which never made it to the proposal system. The v2 implementation proposed by MSC1236 did pass proposal review at the time, however by today's standards it will have failed for a number of reasons. Primarily, the actual implementation of widgets has long since been done with im.vector.modular.widgets events rather than the m.widget(s) events specified in the MSC (at the time the implementation could use the m.* namespace for production, but modern MSCs/implementations cannot). As a result of this implementation, the actual behaviour, parameters, and requirements for widgets drifted far away from the MSC.

Integration managers, a concept not introduced or mentioned here, also slowly adopted their own widget spec which contributed to deviations from the MSC and even platforms. For example, with Element Web (then Vector and Riot) leading the widgets space, the mobile versions were not able to follow in the same footsteps and instead had to do additional/fewer chunks of functionality.

As such, there is currently no implementation whatsoever of this spec PR.

Caveats

This is not a typical spec PR in that it mixes proposed behaviour, MSC behaviour, and reality all into one PR. Each commit in this PR documents the deviations from the MSC and calls out any proposed behaviour. Where behaviour is proposed, it is arguable if it should get its own MSC (which can easily be done).

This spec PR is also entirely based upon the Element Web implementation and ignores any deviations or inconsistencies of the mobile clients. No other clients are known to have significant widget support, and thus were not considered.

In addition to all of the above, this spec PR does not include any specification for the send/receive event structure outside of the sticker events due to significant lack of implementation, conflict within the MSC itself, and functionally undesirable behaviour. As such, it has been broken out to its own MSC: #2762

This spec PR also does not have any definitions for integration manager widgets as proposed by #1957 - this is left as an exercise for MSC1956 - Integrations API.

Some API actions have additionally been excluded from this spec PR:

  • integration_manager_open - Integration managers aren't a thing, and this needs a formal MSC first.
  • get_openid / openid_credentials - These are covered by MSC1960
  • api_version - As mentioned by the versioning commit, this is presumed to have no useful value.

Reviewers: The commit history of this spec PR includes a bunch of widget types which are not in the final version. This is because a later commit removes them due to concerns over their presence in the spec (eg: there's nothing special about Google Docs, so we don't need to specify a widget type for it).

This spec PR further does not specify how a widget actually gets added to a room/user's account. This is left as a problem for the relevant implementations until an integrations API can be specified.

Release/review plan

Considering this is the largest spec PR we've ever done (7500ish words!), and the first time we've created a new specification with our new guidelines, this has some additional practices being applied to it (which have been arbitrarily decided upon).

Before spec core team review is requested:

  • [The Book] An implementation book is mandatory prior to this being merged to master.
    • This specification is written in a new style compared to the other specifications: instead of mixing implementation concerns with the specification, the implementation concerns have largely been left out. This is fully intended to force the creation of an implementation book.
  • Any missing sections to the specification need to be added.
    • Security Considerations for the Widget API
    • ... others as needed.
  • An implementation in Element Web is to be completed to validate both the implementation book and the spec itself.

After the above are all resolved:

  • At least 2 spec core team members will need to review this and approve of it.
  • After being merged to master, it will not be released for at least 2 months to give implementations a chance at testing it out and find problems.
    • If modifications need to be made, a judgement call will be made as to when the release will happen.
    • Other MSCs relating to widgets are welcome to land during this time to make it for the r0.1.0 target.

Other relevant points of interest are:

  • As mentioned, this specification is written in a way where the implementation notes have been left out. This should lead to a more lean spec with supporting documents to help client/widget authors along.
  • The spec is exhaustive in schemas to allow for easy code generation in implementations. This is so much of a priority it is mentioned in the API Standards.

Notes to reviewers

It's probably going to be best to read this commit-by-commit with frequent breaks. It's incredibly dense and almost certainly has typos, grammatical issues, and sentence structure problems. The end result can be seen from the 'docs' status check near the merge button, though consuming and reviewing this PR that way might be too painful. Note that some commits, like the aforementioned widget types, are effectively no-ops due to later removal.

Note to implementors (client/widget authors)

I do not suggest attempting to implement this specification until it has started the spec core team review at a minimum. It will be best to wait until this PR is merged before attempting a proper implementation. Element Web is somewhat exempt from this recommendation due to being my choice of client to validate this in, which is required for me to be comfortable with landing this spec.

No changes from MSC
The data object is completely different from the MSC due to implementation constraints. This spec matches the current implementation used by known integration managers.
No differences from MSC
No changes from the MSC
No changes from the MSC
There is disagreement between integration managers over whether this is supposed to be a `src` or `shareId`, however the MSC wants a `shareId` which at least 1 integration manager agrees with.
No changes from the MSC
There is disagreement between integration managers over whether this is supposed to be a `curl` or `url`, however the MSC wants a `url` which at least 1 integration manager agrees with.
No changes from the MSC
No changes from the MSC
These are extremely common in practice.
There are only a couple differences from the MSC here:
* Timeouts are not specified in the MSC, and neither is the requirement to have a request acknowledged. Both timeouts and acknowledgment requirements happen in practice, so they've been included here.
* The error response is slightly varied from the MSC due to misrepresentation and lack of usage. The MSC wants a JavaScript error object at the same level as the `message`, though in practice we include it under an `_error` key next to the `message`. Due to it being underscored, not used, and generally not translating well outside of web browsers, it's been omitted.
This makes for a slightly easier directory structure to manage
The only deviation from the MSC here is that custom capabilities are allowed to exist, because they do.
No deviations from the MSC
This diverges slightly from the MSC to match reality in that the MSC wants the widget to get all the permissions it asked for or be rejected, however in practice the client can deny some/all of the permissions without notifying the widget and the widget is expected to continue normally.

This deviation is generally fine as most capabilities are in the client->widget direction anyhow.
This has significant deviation from the MSC and practical implementation due to lack of validation. The MSC specifies the request/response schema as described in this commit, though the associated behaviour and timings are different.

In practice, no widget has implemented `supported_api_versions` and no client has ever requested it from a widget. This is expected to change with the specification being present. Additionally, widgets are technically able to send an `api_version` request to get the *current* version, though this is implied by the `supported_api_versions` response and thus not considered.

Tying the version to the spec release is something introduced by this commit, as are the restrictions about what can and cannot be sent by either side. This commit also introduces timing constraints that allow widgets/clients to check the other's versions at any time, even during session initialization, due to the potential need for the initialization sequence to change.

Further, this lumps `0.0.1` and `0.0.2` into the same version (which is expected to release as `0.1.0`) despite minor differences between the two. These differences are years old and effectively not important in the eyes of the spec.

It is arguable that this particular section should be an MSC on top of the widgets MSC rather than landing like this.
There are minor deviations from the MSC on this: 
* The action is different, as documented in the suggestions on the MSC.
* The response is different. The MSC wants a `success: true` parameter, however to maintain consistency this has been removed. Nothing uses this in practice.
@turt2live turt2live marked this pull request as draft September 2, 2020 22:29
All of these widgets can be represented safely as custom widgets, and thus need no specific behaviour.
Comment on lines +99 to +102
Invalid room widgets MUST NOT be shown to users. This is also how widgets are removed from a room:
send a new state event for the same widget ID with at least the ``url`` and/or ``type`` missing
from the event content. Once Matrix allows for state events to be properly deleted then doing so
to the widget state event will be just as valid to remove it from the room.
Copy link
Contributor

@deepbluev7 deepbluev7 Sep 3, 2020

Choose a reason for hiding this comment

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

What prevents us from using redactions to remove widgets? That this would only remove the latest state event?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing? In fact it's saying it's fine.

@turt2live turt2live changed the title Widgets Widgets, the spec Sep 4, 2020
@richvdh
Copy link
Member

richvdh commented Oct 9, 2020

I've not read this thoroughly, but at a skim it looks excellent - very clear and well written. Thank you for your work!

title: WidgetApiErrorResponse
description: |-
Schema definition for an error response in the Widget API. This is a regular ``WidgetApiResponse``
object with the ``response`` specified as follows.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be updated to say error and omit the response object in the WidgetApiErrorResponse.

Comment on lines +20 to +30
api:
type: string
enum: [fromWidget, toWidget]
description: |-
The API for which the request is to be sent over.
example: "fromWidget"
requestId:
type: string
description: |-
An opaque string which uniquely identifies the request in the context of the session.
example: "generated-id-1234"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it that the widget API uses camelCase in some prominent places like this? Could that still be changed, with some backwards-compatibility hacks? Matrix otherwise pretty consistently uses snake_case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
widgets anything to do with widgets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants