Skip to content

Change Capabilities format #6572

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

Merged
merged 3 commits into from
Oct 7, 2020

Conversation

konradkonrad
Copy link
Contributor

@konradkonrad konradkonrad commented Sep 30, 2020

Description

This changes the capabilities according to the proposed changes in #6503 (comment) and/or #6503 (comment)

Fixes: #6503

copy and past from the issue:

Encoding/Serialization for use in matrix avatar_url

"mxc://raiden.network/cap?{capabilities_url_encoded}"

where {capabilities_url_encoded} is the url query parameter encoding of capabilities.

Rules for url encoding:

  • bool values are encoded as "0" for False and "1" for True
  • other values are encoded as strings
  • lists of values are allowed

Deserialization / Decoding

The final interpretation of capability values is up to the receiving client, or rather the specified capability.

Implicit values / Handling of unknown values

  • intentionally omitting (falsy or whatever) known default values is discouraged. Client implementations are asked to explicitly state all known capabilities.
  • Client implementations have to deal with receiving new/unknown capabilities gracefully, i.e. they should expect the peer to act backwards compatible.
  • Client implementations have to deal with not receiving known capabilities gracefully, i.e. assume the peer implementation is going to exert legacy behavior and therefore act backwards compatible.

Previously discussed versioning is postponed until there is a need for truncation of capabilities.

Example

avatar_url = "mxc://raiden.network/cap?Delivered=0&Mediate=1&Receive=1&webRTC=1&list_capability=one&list_capability=two"
capabilities_decoded = {
 'Delivered': False,
 'Mediate': True,
 'Receive': True,
 'webRTC': True,
 'list_capability': ['one', 'two']
}

Rationale for the decision

  • proper url conform encoding is expected to avoid any sanitation issues when communicating with matrix servers
  • json+base64 encoding was dropped because it is harder to inspect by humans
  • url query parameters can be (de-)serialized by existing tooling/libraries
  • the boolean convention (0,1) was necessary because there is no native format for boolean values in url parameters

@konradkonrad konradkonrad force-pushed the capabilities_newformat branch 2 times, most recently from 3f1a071 to 53614b5 Compare September 30, 2020 09:51
@konradkonrad konradkonrad requested a review from ulope September 30, 2020 10:07
@konradkonrad konradkonrad force-pushed the capabilities_newformat branch 2 times, most recently from bd8d695 to 7cf8740 Compare October 6, 2020 14:27
@konradkonrad konradkonrad force-pushed the capabilities_newformat branch from 7cf8740 to c7fee87 Compare October 6, 2020 15:00
@andrevmatos
Copy link
Contributor

andrevmatos commented Oct 6, 2020

IMO, the '0' & '1' shouldn't be converted to bool, and instead handled as a number, where its truthness will do what's is supposed to on code which interprets these values as truthy or falsy. It should work though one way or another on the current cases (which expect only booleans), just leaving this here for completeness/correctness.

Copy link
Collaborator

@ulope ulope left a comment

Choose a reason for hiding this comment

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

LGTM.

Some nitpicks below.

This changes the (de-)serialization of capabilities that are broadcast
via synapse `avatar_url` to be url query parameters.

The new capabilities serialization format in detail:

Capabilities are serialized to `avatar_url` as

    mxc://raiden.network/cap?{capabilities_url_encoded}

The spec for `{capabilities_url_encoded}` is as follows:

- `bool` values are encoded as `'1' == True` and `'0' == False`
- other values are encoded as strings
- the strings `'0'` and `'1'` are deserialized as `bool` (see above).
- other strings are kept as they are
- multiple entries / list values are allowed
- final interpretation of any value is up to the client

Changes in detail:
- hand rolled serialization is removed
- truncation of falsy entries is dropped / all entries are explicit
- unknown capabilities will be preserved
- `parse_capabilities` is renamed to `deserialize_capabilities`
- deserialization errors are treated as empty capabilities
- adjusted unit tests
@konradkonrad konradkonrad force-pushed the capabilities_newformat branch 2 times, most recently from 28c861a to a207ddc Compare October 7, 2020 09:22
@konradkonrad konradkonrad force-pushed the capabilities_newformat branch from a207ddc to b75c6f8 Compare October 7, 2020 09:25
@ulope ulope merged commit 963551a into raiden-network:develop Oct 7, 2020
@konradkonrad konradkonrad deleted the capabilities_newformat branch October 7, 2020 10:21
karlb pushed a commit that referenced this pull request Oct 9, 2020
This ports protocol handshake (#3283 / #6503) capabilities from #6482 / #6572
back to the release branch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discuss format of capabilities
3 participants