Refactor API: replace Warp with Axum

[josecelano]

UPDATED: 12/01/2022

The tracker API uses the web framework Warp.
This PR replaces Warp with Axum.

Tasks

• Basic Axum configuration.
• Authentication using token in GET params.
• Enable SSL.
• Add tests for the current API. Error cases are not tested. I think all cases are when there is a database error, and it returns an ActionStatus::Err error with a message.
• Reimplement endpoints. See subtasks.
• Improve asserts by checking the response body for OK (200) responses.
• Use the same URL prefix as the current API /api/xxx.
• Remove duplicate definitions of routes in Axum server (start and start_tls)
• Add tests for the current API. Error cases when GET params cannot be parsed are not tested. info_hash and seconds_valid Path params, and offset and limit GET params.
• Fix a bug with the auth token. The token must be the first param in the query otherwise, it does not work. In the end, it was not a bug. It only happens with curl.
• Add test for the current API. When you try to remove a torrent from the whitelist twice, you get a 500 response with this body
  : Unhandled rejection: Err { reason: "failed to remove torrent from whitelist" }
• Add some tests for malicious user input.
• Remove the Warp implementation.

Endpoints subtasks

Stats:

• GET /api/stats

Torrents:

• GET /api/torrents?offset=:u32&limit=:u32
• GET /api/torrent/:info_hash

Whitelisted torrents:

• POST /api/whitelist/:info_hash
• DELETE /api/whitelist/:info_hash

Whitelist commands:

• GET /api/whitelist/reload

Keys:

• POST /api/key/:seconds_valid
• DELETE /api/key/:key

Key commands

• GET /api/keys/reload

[da2ce7]

prefer expects over unwrap?

[josecelano]

I've changed it, but we have a lot of "unwraps". For example like this:

InfoHash::from_str(&hash).unwrap();

I think it would not be very pleasant to write a message for all of them:

InfoHash::from_str(&hash).expect("string should be a valid infohash");

I have to think about it, but I would say it makes sense when the case is unique.

Maybe the problem is we are duplicating code, and we should add a function like this:

fn parse_socket_addr_or_fail(bind_address: &str) -> SocketAddr {
    bind_address.parse().expect("the bind address string should contain a valid socket address like this 127.0.1.1:8080")
}

[josecelano]

hi @da2ce7 @WarmBeer It seems Axum does not support SSL directly. I can use the axum-server, which is the one used in this fork.

[josecelano]

hi @da2ce7 @WarmBeer It seems Axum does not support SSL directly. I can use the axum-server, which is the one used in this fork.

I did it using axum-server.

[josecelano]

I'm working on the GET /api/torrent/:info_hash endpoint. I'm getting an error in the serialization of an u128 field.

#[derive(Serialize, Deserialize, Debug, PartialEq, Eq)]
pub struct Peer {
    pub peer_id: Id,
    pub peer_addr: String,
    #[deprecated(since = "2.0.0", note = "please use `updated_milliseconds_ago` instead")]
    pub updated: u128,
    pub updated_milliseconds_ago: u128,
    pub uploaded: i64,
    pub downloaded: i64,
    pub left: i64,
    pub event: String,
}

I have commented on this issue serde-rs/json#502

I'm going to find out why it's working in the current API (if it's)

[josecelano]

The happy path for the endpoint GET /api/torrent/:info_hash is done. I've realized I do not have a test for the case where the torrent does not exist. I will add the test for the current API and implement it in the new Axum API.

[josecelano]

I'm working on the GET /api/torrent/:info_hash endpoint. I'm getting an error in the serialization of an u128 field.

#[derive(Serialize, Deserialize, Debug, PartialEq, Eq)]
pub struct Peer {
    pub peer_id: Id,
    pub peer_addr: String,
    #[deprecated(since = "2.0.0", note = "please use `updated_milliseconds_ago` instead")]
    pub updated: u128,
    pub updated_milliseconds_ago: u128,
    pub uploaded: i64,
    pub downloaded: i64,
    pub left: i64,
    pub event: String,
}

I have commented on this issue serde-rs/json#502

I'm going to find out why it's working in the current API (if it's)

It works if I do not use serde directly:

pub async fn get_torrent(State(tracker): State<Arc<Tracker>>, Path(info_hash): Path<String>) -> Response {
    let optional_torrent_info = get_torrent_info(tracker.clone(), &InfoHash::from_str(&info_hash).unwrap()).await;

    match optional_torrent_info {
        Some(info) => Json(Torrent::from(info)).into_response(),
        None => Json(json!("torrent not known")).into_response(),
    }
}

Probably for the same reason it's working on the old Warp implementation.

[josecelano]

hi, @WarmBeer I think I've found a bug in both the current API implementation and the new one with Axum. It seems the token must be the first query param. Curiously enough, I've reproduced the exact error in the new implementation:

curl -v http://127.0.0.1:1212/api/torrents?limit=1&token=MyAccessToken -> 500 unauthorized
curl -v http://127.0.0.1:1212/api/torrents?token=MyAccessToken&limit=1 -> 200 OK

curl -v http://127.0.0.1:1313/api/torrents?limit=1&token=MyAccessToken -> 500 unauthorized
curl -v http://127.0.0.1:1313/api/torrents?token=MyAccessToken&limit=1 -> 200 OK

I suppose that is not the expected behavior.

[josecelano]

hi, @WarmBeer I think I've found a bug in both the current API implementation and the new one with Axum. It seems the token must be the first query param. Curiously enough, I've reproduced the exact error in the new implementation:

curl -v http://127.0.0.1:1212/api/torrents?limit=1&token=MyAccessToken -> 500 unauthorized
curl -v http://127.0.0.1:1212/api/torrents?token=MyAccessToken&limit=1 -> 200 OK

curl -v http://127.0.0.1:1313/api/torrents?limit=1&token=MyAccessToken -> 500 unauthorized
curl -v http://127.0.0.1:1313/api/torrents?token=MyAccessToken&limit=1 -> 200 OK

I suppose that is not the expected behavior.

It seems the problem is only with curl. I've added a new test, and it does not fail.

[josecelano]

hi @da2ce7 @WarmBeer this is ready to review. I would like to refactor a little bit more, but I think the PR is getting too big. We can merge it as it is and I will create more issues.

I have added some todos and code-reviews in the code.

I also want to make some improvements, like reorganising the API mod structure. Right now, the folder structure is like this:

src/apis/
├── handlers.rs
├── middlewares
│   ├── auth.rs
│   └── mod.rs
├── mod.rs
├── resources
│   ├── auth_key.rs
│   ├── mod.rs
│   ├── peer.rs
│   ├── stats.rs
│   └── torrent.rs
├── responses.rs
├── routes.rs
└── server.rs

It's an implementation-detail or technical-detail organization. I would like to use something more like this:

└── src
    ├── apis
    │   ├── contexts
    │   │   ├── auth_key
    │   │   │   ├── handlers.rs
    │   │   │   ├── mod.rs
    │   │   │   ├── resources.rs
    │   │   │   ├── responses.rs
    │   │   │   └── routes.rs
    │   │   ├── mod.rs
    │   │   ├── stats
    │   │   │   ├── handlers.rs
    │   │   │   ├── mod.rs
    │   │   │   ├── resources.rs
    │   │   │   ├── responses.rs
    │   │   │   └── routes.rs
    │   │   ├── torrent
    │   │   │   ├── handlers.rs
    │   │   │   ├── mod.rs
    │   │   │   ├── resources.rs
    │   │   │   ├── responses.rs
    │   │   │   └── routes.rs
    │   │   └── whitelist
    │   │       ├── handlers.rs
    │   │       ├── mod.rs
    │   │       ├── resources.rs
    │   │       ├── responses.rs
    │   │       └── routes.rs
    │   ├── middlewares
    │   │   ├── auth.rs
    │   │   └── mod.rs
    │   ├── mod.rs
    │   ├── routes.rs
    │   └── server.rs
    └── mod.rs

I like the folder structure to be more domain-oriented, and it's also more scalable. If you agree I can add a new issue for that and we can implement it before implementing the new API version.

[josecelano]

ACK 0c3ca87