RUST-1936: Create public interface for oidc authentication

[pmeredit]

No description provided.

[pmeredit]

timeout_seconds was a bad name for an Instant.

[isabelatkinson]

Is this needed? clippy doesn't complain with it commented out

[pmeredit]

I just missed that one :)

[isabelatkinson]

Would it be possible to collapse State and Callback into one type named Callback? State is probably a more accurate name for this type in terms of our internal usage, but API-wise the distinction between the two is not relevant and may be confusing for users when attempting to construct this type.

I'm thinking something like this:

pub struct Callback {
    inner: Arc<Mutex<Option<CallbackInner>>>,
    is_user_provided: bool,
}

type UserCallback = Box<dyn Fn(CallbackContext) -> BoxFuture<'static, Result<IdpServerResponse>> + Send + Sync>;

struct CallbackInner {
    user_callback: Arc<UserCallback>,
    kind: CallbackKind,
    cache: Cache,
}

impl Callback {
    pub fn human(...) -> Self { ... }

    pub fn machine(...) -> Self { ... }
}

[pmeredit]

Yes, I like this better.

[isabelatkinson]

Suggest removing the Default impl for this type and instead adding a pub(crate) constructor method that does the same thing. Trait impls are always public, and I don't think there's any good use case for a user to call State::default().

[pmeredit]

unfortunately, this is needed for the TypedBuilder derive, but I did add a new function, and I changed the other use of default to new.

[isabelatkinson]

Ahh I had forgotten about the TypedBuilder requirement. I missed in the last PR that the field for this type in Credential was made non-optional; was that necessary for implementation/to avoid lots of unwraps? It'd be more consistent with other fields/types for Credential.oidc_callback to be an Option<Callback> (which would make the Default stuff easier since Default for Option is None), but not a huge deal if that doesn't work.

[pmeredit]

Oh, I moved the Option inside the type. I suppose we could have options at several levels. I’m thinking the inner Arc could also be a Box

[pmeredit]

Oh no, I remember now, it’s because of interior mutability requirements: If I set the top level to None at construction time, it's impossible to later change it for azure/gcp builtins.

[pmeredit]

Not sure what is going on with all these failed tests where none of the sub tests actually failed.

[abr-egn]

Not sure what is going on with all these failed tests where none of the sub tests actually failed.

This means that the tests themselves passed but something else in the script failed, gotta look at the script output rather than the test breakdown. In this case it looks like doctests are failing: https://parsley.mongodb.com/evergreen/mongo_rust_driver_load_balancer_test_load_balancer_5.0_patch_a28225810b02f3c4461fce6489e95b6c84805ff2_6632d54c72f6240007a8b920_24_05_01_23_50_38/0/task?bookmarks=0,4892&shareLine=4534

[isabelatkinson]

some small docs suggestions for consistency with the rest of the driver, otherwise looks good!

[isabelatkinson]

Should this be non-pub?

[pmeredit]

some small docs suggestions for consistency with the rest of the driver, otherwise looks good!

I should have looked at doc examples myself 😂