Skip to content
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

Avoid panics #392

Merged
merged 8 commits into from
Dec 29, 2018
Merged
Show file tree
Hide file tree
Changes from 5 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
2 changes: 1 addition & 1 deletion plume-cli/src/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,5 +59,5 @@ fn new<'a>(args: &ArgMatches<'a>, conn: &Connection) {
open_registrations: open_reg,
short_description_html: String::new(),
long_description_html: String::new()
});
}).expect("Couldn't save instance");
}
2 changes: 1 addition & 1 deletion plume-cli/src/search.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ fn refill<'a>(args: &ArgMatches<'a>, conn: &Connection, searcher: Option<Searche
let len = posts.len();
for (i,post) in posts.iter().enumerate() {
println!("Importing {}/{} : {}", i+1, len, post.title);
searcher.update_document(conn, &post);
searcher.update_document(conn, &post).expect("Couldn't import post");
}
println!("Commiting result");
searcher.commit();
Expand Down
5 changes: 3 additions & 2 deletions plume-cli/src/users.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ fn new<'a>(args: &ArgMatches<'a>, conn: &Connection) {
admin,
&bio,
email,
User::hash_pass(&password),
).update_boxes(conn);
User::hash_pass(&password).expect("Couldn't hash password"),
).expect("Couldn't save new user")
.update_boxes(conn).expect("Couldn't update ActivityPub informations for new user");
}
37 changes: 18 additions & 19 deletions plume-common/src/activity_pub/inbox.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use activitypub::{activity::Create, Object};
use activitypub::{activity::Create, Error as ApError, Object};

use activity_pub::Id;

Expand All @@ -13,31 +13,30 @@ pub enum InboxError {
}

pub trait FromActivity<T: Object, C>: Sized {
fn from_activity(conn: &C, obj: T, actor: Id) -> Self;

fn try_from_activity(conn: &C, act: Create) -> bool {
if let Ok(obj) = act.create_props.object_object() {
Self::from_activity(
conn,
obj,
act.create_props
.actor_link::<Id>()
.expect("FromActivity::try_from_activity: id not found error"),
);
true
} else {
false
}
type Error: From<ApError>;

fn from_activity(conn: &C, obj: T, actor: Id) -> Result<Self, Self::Error>;

fn try_from_activity(conn: &C, act: Create) -> Result<Self, Self::Error> {
Self::from_activity(
conn,
act.create_props.object_object()?,
act.create_props.actor_link::<Id>()?,
)
}
}

pub trait Notify<C> {
fn notify(&self, conn: &C);
type Error;

fn notify(&self, conn: &C) -> Result<(), Self::Error>;
}

pub trait Deletable<C, A> {
fn delete(&self, conn: &C) -> A;
fn delete_id(id: &str, actor_id: &str, conn: &C);
type Error;

fn delete(&self, conn: &C) -> Result<A, Self::Error>;
fn delete_id(id: &str, actor_id: &str, conn: &C) -> Result<A, Self::Error>;
}

pub trait WithInbox {
Expand Down
4 changes: 2 additions & 2 deletions plume-common/src/activity_pub/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ pub fn broadcast<S: sign::Signer, A: Activity, T: inbox::WithInbox + Actor>(

let mut act = serde_json::to_value(act).expect("activity_pub::broadcast: serialization error");
act["@context"] = context();
let signed = act.sign(sender);
let signed = act.sign(sender).expect("activity_pub::broadcast: signature error");
Copy link
Contributor

Choose a reason for hiding this comment

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

i thought we're trying to avoid panics?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahah, right… It actually doesn't add a panic, we just panic "outside" of the function instead of "inside" because it wouldn't make much sense to return a Result from broadcast as it is always the main function of its thread.


for inbox in boxes {
// TODO: run it in Sidekiq or something like that
Expand All @@ -130,7 +130,7 @@ pub fn broadcast<S: sign::Signer, A: Activity, T: inbox::WithInbox + Actor>(
let res = Client::new()
.post(&inbox)
.headers(headers.clone())
.header("Signature", request::signature(sender, &headers))
.header("Signature", request::signature(sender, &headers).expect("activity_pub::broadcast: request signature error"))
.body(body)
.send();
match res {
Expand Down
6 changes: 3 additions & 3 deletions plume-common/src/activity_pub/request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ pub fn headers() -> HeaderMap {
headers
}

pub fn signature<S: Signer>(signer: &S, headers: &HeaderMap) -> HeaderValue {
pub fn signature<S: Signer>(signer: &S, headers: &HeaderMap) -> Result<HeaderValue, ()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you briefly explain this return signature?
maybe then I'll also understand the above expect()

Copy link
Member Author

Choose a reason for hiding this comment

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

It says that it can either success with a HeaderValue, or fail with (), so we don't have much details about the error, we just know it failed, but it is probably enough to know that something failed in this function (or maybe not?)

let signed_string = headers
.iter()
.map(|(h, v)| {
Expand All @@ -125,13 +125,13 @@ pub fn signature<S: Signer>(signer: &S, headers: &HeaderMap) -> HeaderValue {
.join(" ")
.to_lowercase();

let data = signer.sign(&signed_string);
let data = signer.sign(&signed_string).map_err(|_| ())?;
let sign = base64::encode(&data);

HeaderValue::from_str(&format!(
"keyId=\"{key_id}\",algorithm=\"rsa-sha256\",headers=\"{signed_headers}\",signature=\"{signature}\"",
key_id = signer.get_key_id(),
signed_headers = signed_headers,
signature = sign
)).expect("request::signature: signature header error")
)).map_err(|_| ())
}
18 changes: 10 additions & 8 deletions plume-common/src/activity_pub/sign.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,18 @@ pub fn gen_keypair() -> (Vec<u8>, Vec<u8>) {
}

pub trait Signer {
type Error;

fn get_key_id(&self) -> String;

/// Sign some data with the signer keypair
fn sign(&self, to_sign: &str) -> Vec<u8>;
fn sign(&self, to_sign: &str) -> Result<Vec<u8>, Self::Error>;
/// Verify if the signature is valid
fn verify(&self, data: &str, signature: &[u8]) -> bool;
fn verify(&self, data: &str, signature: &[u8]) -> Result<bool, Self::Error>;
}

pub trait Signable {
fn sign<T>(&mut self, creator: &T) -> &mut Self
fn sign<T>(&mut self, creator: &T) -> Result<&mut Self, ()>
where
T: Signer;
fn verify<T>(self, creator: &T) -> bool
Expand All @@ -45,7 +47,7 @@ pub trait Signable {
}

impl Signable for serde_json::Value {
fn sign<T: Signer>(&mut self, creator: &T) -> &mut serde_json::Value {
fn sign<T: Signer>(&mut self, creator: &T) -> Result<&mut serde_json::Value, ()> {
let creation_date = Utc::now().to_rfc3339();
let mut options = json!({
"type": "RsaSignature2017",
Expand All @@ -62,11 +64,11 @@ impl Signable for serde_json::Value {
let document_hash = Self::hash(&self.to_string());
let to_be_signed = options_hash + &document_hash;

let signature = base64::encode(&creator.sign(&to_be_signed));
let signature = base64::encode(&creator.sign(&to_be_signed).map_err(|_| ())?);

options["signatureValue"] = serde_json::Value::String(signature);
self["signature"] = options;
self
Ok(self)
}

fn verify<T: Signer>(mut self, creator: &T) -> bool {
Expand Down Expand Up @@ -107,7 +109,7 @@ impl Signable for serde_json::Value {
}
let document_hash = Self::hash(&self.to_string());
let to_be_signed = options_hash + &document_hash;
creator.verify(&to_be_signed, &signature)
creator.verify(&to_be_signed, &signature).unwrap_or(false)
}
}

Expand Down Expand Up @@ -167,7 +169,7 @@ pub fn verify_http_headers<S: Signer + ::std::fmt::Debug>(
.collect::<Vec<_>>()
.join("\n");

if !sender.verify(&h, &base64::decode(signature).unwrap_or_default()) {
if !sender.verify(&h, &base64::decode(signature).unwrap_or_default()).unwrap_or(false) {
return SignatureValidity::Invalid;
}
if !headers.contains(&"digest") {
Expand Down
32 changes: 25 additions & 7 deletions plume-models/src/api_tokens.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use rocket::{

use db_conn::DbConn;
use schema::api_tokens;
use {Error, Result};

#[derive(Clone, Queryable)]
pub struct ApiToken {
Expand Down Expand Up @@ -63,22 +64,39 @@ impl ApiToken {
}
}

#[derive(Debug)]
pub enum TokenError {
/// The Authorization header was not present
NoHeader,

/// The type of the token was not specified ("Basic" or "Bearer" for instance)
NoType,

/// No value was provided
NoValue,

/// Error while connecting to the database to retrieve all the token metadata
DbError,
}

impl<'a, 'r> FromRequest<'a, 'r> for ApiToken {
type Error = ();
type Error = TokenError;

fn from_request(request: &'a Request<'r>) -> request::Outcome<ApiToken, ()> {
fn from_request(request: &'a Request<'r>) -> request::Outcome<ApiToken, TokenError> {
let headers: Vec<_> = request.headers().get("Authorization").collect();
if headers.len() != 1 {
return Outcome::Failure((Status::BadRequest, ()));
return Outcome::Failure((Status::BadRequest, TokenError::NoHeader));
}

let mut parsed_header = headers[0].split(' ');
let auth_type = parsed_header.next().expect("Expect a token type");
let val = parsed_header.next().expect("Expect a token value");
let auth_type = parsed_header.next()
.map_or_else(|| Outcome::Failure((Status::BadRequest, TokenError::NoType)), |t| Outcome::Success(t))?;
let val = parsed_header.next()
.map_or_else(|| Outcome::Failure((Status::BadRequest, TokenError::NoValue)), |t| Outcome::Success(t))?;

if auth_type == "Bearer" {
let conn = request.guard::<DbConn>().expect("Couldn't connect to DB");
if let Some(token) = ApiToken::find_by_value(&*conn, val) {
let conn = request.guard::<DbConn>().map_failure(|_| (Status::InternalServerError, TokenError::DbError))?;
if let Ok(token) = ApiToken::find_by_value(&*conn, val) {
return Outcome::Success(token);
}
}
Expand Down
12 changes: 6 additions & 6 deletions plume-models/src/apps.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
use canapi::{Error, Provider};
use canapi::{Error as ApiError, Provider};
use chrono::NaiveDateTime;
use diesel::{self, ExpressionMethods, QueryDsl, RunQueryDsl};

use plume_api::apps::AppEndpoint;
use plume_common::utils::random_hex;
use schema::apps;
use Connection;
use {Connection, Error, Result, ApiResult};

#[derive(Clone, Queryable)]
pub struct App {
Expand All @@ -31,15 +31,15 @@ pub struct NewApp {
impl Provider<Connection> for App {
type Data = AppEndpoint;

fn get(_conn: &Connection, _id: i32) -> Result<AppEndpoint, Error> {
fn get(_conn: &Connection, _id: i32) -> ApiResult<AppEndpoint> {
unimplemented!()
}

fn list(_conn: &Connection, _query: AppEndpoint) -> Vec<AppEndpoint> {
unimplemented!()
}

fn create(conn: &Connection, data: AppEndpoint) -> Result<AppEndpoint, Error> {
fn create(conn: &Connection, data: AppEndpoint) -> ApiResult<AppEndpoint> {
let client_id = random_hex();

let client_secret = random_hex();
Expand All @@ -52,7 +52,7 @@ impl Provider<Connection> for App {
redirect_uri: data.redirect_uri,
website: data.website,
},
);
).map_err(|_| ApiError::NotFound("Couldn't register app".into()))?;

Ok(AppEndpoint {
id: Some(app.id),
Expand All @@ -64,7 +64,7 @@ impl Provider<Connection> for App {
})
}

fn update(_conn: &Connection, _id: i32, _new_data: AppEndpoint) -> Result<AppEndpoint, Error> {
fn update(_conn: &Connection, _id: i32, _new_data: AppEndpoint) -> ApiResult<AppEndpoint> {
unimplemented!()
}

Expand Down
1 change: 1 addition & 0 deletions plume-models/src/blog_authors.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use diesel::{self, ExpressionMethods, QueryDsl, RunQueryDsl};

use schema::blog_authors;
use {Error, Result};

#[derive(Clone, Queryable, Identifiable)]
pub struct BlogAuthor {
Expand Down
Loading