Skip to content

Conversation

sandhose
Copy link
Member

@sandhose sandhose commented Mar 25, 2025

This deduplicates client registrations by hashing the client metadata, saving that hash in the database, and look for existing entries during registration.

This was relatively straightforward; I had although to make sure that the property order in the client metadata was stable, and therefore replaced the localized metadata hashmaps with indexmaps, which preserve insertion order.

Copy link

cloudflare-workers-and-pages bot commented Mar 25, 2025

Deploying matrix-authentication-service-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3f66940
Status: ✅  Deploy successful!
Preview URL: https://5800f4a7.matrix-authentication-service-docs.pages.dev
Branch Preview URL: https://quenting-client-reg-dedupe.matrix-authentication-service-docs.pages.dev

View logs

@sandhose sandhose force-pushed the quenting/client-reg-dedupe branch from 93fbb4f to b4ceb00 Compare March 25, 2025 12:35
@sandhose sandhose marked this pull request as ready for review March 25, 2025 13:04
@sandhose sandhose requested a review from reivilibre March 25, 2025 13:05
@sandhose sandhose added A-Client-Registration Related to OIDC Dynamic Client Registration T-Enhancement New feature of request labels Mar 25, 2025
This is done by using the indexmap crate to preserve insertion order for
localized fields.
@sandhose sandhose force-pushed the quenting/client-reg-dedupe branch from f5b609e to 4294dc8 Compare March 25, 2025 14:01
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

seems good otherwise

pub struct Localized<T> {
non_localized: T,
localized: HashMap<LanguageTag, T>,
localized: IndexMap<LanguageTag, T>,
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use a BTreeMap, which sorts by key order?

I'm just thinking the clients might be sending requests with HashMaps on their end, so key order on the wire is not guaranteed to be stable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because LanguageTag doesn't implement Ord, so I can't use a BTreeMap :(

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 just thinking the clients might be sending requests with HashMaps on their end, so key order on the wire is not guaranteed to be stable.

Sure, but that's on them. The deduplication is a best-effort thing anyway 🤷

Copy link
Contributor

@reivilibre reivilibre Mar 26, 2025

Choose a reason for hiding this comment

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

Hmm, I take what you mean, but I'm still not a fan of introducing some fun nondeterminism (even if it's the client's fault) even if it is just a 'best-effort' (or worst-effort as my supervisor liked to say) thing.

Can we maybe call .sort_by(|k1, _, k2, _| k1.as_str().cmp(k2.as_str())) on the IndexMap prior to serialising for hashing?

Copy link
Contributor

Choose a reason for hiding this comment

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

(or cheeky-PR a Ord impl to language_tags ;-))

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 added a sort on all the Localized<T> and Vec<T> properties in 3f66940

@sandhose sandhose requested a review from reivilibre March 26, 2025 13:31
@sandhose sandhose merged commit 76b9a6d into main Mar 26, 2025
21 checks passed
@sandhose sandhose deleted the quenting/client-reg-dedupe branch March 26, 2025 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Client-Registration Related to OIDC Dynamic Client Registration T-Enhancement New feature of request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants