Repository: sebadob/rauthy (language: rust)

Issue:
Upstream OIDC with LastLogin doesn't work for some IDs
Configuring upstream OIDC was very straight-forward. However, I noticed that one of my email addresses didn't work, resulting in

`Uncovered HTTP return status '400'. This should never happen, please report this bug.`

The email address contains a "&" character, which is valid in email addresses. My best guess is the validator didn't like that.


Discussion / hints:
Tbh I am not sure which characters exactly the regex is matching against. I am using the built-in email validation from the `validator` crate.

Is this bug consistently reproducable with the `&` email address or does it happen just sometimes? Because I experienced some cache errors with old requests in the past but a some point was not able to reproduce them. They resulsted in such an error.
Just so I can do a proper test and add it to the code base, was it something like `e&mail@example.com` ?
The `&` in email validation is accepted like it should, the reason must be something else. Please check the logs from Rauthy to get more information. You may set `LOG_LEVEL=debug` to get detailed logging for the auth provider callback.

Edit:

I will update the error message in the UI to show the real error directly, because this is not necessarily a bug with Rauthy, the problem can come from the upstream provider as well in a few places. You can grab the full error response from the network tab during the callback. It is just not deserialized into a message which will show up in the UI at the moment.
Without additional information like debug logs or a way to reproduce it, I can't do anything about this or help you with it though.

An HTTP 400 can almost only come up if there is a problem with the upstream provider, which is why it was treated like a bug / uncovered error so far.
Sorry for the ambiguous thumbs up! Was on mobile. I'm planning to try again with the logging and see if I can get a better errors message for you.
`v0.23.3` should show the detailed error message now, if you get into that situation again.  
It simply shows the JSON deserialized error from the backend as is in the UI, which should help debugging.
Here's what the UI is showing now
![DeepinScreenshot_select-area_20240611061226](https://github.com/sebadob/rauthy/assets/7820200/e4f08d11-1513-45e4-bb35-7e843b797bcd)

I checked the network logs and found this under Respone:

```json
{
  "timestamp": 1718108103,
  "error": "BadRequest",
  "message": "Payload deserialization error: Error(\"invalid type: string \\\"test&@apitman.com\\\", expected a borrowed string\", line: 1, column: 65)"
}
```


It's possible that LastLogin is supposed to be doing some sort of encoding before putting that into the ID token. I'm not sure.
> Here's what the UI is showing now (...)

Oh yeah, my bad... I love Javascript.. should have converted the body to string.

> I checked the network logs and found this under Respone:

The error is expected, since `test&@apitman.com` is an invalid email address. the `&` can't be directly in front of the `@`.
If it would be in the middle of the username part, it would be fine. 
As I am thinking about it, it seems that it complains about not receiving a string in the first place there. I actually created a test payload with `&` in email adresses and everything was fine, all tests passed. So it must be something with the payload itself.
> The error is expected, since `test&@apitman.com` is an invalid email address. the `&` can't be directly in front of the `@`. If it would be in the middle of the username part, it would be fine.

Do you have a link to the grammar you're using? A quick search didn't yield anything on this rule. FWIW gmail and outlook both work fine sending to that address.

> As I am thinking about it, it seems that it complains about not receiving a string in the first place there. I actually created a test payload with & in email adresses and everything was fine, all tests passed. So it must be something with the payload itself.

Happy to help with further debugging if I can. It's certainly possible I'm returning bad data.

I don't know what the validation regex is checking for sure, to what extent, because I am using a built-in email validation. But for valid addresses, there are RFCs like [RFC 5322](https://www.rfc-editor.org/rfc/rfc5322#section-3.4) or [RFC 822](https://www.rfc-editor.org/rfc/rfc822#section-6.1).

But, I just created a new test using `test&@example.com` and it was fine. So it must be something with the encoding coming from your IdP.

Again FWIW https://openidconnect.net/ (and other OIDC clients I've tried) works fine with LastLogin. Here's an example ID token returned:

```
eyJhbGciOiJSUzI1NiIsImtpZCI6InlPeXYtWG1tUVYtUTNrUUVCbkRHVlFGb2hoVTNyaTFiRkY0VEg4VGZYTFUiLCJ0eXAiOiJKV1QifQ.eyJhdWQiOlsiaHR0cHM6Ly9vcGVuaWRjb25uZWN0Lm5ldCJdLCJlbWFpbCI6InRlc3RcdTAwMjZAYXBpdG1hbi5jb20iLCJlbWFpbF92ZXJpZmllZCI6dHJ1ZSwiZXhwIjoxNzE4MjAwNjc2LCJpYXQiOjE3MTgxMTQyNzYsImlzcyI6Imh0dHBzOi8vbGFzdGxvZ2luLmlvIiwibmFtZSI6InRlc3RcdTAwMjYiLCJub25jZSI6IiIsInN1YiI6InRlc3RcdTAwMjZAYXBpdG1hbi5jb20ifQ.IchRsMNdE4isQ9ug8GqFtc_TRkZ3wA1KQw4dqnwqqVgNMcbY3qTuufwRHW-zoLDxQZkNOCl0niJolrLGmSfeSAo0fRtRMrkq41b7hhcnfE2MLBTZbI3E8m2mbxdgGlBJxeOBFdIwhN3meuioxiqnoAdCh1aw8p2dj7TuBaHpNXdREUcB76U1ZXHqSmYFWZDVROzMgVb7PjL2ikPcjksWf-jlFIRrEdbRXm9DygL9LAUFI10IjCZePFv8wjmr0aJS1pXAXYn31hoSWaH0xxoDCmO4t4aBafL392zX6is0i3uRYSu-3L0GJRCT7qmbm_FiGvEqfFQoILslKRvzWG74Ww
```
Decoded:
```json
{
 "aud": [
  "https://openidconnect.net"
 ],
 "email": "test&@apitman.com",
 "email_verified": true,
 "exp": 1718200676,
 "iat": 1718114276,
 "iss": "https://lastlogin.io",
 "name": "test&",
 "nonce": "",
 "sub": "test&@apitman.com"
}
```


So... I found the reason. And its kinda sad. I guess you are using some official JWT library in Go, right?

Whatever you are using is encoding the `&` as unicode, which results in not having really `&` in the string, but `\u0026` under the hood.  
I can make this work, when I deserialize all the upstream IdP values into proper Strings, which means quite a bit of additional memory allocation instead of using references and borrowed data in utf8 format like I am doing it now.

This means, to support stuff like this, Rauthy needs to make a few steps back in optimization and efficiency, which is the sad part about it.

Fix patch:
diff --git a/Cargo.lock b/Cargo.lock
index 4a8128723..88b545231 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -2906,6 +2906,7 @@ checksum = "bd070e393353796e801d209ad339e89596eb4c8d430d18ede6a1cced8fafbd99"
 dependencies = [
  "autocfg",
  "hashbrown 0.12.3",
+ "serde",
 ]
 
 [[package]]
@@ -4596,6 +4597,7 @@ dependencies = [
  "serde",
  "serde_json",
  "serde_json_path",
+ "serde_with",
  "spow",
  "sqlx",
  "time",
@@ -5608,6 +5610,36 @@ dependencies = [
  "serde",
 ]
 
+[[package]]
+name = "serde_with"
+version = "3.8.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "0ad483d2ab0149d5a5ebcd9972a3852711e0153d863bf5a5d0391d28883c4a20"
+dependencies = [
+ "base64 0.22.1",
+ "chrono",
+ "hex",
+ "indexmap 1.9.3",
+ "indexmap 2.2.6",
+ "serde",
+ "serde_derive",
+ "serde_json",
+ "serde_with_macros",
+ "time",
+]
+
+[[package]]
+name = "serde_with_macros"
+version = "3.8.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "65569b702f41443e8bc8bbb1c5779bd0450bbe723b56198980e80ec45780bce2"
+dependencies = [
+ "darling",
+ "proc-macro2",
+ "quote",
+ "syn 2.0.66",
+]
+
 [[package]]
 name = "sha1"
 version = "0.10.6"
diff --git a/Cargo.toml b/Cargo.toml
index d68f4388b..66643814d 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -95,6 +95,7 @@ semver = { version = "1.0.19", features = ["serde"] }
 serde = { version = "1", features = ["derive"] }
 serde_json = "1"
 serde_json_path = "0.6.7"
+serde_with = { version = "3.8.1", features = ["macros"] }
 spow = "0.2"
 sqlx = { version = "0.7", features = ["macros", "migrate", "postgres", "runtime-tokio", "sqlite", "tls-rustls", "uuid"] }
 time = { version = "0.3", features = ["formatting", "local-offset", "macros", "parsing", "serde"] }
diff --git a/rauthy-models/Cargo.toml b/rauthy-models/Cargo.toml
index 8efaffbf1..15061a1a7 100644
--- a/rauthy-models/Cargo.toml
+++ b/rauthy-models/Cargo.toml
@@ -66,6 +66,7 @@ semver = { workspace = true }
 serde = { workspace = true }
 serde_json = { workspace = true }
 serde_json_path = { workspace = true }
+serde_with = { workspace = true }
 spow = { workspace = true }
 sqlx = { workspace = true }
 time = { workspace = true }
diff --git a/rauthy-models/src/entity/auth_providers.rs b/rauthy-models/src/entity/auth_providers.rs
index b6d440249..4229f7c06 100644
--- a/rauthy-models/src/entity/auth_providers.rs
+++ b/rauthy-models/src/entity/auth_providers.rs
@@ -1043,17 +1043,17 @@ struct AuthProviderIdClaims<'a> {
     // azp: Option<&'a str>,
     // even though `email` is mandatory for Rauthy, we set it to optional for
     // the deserialization to have more control over the error message being returned
-    email: Option<&'a str>,
+    email: Option<Cow<'a, str>>,
     email_verified: Option<bool>,
 
-    name: Option<&'a str>,
-    given_name: Option<&'a str>,
-    family_name: Option<&'a str>,
+    name: Option<Cow<'a, str>>,
+    given_name: Option<Cow<'a, str>>,
+    family_name: Option<Cow<'a, str>>,
 
     address: Option<AuthProviderAddressClaims<'a>>,
-    birthdate: Option<&'a str>,
-    locale: Option<&'a str>,
-    phone: Option<&'a str>,
+    birthdate: Option<Cow<'a, str>>,
+    locale: Option<Cow<'a, str>>,
+    phone: Option<Cow<'a, str>>,
 
     json_bytes: Option<&'a [u8]>,
 }
@@ -1070,9 +1070,9 @@ impl<'a> TryFrom<&'a [u8]> for AuthProviderIdClaims<'a> {
 
 impl AuthProviderIdClaims<'_> {
     fn given_name(&self) -> &str {
-        if let Some(given_name) = self.given_name {
+        if let Some(given_name) = &self.given_name {
             given_name
-        } else if let Some(name) = self.name {
+        } else if let Some(name) = &self.name {
             let (given_name, _) = name.split_once(' ').unwrap_or(("N/A", "N/A"));
             given_name
         } else {
@@ -1081,9 +1081,9 @@ impl AuthProviderIdClaims<'_> {
     }
 
     fn family_name(&self) -> &str {
-        if let Some(family_name) = self.family_name {
+        if let Some(family_name) = &self.family_name {
             family_name
-        } else if let Some(name) = self.name {
+        } else if let Some(name) = &self.name {
             let (_, family_name) = name.split_once(' ').unwrap_or(("N/A", "N/A"));
             family_name
         } else {
@@ -1159,7 +1159,7 @@ impl AuthProviderIdClaims<'_> {
                 // On conflict, the DB would return an error anyway, but the error message is
                 // rather cryptic for a normal user.
                 if let Ok(mut user) =
-                    User::find_by_email(data, self.email.unwrap().to_string()).await
+                    User::find_by_email(data, self.email.as_ref().unwrap().to_string()).await
                 {
                     // TODO check if creating a new link for an existing user is allowed
                     if let Some(link) = link_cookie {
@@ -1321,9 +1321,9 @@ impl AuthProviderIdClaims<'_> {
             }
 
             // check / update email
-            if Some(user.email.as_str()) != self.email {
+            if Some(user.email.as_str()) != self.email.as_deref() {
                 old_email = Some(user.email);
-                user.email = self.email.unwrap().to_string();
+                user.email = self.email.as_ref().unwrap().to_string();
             }
 
             // check other existing values and possibly update them
@@ -1370,7 +1370,7 @@ impl AuthProviderIdClaims<'_> {
         } else {
             // Create a new federated user
             let new_user = User {
-                email: self.email.unwrap().to_string(),
+                email: self.email.as_ref().unwrap().to_string(),
                 given_name: self.given_name().to_string(),
                 family_name: self.family_name().to_string(),
                 roles: should_be_rauthy_admin
@@ -1385,7 +1385,7 @@ impl AuthProviderIdClaims<'_> {
                 enabled: true,
                 email_verified: self.email_verified.unwrap_or(false),
                 last_login: Some(now),
-                language: self.locale.map(Language::from).unwrap_or_default(),
+                language: Language::from(self.locale.as_ref().unwrap().as_ref()),
                 auth_provider_id: Some(provider.id.clone()),
                 federation_uid: Some(claims_user_id.to_string()),
                 ..Default::default()
@@ -1413,11 +1413,11 @@ impl AuthProviderIdClaims<'_> {
                 country: None,
             },
         };
-        if let Some(bday) = self.birthdate {
+        if let Some(bday) = &self.birthdate {
             user_values.birthdate = Some(bday.to_string());
             found_values = true;
         }
-        if let Some(phone) = self.phone {
+        if let Some(phone) = &self.phone {
             user_values.phone = Some(phone.to_string());
             found_values = true;
         }
@@ -1464,6 +1464,7 @@ struct OidcCodeRequestParams<'a> {
 mod tests {
     use super::*;
     use cryptr::EncKeys;
+    use rauthy_common::utils::{base64_url_decode, base64_url_no_pad_encode};
 
     #[test]
     fn test_auth_provider_link_cookie() {
@@ -1533,4 +1534,12 @@ mod tests {
         assert_eq!(nodes.len(), 1);
         assert_eq!(nodes.get(0).unwrap().as_str(), Some("yes"));
     }
+
+    #[test]
+    fn test_id_token_deserialization() {
+        // this raw token contains unicode encoded chars
+        let raw = "eyJhbGciOiJSUzI1NiIsImtpZCI6InlPeXYtWG1tUVYtUTNrUUVCbkRHVlFGb2hoVTNyaTFiRkY0VEg4VGZYTFUiLCJ0eXAiOiJKV1QifQ.eyJhdWQiOlsiaHR0cHM6Ly9vcGVuaWRjb25uZWN0Lm5ldCJdLCJlbWFpbCI6InRlc3RcdTAwMjZAYXBpdG1hbi5jb20iLCJlbWFpbF92ZXJpZmllZCI6dHJ1ZSwiZXhwIjoxNzE4MjAwNjc2LCJpYXQiOjE3MTgxMTQyNzYsImlzcyI6Imh0dHBzOi8vbGFzdGxvZ2luLmlvIiwibmFtZSI6InRlc3RcdTAwMjYiLCJub25jZSI6IiIsInN1YiI6InRlc3RcdTAwMjZAYXBpdG1hbi5jb20ifQ.IchRsMNdE4isQ9ug8GqFtc_TRkZ3wA1KQw4dqnwqqVgNMcbY3qTuufwRHW-zoLDxQZkNOCl0niJolrLGmSfeSAo0fRtRMrkq41b7hhcnfE2MLBTZbI3E8m2mbxdgGlBJxeOBFdIwhN3meuioxiqnoAdCh1aw8p2dj7TuBaHpNXdREUcB76U1ZXHqSmYFWZDVROzMgVb7PjL2ikPcjksWf-jlFIRrEdbRXm9DygL9LAUFI10IjCZePFv8wjmr0aJS1pXAXYn31hoSWaH0xxoDCmO4t4aBafL392zX6is0i3uRYSu-3L0GJRCT7qmbm_FiGvEqfFQoILslKRvzWG74Ww";
+        let claims_bytes = AuthProviderIdClaims::self_as_bytes_from_token(raw).unwrap();
+        assert!(AuthProviderIdClaims::try_from(claims_bytes.as_bytes()).is_ok());
+    }
 }


Test patch: