Convert authentication trait to associated async trait

[nemosupremo]

(#34)

This is a change I've worked on in a separate fork as I needed the authenticate function to be async. This is a breaking change, as it changes the signature of the Authenticate trait. This doesn't depend on async_trait, at the cost of making the trait a bit more difficult to implement. However this allows the library user to implement Authenticate without boxing.

If one didn't want to use concrete types, they could box their implementation instead:

use futures::FutureExt;

pub struct MyAuth {}

impl Authentication for MyAuth {
    type Item<'a> = Pin<Box<dyn Future<Output = bool> + Send + 'a>>;

    fn authenticate<'a>(&'a self, username: &str, password: &str) -> Self::Item<'a> {
        let username = username.to_owned();
        let password = password.to_owned();
        async {
            let _username = username;
            let _password = password;
            tokio::time::sleep(std::time::Duration::from_millis(100)).await;
            true
        }
        .boxed()
    }
}

More advanced implementions can be implemented via polling. Secondly, now that Authentication is a associated trait, it can no longer by type erased (dyn Authentication) which means it is now a generic argument on Config (and on all structs that carry Config), which has led to changing some function signatures.

[dizda]

Thanks for your PR, hoping to review it when I get some spare time.

[dizda]

Hey @nemosupremo, rather than having an authenticate function that simply returns a boolean.. would that makes sense to return an Option<T> (which T is a generator)?

So if the authentication succeed, I could simply return the User's info, because at the moment we loose the user's data, which is a pity.

Wdyt?

[dizda]

So the signature should look like:

pub trait Authentication<T>: Send + Sync {
    type Item<'a>: Future<Output = T> + 'a
    where
        Self: 'a;

    fn authenticate<'a>(&'a self, username: &str, password: &str) -> Self::Item<'a>;

[nemosupremo]

type Item<'a>: Future

+ 'a

I think this would require GAT, and I'm not sure how flexible Rust's current implementation of that is. Last I remember there were a number of limitations that didn't make it particularly attractive.

[dizda]

Unless we use async_trait. How much of a downside would it be to use it you reckon?

[dizda]

At the moment I have to create a config file / TCP connection, in order to get the User that has authenticated successfully, eg:

pub struct Socks5Authentication {
    // pub username: String,
    // pub password: String,
    proxy_accounts: ProxyAccounts,
    headers: Headers,
    account: Mutex<Option<Arc<RwLock<ProxyAccount>>>>,
}

impl Authentication for Socks5Authentication {
    // type Item<'a> = futures::future::Ready<bool>;
    type Item<'a> = Pin<Box<dyn Future<Output = bool> + Send + 'a>>;

    fn authenticate<'a>(&'a self, username: &str, password: &str) -> Self::Item<'a> {
        let basic_auth = BasicAuth::new(&username, &password);
        let proxy_req = ProxyRequest::try_from(basic_auth);

        async {
            // move the lifetime inside the async function
            let mut proxy_req = match proxy_req {
                Ok(r) => r,
                Err(err) => {
                    error!(%err, "Authentication failed");
                    return false;
                }
            };

            let res = Auth::new(&self.proxy_accounts, &self.headers, &mut proxy_req)
                .challenge()
                .await;

            match res {
                Ok(proxy_account) => {
                    *self.account.lock() = Some(proxy_account);
                    true
                }
                Err(err) => {
                    false
                }
            }
        }
        .boxed()
    }
}

which is quite inefficient, the config file need to be created for every TCP connection.

[nemosupremo]

I don’t mind adding async_trait, the reason I opted against it was to avoid adding dependencies because every library owner has their own view on new dependencies.

[dizda]

Great, I don't mind it either ;) do you have time to implement it or you want me to do it?

[dizda]

maybe consider to change the futures deps with futures_lite

[dizda]

I'm working on it FYI.

[dizda]

I've added further commits, please see https://github.com/dizda/fast-socks5/pull/37