-
Notifications
You must be signed in to change notification settings - Fork 381
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
MSC4148: Permitting HTTP(S) URLs for SSO IdP icons #4148
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation requirements:
- None required, in my opinion. This MSC is logically possible to implement considering a client's use of the media download and thumbnail APIs.
|
||
## Proposal | ||
|
||
`icon` for the [`m.login.sso` flow schema](https://spec.matrix.org/v1.10/client-server-api/#definition-mloginsso-flow-schema) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to bikeshed too much on an MSC that's only meant as a temporary measure, but couldn't we stick the HTTP(S) URI in a different property, maybe something stupid like icon_location
? This would allow both old and new clients to work properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it'd materially improve the experience, honestly. Clients which don't update will still get broken icons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I concur that this change will break clients. Clients currently expect mxc urls and actively prevent loading https urls. Changing the same property to https will break existing clients and prevent even the fallback to allow loading old media using the old endpoints. Meanwhile I see no goo reason not to use a different property name for these urls. Clients which don't update won't get broken icons immediately without this MSC. With this MSC they will. And servers can even provide a quality of implementation "feature" and allow uploading media for unauthenticated access for the idp urls. So this should really be a separate property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a separate property would lead to clients not showing avatars, which it sounds like would also happen on clients not prepared for non-mxc URLs in the existing field. While it's a technically breaking change, the effective behaviour is the same regardless of the approach.
Note: clients which crash because of non-mxc URLs being present should be fixed regardless of this MSC's direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea is if you provide both it allows clients to migrate more gracefully by a server giving both an MXC and HTTPS URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This thread should probably be resolved one way or the other, so I'll add as concern for it. For what it's worth, having it as a separate field seems sensible to me because servers can send both as a transition period. Since the HTTP(S) icon will only support a single size, you could even call it icon_64
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference would be a separate field, but I don't think it's a huge issue either way, given it's a temporary measure, and just aesthetic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use a separate field here, I wonder what the implementation would look like. Currently, Synapse passes the idp_icon
setting from its config straight though (see doc, which says "must be an MXC URI"). To take advantage of separate fields here, we'd need to support separate fields in Synapse's config, which seems like it is complicating matters for the average Synapse admin.
I also see @turt2live's point that it probably won't help much in practice. Either: your server allows the mxc:
uri, in which case send that; else it doesn't, in which case what's the point of a separate field. The only reason to send both is to help you keep track of the number of old clients that aren't honoring the new field (and thus presumably don't support https:
uris).
IMHO the downsides of a separate field outweigh the upsides.
-0.5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An extra field means extra maintenance, on both (client and server) sides. Given that it's mainly an aesthetic issue, I'm fine with temporary limited breakage as long as we get straight to the to-be state. We have more than enough legacy cruft in the protocol already, let's not add to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory OIDC is expected this year, but I'm not tracking that work. @hughns do you have insight here?
This seems reasonable to me. @mscbot fcp merge |
Team member @mscbot has proposed to merge this. The next step is review by the rest of the tagged people: Concerns:
Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for information about what commands tagged team members can give me. |
@mscbot concern decide on rename vs separate field |
## Potential issues | ||
|
||
Existing clients may block non-`mxc://` URI usage and show no icon. Though this can lead to subpar | ||
user experience, the actual impact is expected to be temporary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably worth noting that mxc
URLs also give the ability for clients to request different sized thumbnails, and this MSC effectively hard codes the quality of the thumbnail to be used.
1. `mxc://` URI usage is *deprecated*, [pending removal](https://spec.matrix.org/v1.10/#deprecation-policy). | ||
2. HTTPS and HTTP URLs are permitted. | ||
3. Servers SHOULD prefer to use HTTPS URLs over HTTP or MXC URLs. | ||
4. Icons SHOULD be 64x64 pixels in size, accounting for a wide range of screen resolutions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bleurgh. it seems crazy to mandate 64x64 for an icon - that's tiny. macOS icons are 512x512 for instance (or 1024x1024 at retina). can we just delete this in favour of "Icons should be high enough resolution to render pleasantly on a wide variety of clients" or just leave it unspecified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(other than that, i'm fine with the msc tho)
@mscbot concern drop the overspecified 64x64 resolution requirement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having written a whole speech about the status of this MSC, I think the best thing for it is to cancel proposed-fcp and restart it once the new vs existing field discussion can conclude. The TLDR is client and server developers don't appear to be overly concerned with the broken behaviour, the author (me) doesn't have the time to maintain this, and given the debate on new vs existing fields it feels important to give SCT members a chance to re-review before sending the MSC along (necessitating FCP cancel and restart).
If there's no significant objections by about December 9th, 2024, I'll cancel the FCP and return this MSC to the backlog.
The longer speech is/was:
In practice, this MSC has all the checkboxes it needs to enter FCP, but has 2 notable blocking concerns preventing that from happening. It's been like this for about 5 months, which is very long. Meanwhile, client (and server) developers don't appear overly bothered with the lack of progress on this particular MSC, making it feel not as urgent as when it was opened. The author (me) has also had extremely limited availability to work on it, clearly.
The concern for removing the 64x64 requirement doesn't materially affect the MSC and can be resolved safely, however the concern about using a new field vs existing field may be breaking enough to cancel proposed-fcp and restart the process. Even if the MSC doesn't change, it doesn't feel fair to suddenly start a 5 day FCP with 5 month-old reviews - SCT members in particular should be given a chance to re-review before the MSC enters FCP.
Typically, we'd cancel and restart FCP if the MSC materially changes (it's a whole new MSC at that point), however we don't usually have such a long delay between achieving checkmarks and identifying a stalled MSC. Where we do want to give SCT members a chance to re-review an MSC, we typically be extremely loud about the MSC entering FCP, but this has had underwhelming success in the past (and also lights a fire where a fire isn't required). Because of the 5 month delay alone, it feels most appropriate to cancel proposed-fcp and restart it once/if the new vs existing field discussion resolves, giving all SCT members a chance to re-review and decide differently to last season's conclusions.
On the client/server implementation side: typically MSCs which are important to developers see some amount of traffic, either in the form of "drive by" comments or a ton of references hidden in the GitHub PR timeline. This MSC has neither, indicating it may not actually be solving a problem felt in practice. This feels untrue though, as surely there is a client out there showing a broken image on a button somewhere - maybe they're referencing the issue in a place where GitHub can't see it? Either way, this year end cleanup post offers developers a chance to say "hey, my users care!" more loudly, which will feed into the MSC's relative priority down the line.
Considering the expected priority from affected implementations and the proposed cancellation of FCP, it's likely the MSC will fall to the infinite backlog until either the author has enough free time (highly unlikely) or a number not much higher than zero developers advocate for the MSC on behalf of their users.
Thoughts from the community and SCT are welcome on this. It is proposed that the MSC cancel its FCP no sooner than December 9th, 2024 to ensure already-received checkmarks can be maintained if desired.
Rendered
In line with matrix-org/matrix-spec#1700, the following disclosure applies:
I am Director of Standards Development at The Matrix.org Foundation C.I.C., Matrix Spec Core Team (SCT) member, employed by Element, and operate the t2bot.io service. This proposal is written and published as a Trust & Safety team member allocated in full to the Foundation.
FCP tickyboxes