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

Refactor jws to be aligned with specification concepts #143

Closed
KendallWeihe opened this issue May 3, 2024 · 4 comments
Closed

Refactor jws to be aligned with specification concepts #143

KendallWeihe opened this issue May 3, 2024 · 4 comments
Assignees

Comments

@KendallWeihe
Copy link
Contributor

I take issue with a number of things inside the jws crate

  • Duplicate signing function
  • new_from_encoded() and encode() both only used internally (albeit encode() is used in vc.rs but that needs to be changed (TODO working on another ticket for refactoring the jwt crate) -- both of these could be removed
  • we're signing a "JwsHeader" but that doesn't really make sense because what's being signed is both the header and the payload
  • splice_parts is out on it's own, and technically is splicing a compact JS
  • we have JwsError but it's the only error in the crate, so why not just call it Error?

I would propose we rethink this crate altogether with the following design:

(I'm going to also include the "decode" design from this ticket #118)

use dids::{bearer::BearerDid, document::KeySelector};

pub enum Error {
    // ... error cases
}

pub struct Header {
    pub alg: String,
    // ... more properties
}

impl Header {
    pub fn from_bearer_did(
        bearer_did: &BearerDid,
        key_selector: &KeySelector,
        typ: &str,
    ) -> Result<Self, Error> {
        unimplemented!()
    }
}

pub struct Decoded {
    pub header: Header,
    pub payload: Vec<u8>,
    pub signature: String,
    pub parts: Vec<String>,
}

pub struct CompactJws;

impl CompactJws {
    pub fn sign(
        bearer_did: &BearerDid,
        key_selector: &KeySelector,
        header: &Header,
        encoded_payload: &[u8], // NOTE: this is not base64 encoded, that happens internal to this function
    ) -> Result<String, Error> {
        unimplemented!()
    }

    pub fn decode(compact_jws: &str) -> Result<Decoded, Error> {
        unimplemented!()
    }

    pub fn verify(compact_jws: &str) -> Result<Decoded, Error> {
        unimplemented!()
    }
}

These changes would break at least jwt and credentials so if we choose to move forward with this proposal then we should probably string these work items together

@nitro-neal
Copy link
Contributor

why not keep it JwsError and we can have at least one default error for every class?

and yes! I like this proposed way very much, very similar to web5-go right?

not sure why we need this function:

impl Header {
    pub fn from_bearer_did(
        bearer_did: &BearerDid,
        key_selector: &KeySelector,
        typ: &str,
    ) -> Result<Self, Error> {
        unimplemented!()
    }
}

We can just construct the Header internally?

@KendallWeihe
Copy link
Contributor Author

yup I'm cool with sticking with JwsError, it's always a thing I go back and forth on... include the trailing namespace item in the type name or not 🤔

yup, very similar to web5-go

yeah I think I'm on board with not moving forward with the from_bearer_did... it's a convenience thing which in principal is ill-advised, at least for the foreseeable future

@nitro-neal
Copy link
Contributor

The v2 is looking great. #162

Just for transparency, we had an upgrade path planned out to make v2 of jws, jwk, and vc, and once its all in place working nice together can rename and gut out the old one.

So basically if all this looks good we can merge in 162

KendallWeihe added a commit that referenced this issue May 7, 2024
@KendallWeihe
Copy link
Contributor Author

KendallWeihe commented May 7, 2024

Merged. We'll hold off on closing this until we complete #144 and #150 & subsequently rip out the old code.

@KendallWeihe KendallWeihe self-assigned this May 8, 2024
nitro-neal added a commit that referenced this issue May 8, 2024
* Refactor jws #143

* update

* lint

---------

Co-authored-by: Kendall Weihe <[email protected]>
enmand pushed a commit to enmand/web5-rs that referenced this issue May 20, 2024
enmand pushed a commit to enmand/web5-rs that referenced this issue May 20, 2024
* Refactor jws TBD54566975#143

* update

* lint

---------

Co-authored-by: Kendall Weihe <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

2 participants