Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

implement read-only auth #373

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
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
5 changes: 0 additions & 5 deletions .github/workflows/clippy.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,3 @@ jobs:
with:
token: ${{ secrets.GITHUB_TOKEN }}
args: --all-features --manifest-path=crates/Cargo.toml
- name: Clippy check y-sweet-worker
uses: actions-rs/clippy-check@v1
with:
token: ${{ secrets.GITHUB_TOKEN }}
args: --all-features --manifest-path=crates/y-sweet-worker/Cargo.toml --target=wasm32-unknown-unknown
17 changes: 6 additions & 11 deletions crates/y-sweet-core/src/api_types.rs
Original file line number Diff line number Diff line change
@@ -1,19 +1,14 @@
use std::collections::HashMap;

use serde::{Deserialize, Serialize};
use serde_json::Value;

#[derive(Serialize)]
pub struct NewDocResponse {
#[serde(rename = "docId")]
pub doc_id: String,
}

#[derive(Serialize, Deserialize)]
#[derive(Copy, Clone, Serialize, Deserialize)]
pub enum Authorization {
#[serde(rename = "none")]
Nothing,
#[serde(rename = "readonly")]
#[serde(rename = "read-only")]
ReadOnly,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This matches the types in the JS client library

#[serde(rename = "full")]
Full,
Expand All @@ -26,14 +21,11 @@ impl Authorization {
}

#[derive(Deserialize)]
#[allow(unused)]
pub struct AuthDocRequest {
#[serde(default = "Authorization::full")]
pub authorization: Authorization,
#[serde(rename = "userId")]
pub user_id: Option<String>,
#[serde(default)]
pub metadata: HashMap<String, Value>,
#[serde(skip_serializing_if = "Option::is_none", rename = "validForSeconds")]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use this at present. And if we did, we'd likely need to encode it into the token which we'd like to avoid since its length is unbounded. Let's just drop this field altogether.

pub valid_for_seconds: Option<u64>,
}
Expand All @@ -43,7 +35,6 @@ impl Default for AuthDocRequest {
Self {
authorization: Authorization::Full,
user_id: None,
metadata: HashMap::new(),
valid_for_seconds: None,
}
}
Expand All @@ -66,6 +57,10 @@ pub struct ClientToken {
/// An optional token that can be used to authenticate the client to the server.
#[serde(skip_serializing_if = "Option::is_none")]
pub token: Option<String>,

/// The authorization level of the client.
#[serde(rename = "authorization")]
pub authorization: Authorization,
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're now returning authorization as part of the ClientToken so that clients can limit front-end functionality (or just render differently) when the token is read-only.


#[derive(Deserialize, Debug)]
Expand Down
129 changes: 99 additions & 30 deletions crates/y-sweet-core/src/auth.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::api_types::Authorization;
use bincode::Options;
use data_encoding::Encoding;
use rand::Rng;
Expand Down Expand Up @@ -90,10 +91,16 @@ pub struct Authenticator {
key_id: Option<String>,
}

#[derive(Serialize, Deserialize)]
pub struct DocPermission {
pub doc_id: String,
pub authorization: Authorization,
}

#[derive(Serialize, Deserialize)]
pub enum Permission {
Server,
Doc(String),
Doc(DocPermission),
}

#[derive(Serialize, Deserialize)]
Expand Down Expand Up @@ -316,10 +323,16 @@ impl Authenticator {
pub fn gen_doc_token(
&self,
doc_id: &str,
authorization: Authorization,
expiration_time: ExpirationTimeEpochMillis,
) -> String {
let payload =
Payload::new_with_expiration(Permission::Doc(doc_id.to_string()), expiration_time);
let payload = Payload::new_with_expiration(
Permission::Doc(DocPermission {
doc_id: doc_id.to_string(),
authorization,
}),
expiration_time,
);
self.sign(payload)
}

Expand All @@ -337,18 +350,18 @@ impl Authenticator {
token: &str,
doc: &str,
current_time_epoch_millis: u64,
) -> Result<(), AuthError> {
) -> Result<Authorization, AuthError> {
let payload = self.verify_token(token, current_time_epoch_millis)?;

match payload {
Permission::Doc(doc_id) => {
if doc_id == doc {
Ok(())
Permission::Doc(doc_permission) => {
if doc_permission.doc_id == doc {
Ok(doc_permission.authorization)
} else {
Err(AuthError::InvalidResource)
}
}
Permission::Server => Ok(()), // Server tokens can access any doc.
Permission::Server => Ok(Authorization::Full), // Server tokens can access any doc.
}
}

Expand Down Expand Up @@ -386,30 +399,68 @@ mod tests {
#[test]
fn test_simple_auth() {
let authenticator = Authenticator::gen_key().unwrap();
let token = authenticator.gen_doc_token("doc123", ExpirationTimeEpochMillis(0));
assert_eq!(authenticator.verify_doc_token(&token, "doc123", 0), Ok(()));
assert_eq!(
let token = authenticator.gen_doc_token(
"doc123",
Authorization::Full,
ExpirationTimeEpochMillis(0),
);
assert!(matches!(
authenticator.verify_doc_token(&token, "doc123", 0),
Ok(Authorization::Full)
));
assert!(matches!(
authenticator.verify_doc_token(&token, "doc123", DEFAULT_EXPIRATION_SECONDS + 1),
Err(AuthError::Expired)
);
assert_eq!(
));
assert!(matches!(
authenticator.verify_doc_token(&token, "doc456", 0),
Err(AuthError::InvalidResource)
));
}

#[test]
fn test_read_only_auth() {
let authenticator = Authenticator::gen_key().unwrap();
let token = authenticator.gen_doc_token(
"doc123",
Authorization::ReadOnly,
ExpirationTimeEpochMillis(0),
);
assert!(matches!(
authenticator.verify_doc_token(&token, "doc123", 0),
Ok(Authorization::ReadOnly)
));
}

#[test]
fn test_server_token_for_doc_auth() {
let authenticator = Authenticator::gen_key().unwrap();
let server_token = authenticator.server_token();
assert!(matches!(
authenticator.verify_doc_token(&server_token, "doc123", 0),
Ok(Authorization::Full)
));
}

#[test]
fn test_key_id() {
let authenticator = Authenticator::gen_key()
.unwrap()
.with_key_id("myKeyId".try_into().unwrap());
let token = authenticator.gen_doc_token("doc123", ExpirationTimeEpochMillis(0));
let token = authenticator.gen_doc_token(
"doc123",
Authorization::Full,
ExpirationTimeEpochMillis(0),
);
assert!(
token.starts_with("myKeyId."),
"Token {} does not start with myKeyId.",
token
);
assert_eq!(authenticator.verify_doc_token(&token, "doc123", 0), Ok(()));
assert!(matches!(
authenticator.verify_doc_token(&token, "doc123", 0),
Ok(Authorization::Full)
));

let token = authenticator.server_token();
assert!(
Expand Down Expand Up @@ -438,65 +489,83 @@ mod tests {
let authenticator = Authenticator::gen_key()
.unwrap()
.with_key_id("myKeyId".try_into().unwrap());
let token = authenticator.gen_doc_token("doc123", ExpirationTimeEpochMillis(0));
let token = authenticator.gen_doc_token(
"doc123",
Authorization::Full,
ExpirationTimeEpochMillis(0),
);
let token = token.replace("myKeyId.", "aDifferentKeyId.");
assert!(token.starts_with("aDifferentKeyId."));
assert_eq!(
assert!(matches!(
authenticator.verify_doc_token(&token, "doc123", 0),
Err(AuthError::KeyMismatch)
);
));
}

#[test]
fn test_missing_key_id() {
let authenticator = Authenticator::gen_key()
.unwrap()
.with_key_id("myKeyId".try_into().unwrap());
let token = authenticator.gen_doc_token("doc123", ExpirationTimeEpochMillis(0));
let token = authenticator.gen_doc_token(
"doc123",
Authorization::Full,
ExpirationTimeEpochMillis(0),
);
let token = token.replace("myKeyId.", "");
assert_eq!(
assert!(matches!(
authenticator.verify_doc_token(&token, "doc123", 0),
Err(AuthError::KeyMismatch)
);
));
}

#[test]
fn test_unexpected_key_id() {
let authenticator = Authenticator::gen_key().unwrap();
let token = authenticator.gen_doc_token("doc123", ExpirationTimeEpochMillis(0));
let token = authenticator.gen_doc_token(
"doc123",
Authorization::Full,
ExpirationTimeEpochMillis(0),
);
let token = format!("unexpectedKeyId.{}", token);
assert_eq!(
assert!(matches!(
authenticator.verify_doc_token(&token, "doc123", 0),
Err(AuthError::KeyMismatch)
);
));
}

#[test]
fn test_invalid_signature() {
let authenticator = Authenticator::gen_key().unwrap();
let actual_payload = Payload::new(Permission::Doc("doc123".to_string()));
let actual_payload = Payload::new(Permission::Doc(DocPermission {
doc_id: "doc123".to_string(),
authorization: Authorization::Full,
}));
let mut encoded_payload =
bincode_encode(&actual_payload).expect("Bincode serialization should not fail.");
encoded_payload.extend_from_slice(&authenticator.private_key);

let token = hash(&encoded_payload);

let auth_req = AuthenticatedRequest {
payload: Payload::new(Permission::Doc("abc123".to_string())),
payload: Payload::new(Permission::Doc(DocPermission {
doc_id: "abc123".to_string(),
authorization: Authorization::Full,
})),
token,
};

let auth_enc = bincode_encode(&auth_req).expect("Bincode serialization should not fail.");
let signed = b64_encode(&auth_enc);

assert_eq!(
assert!(matches!(
authenticator.verify_doc_token(&signed, "doc123", 0),
Err(AuthError::InvalidSignature)
);
assert_eq!(
));
assert!(matches!(
authenticator.verify_doc_token(&signed, "abc123", 0),
Err(AuthError::InvalidSignature)
);
));
}

#[test]
Expand Down
38 changes: 31 additions & 7 deletions crates/y-sweet-core/src/doc_connection.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::api_types::Authorization;
use crate::sync::{
self, awareness::Awareness, DefaultProtocol, Message, Protocol, SyncMessage, MSG_SYNC,
MSG_SYNC_UPDATE,
Expand Down Expand Up @@ -30,6 +31,7 @@ pub struct DocConnection {
doc_subscription: Subscription,
#[allow(unused)] // acts as RAII guard
awareness_subscription: Subscription,
authorization: Authorization,
callback: Callback,
closed: Arc<OnceLock<()>>,

Expand All @@ -48,14 +50,22 @@ impl DocConnection {
}

#[cfg(feature = "sync")]
pub fn new<F>(awareness: Arc<RwLock<Awareness>>, callback: F) -> Self
pub fn new<F>(
awareness: Arc<RwLock<Awareness>>,
authorization: Authorization,
callback: F,
) -> Self
where
F: Fn(&[u8]) + 'static + Send + Sync,
{
Self::new_inner(awareness, Arc::new(callback))
Self::new_inner(awareness, authorization, Arc::new(callback))
}

pub fn new_inner(awareness: Arc<RwLock<Awareness>>, callback: Callback) -> Self {
pub fn new_inner(
awareness: Arc<RwLock<Awareness>>,
authorization: Authorization,
callback: Callback,
) -> Self {
let closed = Arc::new(OnceLock::new());

let (doc_subscription, awareness_subscription) = {
Expand Down Expand Up @@ -127,6 +137,7 @@ impl DocConnection {
awareness,
doc_subscription,
awareness_subscription,
authorization,
callback,
client_id: OnceLock::new(),
closed,
Expand All @@ -152,6 +163,7 @@ impl DocConnection {
protocol: &P,
msg: Message,
) -> Result<Option<Message>, sync::Error> {
let can_write = matches!(self.authorization, Authorization::Full);
let a = &self.awareness;
match msg {
Message::Sync(msg) => match msg {
Expand All @@ -160,12 +172,24 @@ impl DocConnection {
protocol.handle_sync_step1(&awareness, sv)
}
SyncMessage::SyncStep2(update) => {
let mut awareness = a.write().unwrap();
protocol.handle_sync_step2(&mut awareness, Update::decode_v1(&update)?)
if can_write {
let mut awareness = a.write().unwrap();
protocol.handle_sync_step2(&mut awareness, Update::decode_v1(&update)?)
} else {
Err(sync::Error::PermissionDenied {
reason: "Token does not have write access".to_string(),
})
}
}
SyncMessage::Update(update) => {
let mut awareness = a.write().unwrap();
protocol.handle_update(&mut awareness, Update::decode_v1(&update)?)
if can_write {
let mut awareness = a.write().unwrap();
protocol.handle_update(&mut awareness, Update::decode_v1(&update)?)
} else {
Err(sync::Error::PermissionDenied {
reason: "Token does not have write access".to_string(),
})
Comment on lines +189 to +191
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we plan to make the clients aware of the authorization level as well? Not sure how these errors are logged/captured/etc but as of now this is an expected code path, since clients will send updates as though they have full write access.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this just drops out before writing, and the resulting error gets logged in the y-sweet server. It shouldn't interrupt the websocket connection. I think we'll want to include the level of Authorization (full or read-only) in the ClientToken so that clients can use it when rendering the UI. I'm not sure how easy it is for the YSweetProvider to interrupt local changes to the YDoc, but we could at least have the provider subscribe to changes on the YDoc and log warnings when in read-only mode.

}
}
},
Message::Auth(reason) => {
Expand Down
Loading
Loading