Fix the authorization grant template

It previously relied on the client being in the authorization grant,
which is not the case anymore. This commit also adds a test to ensure
we're not breaking this template in the future.

crates/data-model/src/oauth2/authorization_grant.rs

@@ -19,6 +19,11 @@ use mas_iana::oauth::PkceCodeChallengeMethod;
 use oauth2_types::{
     pkce::{CodeChallengeError, CodeChallengeMethodExt},
     requests::ResponseMode,
+    scope::{Scope, OPENID, PROFILE},
+};
+use rand::{
+    distributions::{Alphanumeric, DistString},
+    RngCore,
 };
 use serde::Serialize;
 use ulid::Ulid;
@@ -146,7 +151,7 @@ pub struct AuthorizationGrant {
     pub code: Option<AuthorizationCode>,
     pub client_id: Ulid,
     pub redirect_uri: Url,
-    pub scope: oauth2_types::scope::Scope,
+    pub scope: Scope,
     pub state: Option<String>,
     pub nonce: Option<String>,
     pub max_age: Option<NonZeroU32>,
@@ -190,4 +195,25 @@ impl AuthorizationGrant {
         self.stage = self.stage.cancel(canceld_at)?;
         Ok(self)
     }
+
+    pub fn sample(now: DateTime<Utc>, rng: &mut impl RngCore) -> Self {
+        Self {
+            id: Ulid::from_datetime_with_source(now.into(), rng),
+            stage: AuthorizationGrantStage::Pending,
+            code: Some(AuthorizationCode {
+                code: Alphanumeric.sample_string(rng, 10),
+                pkce: None,
+            }),
+            client_id: Ulid::from_datetime_with_source(now.into(), rng),
+            redirect_uri: Url::parse("http://localhost:8080").unwrap(),
+            scope: Scope::from_iter([OPENID, PROFILE]),
+            state: Some(Alphanumeric.sample_string(rng, 10)),
+            nonce: Some(Alphanumeric.sample_string(rng, 10)),
+            max_age: None,
+            response_mode: ResponseMode::Query,
+            response_type_id_token: false,
+            created_at: now,
+            requires_consent: false,
+        }
+    }
 }

crates/data-model/src/oauth2/client.rs

@@ -12,12 +12,14 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use chrono::{DateTime, Utc};
 use mas_iana::{
     jose::JsonWebSignatureAlg,
     oauth::{OAuthAuthorizationEndpointResponseType, OAuthClientAuthenticationMethod},
 };
 use mas_jose::jwk::PublicJsonWebKeySet;
 use oauth2_types::requests::GrantType;
+use rand::RngCore;
 use serde::Serialize;
 use thiserror::Error;
 use ulid::Ulid;
@@ -120,4 +122,56 @@ impl Client {
             _ => Err(InvalidRedirectUriError::NotAllowed),
         }
     }
+
+    pub fn samples(now: DateTime<Utc>, rng: &mut impl RngCore) -> Vec<Client> {
+        vec![
+            // A client with all the URIs set
+            Self {
+                id: Ulid::from_datetime_with_source(now.into(), rng),
+                client_id: "client1".to_owned(),
+                encrypted_client_secret: None,
+                redirect_uris: vec![
+                    Url::parse("https://client1.example.com/redirect").unwrap(),
+                    Url::parse("https://client1.example.com/redirect2").unwrap(),
+                ],
+                response_types: vec![OAuthAuthorizationEndpointResponseType::Code],
+                grant_types: vec![GrantType::AuthorizationCode, GrantType::RefreshToken],
+                contacts: vec!["foo@client1.example.com".to_owned()],
+                client_name: Some("Client 1".to_owned()),
+                client_uri: Some(Url::parse("https://client1.example.com").unwrap()),
+                logo_uri: Some(Url::parse("https://client1.example.com/logo.png").unwrap()),
+                tos_uri: Some(Url::parse("https://client1.example.com/tos").unwrap()),
+                policy_uri: Some(Url::parse("https://client1.example.com/policy").unwrap()),
+                initiate_login_uri: Some(
+                    Url::parse("https://client1.example.com/initiate-login").unwrap(),
+                ),
+                token_endpoint_auth_method: Some(OAuthClientAuthenticationMethod::None),
+                token_endpoint_auth_signing_alg: None,
+                id_token_signed_response_alg: None,
+                userinfo_signed_response_alg: None,
+                jwks: None,
+            },
+            // Another client without any URIs set
+            Self {
+                id: Ulid::from_datetime_with_source(now.into(), rng),
+                client_id: "client2".to_owned(),
+                encrypted_client_secret: None,
+                redirect_uris: vec![Url::parse("https://client2.example.com/redirect").unwrap()],
+                response_types: vec![OAuthAuthorizationEndpointResponseType::Code],
+                grant_types: vec![GrantType::AuthorizationCode, GrantType::RefreshToken],
+                contacts: vec!["foo@client2.example.com".to_owned()],
+                client_name: None,
+                client_uri: None,
+                logo_uri: None,
+                tos_uri: None,
+                policy_uri: None,
+                initiate_login_uri: None,
+                token_endpoint_auth_method: None,
+                token_endpoint_auth_signing_alg: None,
+                id_token_signed_response_alg: None,
+                userinfo_signed_response_alg: None,
+                jwks: None,
+            },
+        ]
+    }
 }

crates/handlers/src/oauth2/authorization/complete.rs

@@ -21,7 +21,7 @@ use axum::{
 use axum_extra::extract::PrivateCookieJar;
 use hyper::StatusCode;
 use mas_axum_utils::SessionInfoExt;
-use mas_data_model::{AuthorizationGrant, BrowserSession};
+use mas_data_model::{AuthorizationGrant, BrowserSession, Client};
 use mas_keystore::Encrypter;
 use mas_policy::PolicyFactory;
 use mas_router::{PostAuthAction, Route};
@@ -47,6 +47,9 @@ pub enum RouteError {
 
     #[error("authorization grant is not in a pending state")]
     NotPending,
+
+    #[error("failed to load client")]
+    NoSuchClient,
 }
 
 impl IntoResponse for RouteError {
@@ -62,8 +65,8 @@ impl IntoResponse for RouteError {
                 "authorization grant not in a pending state",
             )
                 .into_response(),
-            RouteError::Internal(e) => {
+            RouteError::Internal(_) | Self::NoSuchClient => {
-                (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()).into_response()
+                (StatusCode::INTERNAL_SERVER_ERROR, self.to_string()).into_response()
             }
         }
     }
@@ -112,7 +115,13 @@ pub(crate) async fn get(
         return Ok((cookie_jar, mas_router::Login::and_then(continue_grant).go()).into_response());
     };
 
-    match complete(rng, clock, grant, session, &policy_factory, repo).await {
+    let client = repo
+        .oauth2_client()
+        .lookup(grant.client_id)
+        .await?
+        .ok_or(RouteError::NoSuchClient)?;
+
+    match complete(rng, clock, grant, client, session, &policy_factory, repo).await {
         Ok(params) => {
             let res = callback_destination.go(&templates, params).await?;
             Ok((cookie_jar, res).into_response())
@@ -128,7 +137,6 @@ pub(crate) async fn get(
         }
         Err(GrantCompletionError::NotPending) => Err(RouteError::NotPending),
         Err(GrantCompletionError::Internal(e)) => Err(RouteError::Internal(e)),
-        Err(e) => Err(RouteError::Internal(e.into())),
     }
 }
 
@@ -148,9 +156,6 @@ pub enum GrantCompletionError {
 
     #[error("denied by the policy")]
     PolicyViolation,
-
-    #[error("failed to load client")]
-    NoSuchClient,
 }
 
 impl_from_error_for_route!(GrantCompletionError: mas_storage::RepositoryError);
@@ -163,6 +168,7 @@ pub(crate) async fn complete(
     mut rng: BoxRng,
     clock: BoxClock,
     grant: AuthorizationGrant,
+    client: Client,
     browser_session: BrowserSession,
     policy_factory: &PolicyFactory,
     mut repo: BoxRepository,
@@ -181,19 +187,13 @@ pub(crate) async fn complete(
     // Run through the policy
     let mut policy = policy_factory.instantiate().await?;
     let res = policy
-        .evaluate_authorization_grant(&grant, &browser_session.user)
+        .evaluate_authorization_grant(&grant, &client, &browser_session.user)
         .await?;
 
     if !res.valid() {
         return Err(GrantCompletionError::PolicyViolation);
     }
 
-    let client = repo
-        .oauth2_client()
-        .lookup(grant.client_id)
-        .await?
-        .ok_or(GrantCompletionError::NoSuchClient)?;
-
     let current_consent = repo
         .oauth2_client()
         .get_consent_for_user(&client, &browser_session.user)

crates/handlers/src/oauth2/authorization/mod.rs

@@ -341,6 +341,7 @@ pub(crate) async fn get(
                         rng,
                         clock,
                         grant,
+                        client,
                         user_session,
                         &policy_factory,
                         repo,
@@ -372,10 +373,7 @@ pub(crate) async fn get(
                         Err(GrantCompletionError::Internal(e)) => {
                             return Err(RouteError::Internal(e))
                         }
-                        Err(
+                        Err(e @ GrantCompletionError::NotPending) => {
-                            e @ (GrantCompletionError::NotPending
-                            | GrantCompletionError::NoSuchClient),
-                        ) => {
                             // This should never happen
                             return Err(RouteError::Internal(Box::new(e)));
                         }
@@ -388,6 +386,7 @@ pub(crate) async fn get(
                         rng,
                         clock,
                         grant,
+                        client,
                         user_session,
                         &policy_factory,
                         repo,
@@ -413,10 +412,7 @@ pub(crate) async fn get(
                         Err(GrantCompletionError::Internal(e)) => {
                             return Err(RouteError::Internal(e))
                         }
-                        Err(
+                        Err(e @ GrantCompletionError::NotPending) => {
-                            e @ (GrantCompletionError::NotPending
-                            | GrantCompletionError::NoSuchClient),
-                        ) => {
                             // This should never happen
                             return Err(RouteError::Internal(Box::new(e)));
                         }

crates/handlers/src/oauth2/consent.rs

@@ -97,6 +97,12 @@ pub(crate) async fn get(
         .await?
         .ok_or(RouteError::GrantNotFound)?;
 
+    let client = repo
+        .oauth2_client()
+        .lookup(grant.client_id)
+        .await?
+        .ok_or(RouteError::NoSuchClient)?;
+
     if !matches!(grant.stage, AuthorizationGrantStage::Pending) {
         return Err(RouteError::GrantNotPending);
     }
@@ -106,11 +112,11 @@ pub(crate) async fn get(
 
         let mut policy = policy_factory.instantiate().await?;
         let res = policy
-            .evaluate_authorization_grant(&grant, &session.user)
+            .evaluate_authorization_grant(&grant, &client, &session.user)
             .await?;
 
         if res.valid() {
-            let ctx = ConsentContext::new(grant)
+            let ctx = ConsentContext::new(grant, client)
                 .with_session(session)
                 .with_csrf(csrf_token.form_value());
 
@@ -118,7 +124,7 @@ pub(crate) async fn get(
 
             Ok((cookie_jar, Html(content)).into_response())
         } else {
-            let ctx = PolicyViolationContext::new(grant)
+            let ctx = PolicyViolationContext::new(grant, client)
                 .with_session(session)
                 .with_csrf(csrf_token.form_value());
 
@@ -167,21 +173,21 @@ pub(crate) async fn post(
         return Ok((cookie_jar, login.go()).into_response());
     };
 
-    let mut policy = policy_factory.instantiate().await?;
-    let res = policy
-        .evaluate_authorization_grant(&grant, &session.user)
-        .await?;
-
-    if !res.valid() {
-        return Err(RouteError::PolicyViolation);
-    }
-
     let client = repo
         .oauth2_client()
         .lookup(grant.client_id)
         .await?
         .ok_or(RouteError::NoSuchClient)?;
 
+    let mut policy = policy_factory.instantiate().await?;
+    let res = policy
+        .evaluate_authorization_grant(&grant, &client, &session.user)
+        .await?;
+
+    if !res.valid() {
+        return Err(RouteError::PolicyViolation);
+    }
+
     // Do not consent for the "urn:matrix:org.matrix.msc2967.client:device:*" scope
     let scope_without_device = grant
         .scope

crates/policy/src/lib.rs

@@ -17,7 +17,7 @@
 #![warn(clippy::pedantic)]
 #![allow(clippy::missing_errors_doc)]
 
-use mas_data_model::{AuthorizationGrant, User};
+use mas_data_model::{AuthorizationGrant, Client, User};
 use oauth2_types::registration::VerifiedClientMetadata;
 use opa_wasm::Runtime;
 use serde::Deserialize;
@@ -246,6 +246,7 @@ impl Policy {
         skip_all,
         fields(
             data.authorization_grant.id = %authorization_grant.id,
+            data.client.id = %client.id,
             data.user.id = %user.id,
         ),
         err,
@@ -253,12 +254,14 @@ impl Policy {
     pub async fn evaluate_authorization_grant(
         &mut self,
         authorization_grant: &AuthorizationGrant,
+        client: &Client,
         user: &User,
     ) -> Result<EvaluationResult, EvaluationError> {
         let authorization_grant = serde_json::to_value(authorization_grant)?;
         let user = serde_json::to_value(user)?;
         let input = serde_json::json!({
             "authorization_grant": authorization_grant,
+            "client": client,
             "user": user,
         });
 

crates/templates/src/context.rs

@@ -18,8 +18,8 @@
 
 use chrono::Utc;
 use mas_data_model::{
-    AuthorizationGrant, BrowserSession, CompatSsoLogin, CompatSsoLoginState, UpstreamOAuthLink,
+    AuthorizationGrant, BrowserSession, Client, CompatSsoLogin, CompatSsoLoginState,
-    UpstreamOAuthProvider, User, UserEmail, UserEmailVerification,
+    UpstreamOAuthLink, UpstreamOAuthProvider, User, UserEmail, UserEmailVerification,
 };
 use mas_router::{PostAuthAction, Route};
 use rand::Rng;
@@ -395,25 +395,42 @@ impl RegisterContext {
 #[derive(Serialize)]
 pub struct ConsentContext {
     grant: AuthorizationGrant,
+    client: Client,
     action: PostAuthAction,
 }
 
 impl TemplateContext for ConsentContext {
-    fn sample(_now: chrono::DateTime<Utc>, _rng: &mut impl Rng) -> Vec<Self>
+    fn sample(now: chrono::DateTime<Utc>, rng: &mut impl Rng) -> Vec<Self>
     where
         Self: Sized,
     {
-        // TODO
+        Client::samples(now, rng)
-        vec![]
+            .into_iter()
+            .map(|client| {
+                let mut grant = AuthorizationGrant::sample(now, rng);
+                let action = PostAuthAction::continue_grant(grant.id);
+                // XXX
+                grant.client_id = client.id;
+                Self {
+                    grant,
+                    client,
+                    action,
+                }
+            })
+            .collect()
     }
 }
 
 impl ConsentContext {
     /// Constructs a context for the client consent page
     #[must_use]
-    pub fn new(grant: AuthorizationGrant) -> Self {
+    pub fn new(grant: AuthorizationGrant, client: Client) -> Self {
         let action = PostAuthAction::continue_grant(grant.id);
-        Self { grant, action }
+        Self {
+            grant,
+            client,
+            action,
+        }
     }
 }
 
@@ -421,25 +438,42 @@ impl ConsentContext {
 #[derive(Serialize)]
 pub struct PolicyViolationContext {
     grant: AuthorizationGrant,
+    client: Client,
     action: PostAuthAction,
 }
 
 impl TemplateContext for PolicyViolationContext {
-    fn sample(_now: chrono::DateTime<Utc>, _rng: &mut impl Rng) -> Vec<Self>
+    fn sample(now: chrono::DateTime<Utc>, rng: &mut impl Rng) -> Vec<Self>
     where
         Self: Sized,
     {
-        // TODO
+        Client::samples(now, rng)
-        vec![]
+            .into_iter()
+            .map(|client| {
+                let mut grant = AuthorizationGrant::sample(now, rng);
+                let action = PostAuthAction::continue_grant(grant.id);
+                // XXX
+                grant.client_id = client.id;
+                Self {
+                    grant,
+                    client,
+                    action,
+                }
+            })
+            .collect()
     }
 }
 
 impl PolicyViolationContext {
     /// Constructs a context for the policy violation page
     #[must_use]
-    pub const fn new(grant: AuthorizationGrant) -> Self {
+    pub const fn new(grant: AuthorizationGrant, client: Client) -> Self {
         let action = PostAuthAction::continue_grant(grant.id);
-        Self { grant, action }
+        Self {
+            grant,
+            client,
+            action,
+        }
     }
 }
 

templates/pages/consent.html

@@ -23,17 +23,17 @@ limitations under the License.
         <div class="rounded-lg bg-grey-25 dark:bg-grey-450 p-2 flex flex-col">
           <div class="text-center">
             <div class="bg-white rounded w-16 h-16 overflow-hidden mx-auto">
-              {% if grant.client.logo_uri %}
+              {% if client.logo_uri %}
-              <img class="w-16 h-16" src="{{ grant.client.logo_uri }}" />
+              <img class="w-16 h-16" src="{{ client.logo_uri }}" />
               {% endif %}
             </div>
-            <h1 class="text-lg text-center font-medium"><a target="_blank" href="{{ grant.client.client_uri }}" class="text-accent">{{ grant.client.client_name | default(value=grant.client.client_id) }}</a></h1>
+            <h1 class="text-lg text-center font-medium"><a target="_blank" href="{{ client.client_uri }}" class="text-accent">{{ client.client_name | default(value=client.client_id) }}</a></h1>
             <h1>at {{ grant.redirect_uri }}</h1>
             <h1>wants to access your Matrix account</h1>
           </div>
           <div class="flex items-center m-2">
             <div class="px-4 flex-1">
-              <p>This will allow <a target="_blank" href="{{ grant.client.client_uri }}" class="text-accent">{{ grant.client.client_name | default(value=grant.client.client_id) }}</a> to:</p>
+              <p>This will allow <a target="_blank" href="{{ client.client_uri }}" class="text-accent">{{ client.client_name | default(value=client.client_id) }}</a> to:</p>
 
               <p class="my-2">
                 <ul class="list-disc">
@@ -49,19 +49,19 @@ limitations under the License.
                   {% endfor %}
                 </ul>  
               </p>
-              <p class="font-bold my-2">Make sure that you trust {{ grant.client.client_name }}</p>
+              <p class="font-bold my-2">Make sure that you trust {{ client.client_name }}</p>
               <p>
                 You may be sharing sensitive information with this site or app.
-                {% if grant.client.policy_uri or grant.client.tos_uri %}
+                {% if client.policy_uri or client.tos_uri %}
-                  Find out how {{ grant.client.client_name }} will handle your data by reviewing it's
+                  Find out how {{ client.client_name }} will handle your data by reviewing it's
-                  {% if grant.client.policy_uri %}
+                  {% if client.policy_uri %}
-                    <a target="_blank" href="{{ grant.client.policy_uri }}" class="text-accent">privacy policy</a>{% if not grant.client.tos_uri %}.{% endif %}
+                    <a target="_blank" href="{{ client.policy_uri }}" class="text-accent">privacy policy</a>{% if not client.tos_uri %}.{% endif %}
                   {% endif %}
-                  {% if grant.client.policy_uri and grant.client.tos_uri%}
+                  {% if client.policy_uri and client.tos_uri%}
                     and
                   {% endif %}
-                  {% if grant.client.tos_uri %}
+                  {% if client.tos_uri %}
-                    <a target="_blank" href="{{ grant.client.tos_uri }}" class="text-accent">terms of service</a>.
+                    <a target="_blank" href="{{ client.tos_uri }}" class="text-accent">terms of service</a>.
                   {% endif %}
                 {% endif %}
               </p>

templates/pages/policy_violation.html

@@ -24,11 +24,11 @@ limitations under the License.
         <p>This might be because of the client which authored the request, the currently logged in user, or the request itself.</p>
         <div class="rounded-lg bg-grey-25 dark:bg-grey-450 p-2 flex items-center">
           <div class="bg-white rounded w-16 h-16 overflow-hidden mx-auto">
-            {% if grant.client.logo_uri %}
+            {% if client.logo_uri %}
-            <img class="w-16 h-16" src="{{ grant.client.logo_uri }}" />
+            <img class="w-16 h-16" src="{{ client.logo_uri }}" />
             {% endif %}
           </div>
-          <h1 class="text-lg text-center font-medium flex-1"><a target="_blank" href="{{ grant.client.client_uri }}" class="text-accent">{{ grant.client.client_name | default(value=grant.client.client_id) }}</a></h1>
+          <h1 class="text-lg text-center font-medium flex-1"><a target="_blank" href="{{ client.client_uri }}" class="text-accent">{{ client.client_name | default(value=client.client_id) }}</a></h1>
         </div>
 
         <div class="rounded-lg bg-grey-25 dark:bg-grey-450 p-2 flex items-center">