Update m.id.phone to use 'phone' instead of 'number' #7687
Conversation
Fixes bug introduced in #1994
|
According to @turt2live Riot-web sends Riot-web is going to send both on their side. |
|
ref element-hq/element-web#14002 though I'm not going to be taking this on any time soon. edit: but someone else might |
|
Riot-Android apparently also uses the wrong identifier. Edit: this functionality is not currently implemented in RiotX. |
anoadragon453
force-pushed
the
anoa/fix_m_id_phone
branch
from
97a2305
to
91c1b70
Compare
|
@anoadragon453 Would it be reasonable to add unit tests for this? The changes look good though! |
Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
|
@clokep There are unit tests for this included in #7438: https://github.com/matrix-org/synapse/pull/7438/files#diff-3f3fca8f7ff4993ebce1ac5cf7695c64R662 Though tbh this is probably something that could benefit from a sytest as well. |
synapse/rest/client/v1/login.py
Outdated
| @@ -60,10 +60,18 @@ def login_id_thirdparty_from_phone(identifier): | |||
|
|
|||
| Returns: Login identifier dict of type 'm.id.threepid' | |||
| """ | |||
| if "country" not in identifier or "number" not in identifier: | |||
| if "country" not in identifier or ( | |||
| # XXX: We used to require `number` instead of `phone`. The spec | |||
There was a problem hiding this comment.
Why XXX? I usually reserve that for something that's an action, maybe something like:
The specification requires a
phonefield, but Synapse used to require anumberfield. Accept both for backwards compatibility.
There was a problem hiding this comment.
I guess it was more intended as a TODO, we should remove this later.
...but, we're probably never going to remove it, so I think your suggestion does work better.
Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
|
@anoadragon453 Also -- do you have a plan / strategy for how to let client teams know about this change? |
|
@clokep I've contacted Riot-web/mobile gangs already. Other than that, since we're adding backwards compatibility, I would just leave it up to the changelog. But I could drop a note in |
* commit '03619324f': Create a ListenerConfig object (#7681) Fix changelog wording 1.15.1 Wrap register_device coroutine in an ensureDeferred (#7684) Ensure the body is a string before comparing push rules. (#7701) Ensure etag is a string for GET room_keys/version response (#7691) Update m.id.phone to use 'phone' instead of 'number' (#7687) Fix "There was no active span when trying to log." error (#7698) Enable 3PID add/bind/unbind endpoints on r0 routes Discard RDATA from already seen positions. (#7648) Replace iteritems/itervalues/iterkeys with native versions. (#7692) Fix warnings about losing log context during UI auth. (#7688) Fix a typo when comparing the URI & method during UI Auth. (#7689) Remove "user_id" from GET /presence. (#7606) Increase the default SAML session expirary time to 15 minutes. (#7664) fix typo in sample_config.yaml (#7652) Take out a lock before modifying _CACHES (#7663) Add option to enable encryption by default for new rooms (#7639) Clean-up the fallback login code. (#7657)
The spec states that
m.id.phonerequires the fieldcountryandphone.In Synapse, we've been enforcing
countryandnumber.I am not currently sure whether this affects any client implementations.
This issue was introduced in #1994.