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.

Sign up for GitHub

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

Jump to bottom

MSC4148: Permitting HTTP(S) URLs for SSO IdP icons #4148

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

MSC4148: Permitting HTTP(S) URLs for SSO IdP icons #4148

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions proposals/4148-https-sso-icons.md
View file
Open in desktop
Copy link
Member Author

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
# MSC4148: Permitting HTTP(S) URLs for SSO IdP icons

The current [`m.login.sso` flow schema](https://spec.matrix.org/v1.10/client-server-api/#definition-mloginsso-flow-schema)
includes an optional `icon` field for clients to use when representing the Identity Provider.

Use of this icon currently relies on unauthenticated media download/thumbnail endpoints, which are
slated for deprecation and eventual removal through [MSC3916](https://github.com/matrix-org/matrix-spec-proposals/pull/3916).
Clients do not yet have credentials for the user and therefore may not be able to access these icons.
MSC3916 recommends that servers continue to allow the icons on the unauthenticated endpoints, though
this is only helpful so long as the endpoints exist.

This proposal introduces yet another temporary measure for handling these icons in the face of authenticated
media and the upcoming transition to OIDC: servers are permitted to use HTTP(S) URLs instead.

## Proposal

`icon` for the [`m.login.sso` flow schema](https://spec.matrix.org/v1.10/client-server-api/#definition-mloginsso-flow-schema)
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member

@dbkr dbkr Jun 18, 2024 •
edited
Loading

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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?

has the following changes:

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.
Copy link
Member

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.

Copy link
Member

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)

* This is because the thumbnail APIs are no longer accessible to clients with HTTP(S) URLs, so
"reasonable" sizes should be picked. This is not a strong requirement as deployment-specific
considerations may apply (larger sign-in button on a custom client, etc).

Some suggested spec text for `icon` to capture these changes would be:

> Optional. An HTTP(S) URL to provide an image/icon representing the IdP. Intended to be shown alongside
> `name` if provided.
>
> Servers SHOULD prefer HTTPS URLs over HTTP. `mxc://` URIs SHOULD NOT be used as their usage is deprecated
> here, though are permitted. Clients may have to use the deprecated, unauthenticated, media download
> and thumbnail endpoints to access content addressed by `mxc://` URIs.
>
> Images SHOULD be 64x64 pixels in size.

## 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.
Copy link
Member

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.


## Alternatives

* As it turns out, Element Web [overrides](https://github.com/matrix-org/matrix-react-sdk/blob/679b170bc5ae5eeb9c04c47fc5eeb663cb45b30c/src/components/views/elements/SSOButtons.tsx#L42)
the icon if it recognizes the `brand`. This behaviour prevents authenticated media from breaking the
icons, and avoids issues with HTTP(S) URL compatibility for the client. Other clients may wish to
pursue a similar approach, particularly for popular brands/sign in methods.

* This entire SSO login mechanism could be replaced, which is what is happening with OIDC (as of
writing). This MSC is intended to fill a gap between 'now' and when OIDC lands rather than being
a complete solution.

## Security considerations

MXC URIs are intended to keep media 'inside' Matrix to reduce opportunities for user information to
be harvested by external entities. Unlike random image uploads to a room though, the server the user
is about to register an account on is the only entity able to control these URLs, reducing the risk
surface. Clients may wish to look into an approach similar to Element Web for handling common icons,
and possibly ways of not using icons if they feel "too sketchy". For example, if the icons aren't
served by a subdomain of the Client-Server API URL (`https://*.matrix.org` in the case of
turt2live marked this conversation as resolved.
Show resolved Hide resolved
`https://matrix-client.matrix.org`), the icon could be omitted.

## Unstable prefix

Placing this MSC behind an unstable prefix becomes very difficult very quickly. Instead, clients and
servers are suggested to *not* implement this proposal until it is considered stable.

## Dependencies

No direct dependencies. [MSC3916](https://github.com/matrix-org/matrix-spec-proposals/pull/3916)
increases the need for this MSC, however.