-
Notifications
You must be signed in to change notification settings - Fork 593
Store client credentials in a new system table #2983
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious what the bug you alluded to during standup is.
@@ -63,22 +63,22 @@ fn creds_store() -> credentials::File { | |||
/// Our `on_connect` callback: save our credentials to a file. | |||
fn on_connected(_ctx: &DbConnection, _identity: Identity, token: &str) { | |||
if let Err(e) = creds_store().save(token) { | |||
eprintln!("Failed to save credentials: {:?}", e); | |||
eprintln!("Failed to save credentials: {e:?}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear to me why these changed, but I also don't care.
crates/client-api/src/auth.rs
Outdated
pub subject: String, | ||
pub issuer: String, | ||
pub claims: SpacetimeIdentityClaims, | ||
// The decoded JWT payload. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// The decoded JWT payload. | |
/// The decoded JWT payload. |
Use ///
for doc comments.
crates/client-api/src/auth.rs
Outdated
pub issuer: String, | ||
pub claims: SpacetimeIdentityClaims, | ||
// The decoded JWT payload. | ||
pub raw_payload: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dissonance between comment saying "decoded" and field name saying "raw" is weird. I assume this is decoded from base64 into a string which will contain a JSON object, but is raw in the sense that the JSON hasn't been parsed.
#[derive(Clone, Debug, Eq, PartialEq, SpacetimeType)] | ||
#[sats(crate = spacetimedb_lib)] | ||
pub struct StConnectionCredentialsRow { | ||
pub connection_id: ConnectionIdViaU128, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't otherwise enforce uniqueness of connection IDs, only of (ConnectionId, Identity)
pairs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks reasonable at this stage.
|
||
// TODO: Is this being broadcast? Does it need to be, or are st_client table subscriptions | ||
// not allowed? | ||
// I don't think it was being broadcast previously. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// I don't think it was being broadcast previously. | |
// I (jsdt 2025-08-06) don't think it was being broadcast previously. |
This sounds like the kind of bug you were talking about the other day, yeah. st_client
subscriptions are not disallowed, but also will not work in any of the SDKs due to missing bindings, and aren't an advertised feature. Maybe we should just make some or all of the system tables private and not have to think about it any more. Or maybe we should fix the bug and broadcast this TX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #3130 .
Description of Changes
This adds a new system table to store the jwt payloads of connected clients. I'm planning to use this system table to expose client claims to modules in subsequent PRs.
The new table is called
st_connection_credentials
. It is a private system table which stores a mapping fromconnection_id
tojwt_payload
. Note that a jwt payload is a json representation of the clients claims, not a fully signed token.The times when we need to insert and delete these rows closely mirrors that of the existing
st_client
table, with 1.5 exceptions:st_client
until after theOnConnect
reducer ran (even though it was in the same transaction). We wantst_connection_credentials
to be populated before calling the reducer, so that the reducer can use it get the credentials, so I made a change to insert tost_client
andst_connection_credentials
before calling the reducer.Expected complexity level and risk
2.5
Adding a system table is a bit risky. My understanding is that this change will not be rollback safe. I will look into making the addition of new system tables rollback safe in the future, but that would delay this by a release anyway.
Testing
Manual for me to do:
I also need to figure out how to write some tests to ensure this table is getting updated correctly.