Fix inconsistent auth/identity creation #735

coolreader18 · 2024-01-19T20:15:29Z

Description of Changes

Now, in most cases, it'll create an identity if the user doesn't pass one.

Testing

spacetime init my_new_project --lang rust
cd my_new_project
spacetime publish my_module

# Make sure we can call a reducer with no identity
curl -X POST http://localhost:3000/database/my_module/say_hello

bfops

I think this mostly makes sense to me! I left some questions and confusions.

Also I have no idea if there's any automated test coverage we could/should be updating or anything 🤷

(I also left some notes for other reviewers based on my read-through; I'm not looking for a response from you on those! 🙂)

bfops · 2024-01-31T17:06:04Z

crates/client-api/src/auth.rs

+#[derive(Clone, Deserialize)]
+pub struct SpacetimeCreds {
+    token: String,
+}


sorry, sanity-check - is this type serialized+deserialized anywhere besides via the Credentials trait? are we breaking compatibility with anything on some disk / over the wire / etc?

Yeah, nowhere else - it's just for Credentials.

bfops · 2024-01-31T17:18:06Z

crates/standalone/src/routes/mod.rs

+            database::control_routes(ctx.clone()).merge(database::worker_routes(ctx.clone())),
+        )
+        .nest("/identity", identity::router(ctx.clone()))
+        .nest("/energy", energy::router(ctx.clone()))


probably a dumb question... why do only some of these routes get the ctx? Since the function is generic, I'd kind of expect it to just pass along ctx to everything?

Limitations of axum, sorta - the middleware needs access to ctx, but axum doesn't let middleware access the state from the router, so you have to double up on passing the state.

crates/client-api/src/auth.rs

bfops · 2024-01-31T17:32:10Z

crates/client-api/src/routes/subscribe.rs

-        TypedHeader(SpacetimeIdentity(auth.identity)),
-        TypedHeader(SpacetimeIdentityToken(auth.creds)),
-        res,
-    ))


note for review: this got moved into the auth_middleware

bfops · 2024-01-31T17:34:39Z

crates/client-api/src/routes/identity.rs

-            Err(StatusCode::BAD_REQUEST.into())
-        }
-    } else {
-        Err(StatusCode::UNAUTHORIZED.into())


note for review: this outer if got eaten by the change to SpacetimeAuthRequired

bfops · 2024-01-31T17:36:44Z

crates/client-api/src/routes/identity.rs

+    Ok((
+        [(CONTENT_TYPE, "application/pem-certificate-chain")],
+        ctx.public_key_bytes().to_owned(),
+    ))


sorry, this one's confusing me - why did this change / is it the same behavior?

Same behavior, yeah, just more idiomatic.

bfops · 2024-01-31T17:50:33Z

crates/client-api/src/auth.rs

-    Jwt(JwtErrorKind),
-    Header(TypedHeaderRejection),
-    MalformedTokenQueryString,
-    CantDecodeAuthorizationToken,


where did CantDecodeAuthorizationToken go?

That wasn't an error that could ever actually occur; we basically encoded the token to base64 and then decoded it from base64 and if the decoding failed we returned that error. It could've just been an unwrap() instead of a variant.

bfops · 2024-01-31T17:54:05Z

crates/client-api/src/auth.rs

+        let Some(creds) = SpacetimeCreds::from_request_parts(parts)? else {
+            return Ok(Self { auth: None });
+        };
+        let claims = creds.decode_token(state.public_key())?;


again, a dumb question - we used to do e.into_kind() and wrap it in a specific return - does this work fine if we just return the "bare" Err value here?

Yeah, the error handling before was unnecessarily complicated, this is equivalent.

bfops · 2024-01-31T17:57:14Z

crates/client-api/src/auth.rs

+        if let Ok(Query(creds)) = Query::<Self>::try_from_uri(&parts.uri) {
+            return Ok(Some(creds));
+        }
+        res.map(|()| None)


I might be missing something simple.. why do we return Ok(()) into res, only to .map(|()| None) here? Couldn't we just => Ok(None) above?

Yeah, I guess, but then it would be a None that would never be Some. it doesn't much matter I suppose, it could be Ok(None), the logic is just kinda convoluted and I was trying to preserve it while making it idiomatic.

bfops · 2024-01-31T18:04:37Z

Summary of changes, I think:

Restructuring SpacetimeCreds datatype
Factoring out helper functions, especially in SpacetimeCreds + SpacetimeAuthHeader
Add SpacetimeAuthRequired
Plumb a ctx parameter through our routes
Add auth_middleware to create identities "on the fly" (and return them if appropriate)
Wrap a bunch of routes in auth_middleware

bfops

Okay, I think this looks good to me - thank you for explaining everything!

bfops

I did another review pass, and had a few more comments

bfops · 2024-02-06T20:24:00Z

crates/client-api/src/auth.rs

@@ -260,3 +243,15 @@ impl headers::Header for SpacetimeExecutionDurationMicros {
        values.extend([(self.0.as_micros() as u64).into()])
    }
 }
+
+pub async fn auth_middleware<S: ControlStateDelegate + NodeDelegate>(


what do you think about renaming this to something more descriptive? e.g. anonymous_auth_middleware?

Not fully opposed, but I feel that this is descriptive enough as is - it's a feature of our auth system that you don't actually need to be auth'd, that we'll generate an identity for you. So this is the full auth middleware; it does do standard authentication (verify you are who you say you are), not just anonymous auth.

The only places we don't generate it for you is where it doesn't make any sense to - but there's no urgent reason to disallow e.g. set_email on an anonymous identity, just that it doesn't make much sense and is likely a mistake.

crates/client-api/src/auth.rs

bfops · 2024-02-20T23:05:35Z

Our reviewers are a bit bottlenecked, and I don't think we should necessarily trust me as solo-reviewer yet 😅 - I moved just the auth.rs changes into their own refactor PR, on the hope that might make it easier to get review on everything. What do you think?

* [zeke/noa-router-shuffle]: bring over auth.rs changes from #735 * [zeke/noa-router-shuffle]: review

crates/client-api/src/routes/energy.rs

bfops · 2024-05-22T17:31:58Z

crates/client-api/src/routes/identity.rs

@@ -199,39 +197,39 @@ pub struct ValidateTokenParams {

 pub async fn validate_token(
    Path(ValidateTokenParams { identity }): Path<ValidateTokenParams>,
-    auth: SpacetimeAuthHeader,
+    SpacetimeAuthRequired(auth): SpacetimeAuthRequired,


nit: this one ends up doing a auth.identity != identity check anyway, so would anonymous auth be fine like the others?

I think given we're validating the token - it makes sense to require a token to validate against. If you don't pass a token to validate_token it was probably a mistake.

bfops · 2024-05-22T17:33:16Z

crates/core/src/auth/identity.rs

-        timer += Duration::from_secs(seconds);
-        // SAFETY: duration_since will panic if an argument is later than the time
-        // used for the duration calculation. In case of UNIX_EPOCH it can't be the case
-        timer.duration_since(UNIX_EPOCH).unwrap().as_secs()


why did this get to be removed?

Tidying up auth stuff, I guess. serde_with can do this conversion for us.

Ah I see the new serde_with line. Seems reasonable.

bfops · 2024-05-23T16:47:16Z