Support a client-id allowlist

[djmitche]

This will support setting up publicly-accessible personal servers, without also allowing anyone to create a new client.

[djmitche]

@Necior do you want to have a look?

[Necior]

Would it make sense to have allow_client: impl Fn(Uuid) -> bool instead?

---

Whenever I see Option<SomeContainer<T>>, I ask if None and an empty container mean different things. And whether SomeContainer<T> would suffice.

In this case there is a huge difference between None (allow everyone) and an empty set (forbid everyone). We can't just drop the outer Option<> here.

But if we take a step back, it seems like WebServer doesn't really need allowed client IDs. What it needs is a predicate "is a given client ID allowed?", a function from Uuid to bool.

It could be a closure with a HashSet inside if --allow-client-id is specified, or a function that always returns true otherwise.

[djmitche]

Thinking about how to implement this, in main we would want to make some intermediate data structure to do the lookup. If that data structure is of type Option<HashSet<Uuid>> then we are just doing a more complicated version of the current version. Processing matches into a HashSet<Uuid> on each invocation is pretty inefficient.

The other alternative is to create two closures: one for the "allow any" case and one for the "allow from this HashSet" case:

    let allow_client_id;                                   
    if let Some(ids) = matches.get_many("allow-client-id") {
        let ids: HashSet<Uuid> = ids.copied().collect();                    
        allow_client_id = move |client_id| ids.contains(client_id);
    } else {                                
        allow_client_id = |_| true
    }

but

   |
59 |         allow_client_id = move |client_id| ids.contains(client_id);
   |                           ---------------- the expected closure
60 |     } else {
61 |         allow_client_id = |_| true
   |                           ^^^^^^^^ expected closure, found a different closure
   |
   = note: expected closure `{closure@server/src/bin/taskchampion-sync-server.rs:59:27: 59:43}`
              found closure `{closure@server/src/bin/taskchampion-sync-server.rs:61:27: 61:30}`
   = note: no two closures, even if identical, have the same type
   = help: consider boxing your closure and/or using it as a trait object

Boxing or dyn Fn seem to push the complexity well beyond the relatively simple Option<HashSet<Uuid>> solution.

This is all within the server crate, not a public API, so I'm not too worried about it.

[Necior]

no two closures, even if identical, have the same type

Oh, indeed.

Thinking about how to implement this, in main we would want to make some intermediate data structure to do the lookup. If that data structure is of type Option<HashSet<Uuid>> then we are just doing a more complicated version of the current version.

Yeah, constructing a single closure works:

use std::collections::HashSet;

fn check_access(client_id: i32, predicate: impl Fn(i32) -> bool) -> bool {
    predicate(client_id)
}

fn main() {
    let allowed_ids = {
        let mut hs = HashSet::new();
        hs.insert(1);
        hs.insert(3);
        hs
    };
    let client_id_allowlist: Option<HashSet<i32>> = Some(allowed_ids);

    // Key part here, construction of the closure.
    let allow_client_id = |client_id: i32| {
        if let Some(set) = &client_id_allowlist {
            set.contains(&client_id)
        } else {
            true
        }
    };

    for i in 1..3 {
        println!("{i} access: {}", check_access(i, allow_client_id));
    }
}

It does hide complexity from the users of the predicate (they don't need to know the data structure inside and how to query it) but at the expense of a complex closure construction (as shown above).

This is all within the server crate, not a public API, so I'm not too worried about it.

Agreed. Internals are much easier to change.