Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,11 @@ version = "0.27.5"
features = ["http1", "http2"]
default-features = false

# HashMap which preserves insertion order
[workspace.dependencies.indexmap]
version = "2.8.0"
features = ["serde"]

# Snapshot testing
[workspace.dependencies.insta]
version = "1.42.2"
Expand Down
2 changes: 1 addition & 1 deletion crates/handlers/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ chrono.workspace = true
elliptic-curve.workspace = true
hex.workspace = true
governor.workspace = true
indexmap = "2.8.0"
indexmap.workspace = true
pkcs8.workspace = true
psl = "2.1.96"
sha2.workspace = true
Expand Down
2 changes: 2 additions & 0 deletions crates/oauth2-types/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,11 @@ serde_with = { version = "3.12.0", features = ["chrono"] }
chrono.workspace = true
sha2.workspace = true
thiserror.workspace = true
indexmap.workspace = true

mas-iana.workspace = true
mas-jose.workspace = true

[dev-dependencies]
assert_matches = "1.5.0"
insta.workspace = true
49 changes: 32 additions & 17 deletions crates/oauth2-types/src/registration/client_metadata_serde.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@
// SPDX-License-Identifier: AGPL-3.0-only
// Please see LICENSE in the repository root for full details.

use std::{borrow::Cow, collections::HashMap};
use std::borrow::Cow;

use chrono::Duration;
use indexmap::IndexMap;
use language_tags::LanguageTag;
use mas_iana::{
jose::{JsonWebEncryptionAlg, JsonWebEncryptionEnc, JsonWebSignatureAlg},
Expand Down Expand Up @@ -45,18 +46,18 @@ impl<T> Localized<T> {
}

fn deserialize(
map: &mut HashMap<String, HashMap<Option<LanguageTag>, Value>>,
map: &mut IndexMap<String, IndexMap<Option<LanguageTag>, Value>>,
field_name: &'static str,
) -> Result<Option<Self>, serde_json::Error>
where
T: DeserializeOwned,
{
let Some(map) = map.remove(field_name) else {
let Some(map) = map.shift_remove(field_name) else {
return Ok(None);
};

let mut non_localized = None;
let mut localized = HashMap::with_capacity(map.len() - 1);
let mut localized = IndexMap::with_capacity(map.len() - 1);

for (k, v) in map {
let value = serde_json::from_value(v)?;
Expand Down Expand Up @@ -350,8 +351,8 @@ impl<'de> Deserialize<'de> for ClientMetadataLocalizedFields {
where
D: serde::Deserializer<'de>,
{
let map = HashMap::<Cow<'de, str>, Value>::deserialize(deserializer)?;
let mut new_map: HashMap<String, HashMap<Option<LanguageTag>, Value>> = HashMap::new();
let map = IndexMap::<Cow<'de, str>, Value>::deserialize(deserializer)?;
let mut new_map: IndexMap<String, IndexMap<Option<LanguageTag>, Value>> = IndexMap::new();

for (k, v) in map {
let (prefix, lang) = if let Some((prefix, lang)) = k.split_once('#') {
Expand Down Expand Up @@ -392,6 +393,8 @@ impl<'de> Deserialize<'de> for ClientMetadataLocalizedFields {

#[cfg(test)]
mod tests {
use insta::assert_yaml_snapshot;

use super::*;

#[test]
Expand Down Expand Up @@ -464,16 +467,28 @@ mod tests {
.validate()
.unwrap();

assert_eq!(
serde_json::to_value(metadata).unwrap(),
serde_json::json!({
"redirect_uris": ["http://localhost/oidc"],
"client_name": "Postbox",
"client_name#fr": "Boîte à lettres",
"client_uri": "https://localhost/",
"client_uri#fr": "https://localhost/fr",
"client_uri#de": "https://localhost/de",
})
);
assert_yaml_snapshot!(metadata, @r###"
redirect_uris:
- "http://localhost/oidc"
client_name: Postbox
"client_name#fr": Boîte à lettres
client_uri: "https://localhost/"
"client_uri#fr": "https://localhost/fr"
"client_uri#de": "https://localhost/de"
"###);

// Do a roundtrip, we should get the same metadata back with the same order
let metadata: ClientMetadata =
serde_json::from_value(serde_json::to_value(metadata).unwrap()).unwrap();
let metadata = metadata.validate().unwrap();
assert_yaml_snapshot!(metadata, @r###"
redirect_uris:
- "http://localhost/oidc"
client_name: Postbox
"client_name#fr": Boîte à lettres
client_uri: "https://localhost/"
"client_uri#fr": "https://localhost/fr"
"client_uri#de": "https://localhost/de"
"###);
}
}
9 changes: 5 additions & 4 deletions crates/oauth2-types/src/registration/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@
//!
//! [Dynamic Client Registration]: https://openid.net/specs/openid-connect-registration-1_0.html

use std::{collections::HashMap, ops::Deref};
use std::ops::Deref;

use chrono::{DateTime, Duration, Utc};
use indexmap::IndexMap;
use language_tags::LanguageTag;
use mas_iana::{
jose::{JsonWebEncryptionAlg, JsonWebEncryptionEnc, JsonWebSignatureAlg},
Expand Down Expand Up @@ -58,7 +59,7 @@ pub const DEFAULT_ENCRYPTION_ENC_ALGORITHM: &JsonWebEncryptionEnc =
#[derive(Debug, Clone, PartialEq, Eq)]
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

}

impl<T> Localized<T> {
Expand Down Expand Up @@ -104,8 +105,8 @@ impl<T> Localized<T> {
}
}

impl<T> From<(T, HashMap<LanguageTag, T>)> for Localized<T> {
fn from(t: (T, HashMap<LanguageTag, T>)) -> Self {
impl<T> From<(T, IndexMap<LanguageTag, T>)> for Localized<T> {
fn from(t: (T, IndexMap<LanguageTag, T>)) -> Self {
Localized {
non_localized: t.0,
localized: t.1,
Expand Down
Loading