OxideTalis/oxidetalis

Fork 0

refactor: Use `PublicKey` and `Signature` as parameters #33

Manually merged

awiteb merged 9 commits from awiteb/public-key_and_siganture_as_parameters into master

2024-07-28 11:56:46 +02:00

AGit

awiteb commented

2024-07-27 01:58:54 +02:00

Owner

Fix #21

Changes

Implement salvo_oapi::Extractible and salvo_oapi::EndpointArgRegister for PublicKey
Implement salvo_oapi::ToParameters for Signature

Notes

I did not implement salvo_oapi::ToParameters for PublicKey because it will
not be used as an OpenAPI parameter. Instead, Salvo will register it using the
EndpointArgRegister trait.

Similarly, I did not implement salvo_oapi::EndpointArgRegister for Signature
because it will not be used as an argument in the endpoint. Instead, the
signature will be verified by the signature middleware, and we will only use it
as a parameter.

Although there is an existing salvo_oapi::Extractible implementation for
Signature, it is currently unreachable. The salvo_oapi::ToParameters
requires it unnecessarily. If salvo_oapi::ToParameters no longer requires it
in the future, I will remove it. I did not add a FIXME because its removal is
uncertain, and there is an open issue regarding this matter here,
which has not yet received a response.

Fix https://git.4rs.nl/OxideTalis/oxidetalis/issues/21 ### Changes - Implement `salvo_oapi::Extractible` and `salvo_oapi::EndpointArgRegister` for `PublicKey` - Implement `salvo_oapi::ToParameters` for `Signature` ### Notes I did not implement `salvo_oapi::ToParameters` for `PublicKey` because it will not be used as an OpenAPI parameter. Instead, Salvo will register it using the `EndpointArgRegister` trait. Similarly, I did not implement `salvo_oapi::EndpointArgRegister` for `Signature` because it will not be used as an argument in the endpoint. Instead, the signature will be verified by the signature middleware, and we will only use it as a parameter. Although there is an existing `salvo_oapi::Extractible` implementation for `Signature`, it is currently unreachable. The `salvo_oapi::ToParameters` requires it unnecessarily. If `salvo_oapi::ToParameters` no longer requires it in the future, I will remove it. I did not add a FIXME because its removal is uncertain, and there is an open issue regarding this matter [here][issue1], which has not yet received a response. [issue1]: https://github.com/salvo-rs/salvo/issues/838

awiteb added 6 commits 2024-07-27 01:58:54 +02:00

chore: Move serde_json to be shared dependencie 9f3fd659d2

Signed-off-by: Awiteb <a@4rs.nl>

chore: Add serde_json & salvo_core to core dependencies 6fc14d380b

Signed-off-by: Awiteb <a@4rs.nl>

remove: Remove Public key middleware public_key_check 2b282cbec6

Signed-off-by: Awiteb <a@4rs.nl>

chore: Imple openAPI traits for PublicKey and Signature 1d0268c501

Signed-off-by: Awiteb <a@4rs.nl>

chore: Use Signature as a openAPI parameter 531e27ab2c

Signed-off-by: Awiteb <a@4rs.nl>

chore: Use PublicKey as an argument and openapi doc

DCO checker / DCO checker (pull_request) Successful in 14s

Details

Rust CI / Rust CI (pull_request) Successful in 4m39s

Details

f4df5b26d3

Signed-off-by: Awiteb <a@4rs.nl>

awiteb requested review from Amjad50 2024-07-27 02:15:18 +02:00

Amjad50 commented

2024-07-27 12:21:16 +02:00

Member

you can fix the link to the salvo issue in the PR description ...regarding this matter [here][issue1].

Beside that, this is okay. though I notice that in signature_check middleware, we still use utils::extract_signature and utils::extract_public_key. can't we use these directly in the parameter? maybe we would need to implement the extraction correctly for Signature

you can fix the link to the salvo issue in the PR description `...regarding this matter [here][issue1]`. Beside that, this is okay. though I notice that in `signature_check` middleware, we still use `utils::extract_signature` and `utils::extract_public_key`. can't we use these directly in the parameter? maybe we would need to implement the extraction correctly for `Signature`

awiteb commented

2024-07-27 12:27:11 +02:00

Author

Owner

you can fix the link to the salvo issue in the PR description ...regarding this matter [here][issue1].

Sorry, here it's https://github.com/salvo-rs/salvo/issues/838

can't we use these directly in the parameter? maybe we would need to implement the extraction correctly for Signature

No, because it is an oxidetalis utility, so we can't use it in the core

> you can fix the link to the salvo issue in the PR description ...regarding this matter [here][issue1]. Sorry, here it's https://github.com/salvo-rs/salvo/issues/838 > can't we use these directly in the parameter? maybe we would need to implement the extraction correctly for Signature No, because it is an `oxidetalis` utility, so we can't use it in the core

awiteb commented

2024-07-27 12:31:38 +02:00

Author

Owner

Oh, I get it. Yes we can use extract function. I'll fix that

Oh, I get it. Yes we can use `extract` function. I'll fix that

Amjad50 added the

Status

Waiting On Author

label 2024-07-28 02:23:00 +02:00

awiteb force-pushed awiteb/public-key_and_siganture_as_parameters from c103a4fef1 to c137129488

2024-07-28 08:49:20 +02:00

Compare

awiteb added

and removed

labels 2024-07-28 08:50:18 +02:00

Amjad50 commented

2024-07-28 10:36:15 +02:00

Member

This is good, but my idea was also removing all of these calls and instead just using it in the arguments.

#[handler]
pub async fn signature_check(
    req: &mut Request,
    res: &mut Response,
    depot: &mut Depot,
    ctrl: &mut FlowCtrl,
    signature: Signature,
    sender_public_key: PublicKey,
)

This is good, but my idea was also removing all of these calls and instead just using it in the arguments. ```rust #[handler] pub async fn signature_check( req: &mut Request, res: &mut Response, depot: &mut Depot, ctrl: &mut FlowCtrl, signature: Signature, sender_public_key: PublicKey, ) ```

awiteb commented

2024-07-28 11:09:28 +02:00

Author

Owner

but my idea was also removing all of these calls and instead just using it in the arguments.

Middlewares cannot return a response; they can only modify it. As you can see, the response is mutable.

> but my idea was also removing all of these calls and instead just using it in the arguments. Middlewares cannot return a response; they can only modify it. As you can see, the response is mutable.

awiteb commented

2024-07-28 11:19:08 +02:00

Author

Owner

Nice, Salvo actually modifies the response if there is an error when extracting an argument.

Nice, Salvo actually modifies the response if there is an error when extracting an argument. <img width="544" alt="image" src="/attachments/fedee1ea-76bd-44b0-a3de-090409fa6e98">

image.png

41 KiB

image.png

42 KiB

Amjad50 commented

2024-07-28 11:20:18 +02:00

Member

but my idea was also removing all of these calls and instead just using it in the arguments.

Middlewares cannot return a response; they can only modify it. As you can see, the response is mutable.

not about the return, just taking Signature and PublicKey as arguments, and it works.

> > but my idea was also removing all of these calls and instead just using it in the arguments. > > Middlewares cannot return a response; they can only modify it. As you can see, the response is mutable. not about the return, just taking `Signature` and `PublicKey` as arguments, and it works.

awiteb commented

2024-07-28 11:23:22 +02:00

Author

Owner

not about the return, just taking Signature and PublicKey as arguments, and it works.

I thought Salvo returning an extraction error, but it's modifying the response.

> not about the return, just taking Signature and PublicKey as arguments, and it works. I thought Salvo returning an extraction error, but it's modifying the response.

👍 1

awiteb added 1 commit 2024-07-28 11:27:21 +02:00

chore: Let salvo extract the public key and the signature

DCO checker / DCO checker (pull_request) Successful in 4s

Details

Rust CI / Rust CI (pull_request) Successful in 4m22s

Details

71d63580bf