-
Notifications
You must be signed in to change notification settings - Fork 178
RUST-1663: Implement and enable reauthentication for OIDC #1044
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2aaaf20
e4667d0
3966e71
a6dcd29
3464d03
c6e98e0
8d3a7b7
22d8ddd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -280,7 +280,8 @@ impl Client { | |
} | ||
|
||
/// Selects a server and executes the given operation on it, optionally using a provided | ||
/// session. Retries the operation upon failure if retryability is supported. | ||
/// session. Retries the operation upon failure if retryability is supported or after | ||
/// reauthenticating if reauthentication is required. | ||
async fn execute_operation_with_retry<T: Operation>( | ||
&self, | ||
mut op: T, | ||
|
@@ -397,6 +398,30 @@ impl Client { | |
implicit_session, | ||
}, | ||
Err(mut err) => { | ||
// If the error is a reauthentication required error, we reauthenticate and | ||
// retry the operation. | ||
if err.is_reauthentication_required() { | ||
let credential = self.inner.options.credential.as_ref().ok_or( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. both of these errors should be impossible under the current sever spec, but this does future proof us. |
||
ErrorKind::Authentication { | ||
message: "No Credential when reauthentication required error \ | ||
occured" | ||
.to_string(), | ||
}, | ||
)?; | ||
let server_api = self.inner.options.server_api.as_ref(); | ||
|
||
credential | ||
.mechanism | ||
.as_ref() | ||
.ok_or(ErrorKind::Authentication { | ||
message: "No AuthMechanism when reauthentication required error \ | ||
occured" | ||
.to_string(), | ||
})? | ||
.reauthenticate_stream(&mut conn, credential, server_api) | ||
.await?; | ||
continue; | ||
} | ||
err.wire_version = conn.stream_description()?.max_wire_version; | ||
|
||
// Retryable writes are only supported by storage engines with document-level | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,17 @@ | ||
use crate::{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I decided to correct these two tests in this PR as a start on the rest of the prose tests. These tests need to assert the number of times the callback is called.... getting a shared Arc into closures with async blocks is nearly impossible. There does not appear to be a way to do it without the top level function here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The ownership here is gnarly, but it can be made to work: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=437cdfd395260f80a01ddce572d97d52 The trick is that you need to This can be boiled down a little bit to remove the async aspect, which makes it easier to see what's going on: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=5f8a27ba1ae567f9ba337489d6a49a40 This is also why the distinction between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting. I thought I tried cloning twice and that was what was really shocking to me. I must be misremembering |
||
client::{ | ||
auth::{oidc, AuthMechanism, Credential}, | ||
options::ClientOptions, | ||
}, | ||
test::log_uncaptured, | ||
Client, | ||
}; | ||
use std::sync::{Arc, Mutex}; | ||
|
||
// Machine Callback tests | ||
// Prose test 1.1 Single Principal Implicit Username | ||
#[tokio::test] | ||
async fn single_principal_implicit_username() -> anyhow::Result<()> { | ||
use crate::{ | ||
client::{ | ||
auth::{oidc, AuthMechanism, Credential}, | ||
options::ClientOptions, | ||
}, | ||
test::log_uncaptured, | ||
Client, | ||
}; | ||
async fn machine_single_principal_implicit_username() -> anyhow::Result<()> { | ||
use bson::Document; | ||
use futures_util::FutureExt; | ||
|
||
|
@@ -17,10 +20,16 @@ async fn single_principal_implicit_username() -> anyhow::Result<()> { | |
return Ok(()); | ||
} | ||
|
||
// we need to assert that the callback is only called once | ||
let call_count = Arc::new(Mutex::new(0)); | ||
let cb_call_count = call_count.clone(); | ||
|
||
let mut opts = ClientOptions::parse("mongodb://localhost/?authMechanism=MONGODB-OIDC").await?; | ||
opts.credential = Credential::builder() | ||
.mechanism(AuthMechanism::MongoDbOidc) | ||
.oidc_callback(oidc::Callback::machine(|_| { | ||
.oidc_callback(oidc::Callback::machine(move |_| { | ||
let call_count = cb_call_count.clone(); | ||
*call_count.lock().unwrap() += 1; | ||
async move { | ||
Ok(oidc::IdpServerResponse { | ||
access_token: tokio::fs::read_to_string("/tmp/tokens/test_user1").await?, | ||
|
@@ -38,14 +47,14 @@ async fn single_principal_implicit_username() -> anyhow::Result<()> { | |
.collection::<Document>("test") | ||
.find_one(None, None) | ||
.await?; | ||
assert_eq!(1, *(*call_count).lock().unwrap()); | ||
Ok(()) | ||
} | ||
|
||
// TODO RUST-1497: The following test will be removed because it is not an actual test in the spec, | ||
// but just showing that the human flow is still working for two_step (nothing in caching is | ||
// correctly exercised here) | ||
// Human Callback tests | ||
// Prose test 1.1 Single Principal Implicit Username | ||
#[tokio::test] | ||
async fn human_flow() -> anyhow::Result<()> { | ||
async fn human_single_principal_implicit_username() -> anyhow::Result<()> { | ||
use crate::{ | ||
client::{ | ||
auth::{oidc, AuthMechanism, Credential}, | ||
|
@@ -62,10 +71,16 @@ async fn human_flow() -> anyhow::Result<()> { | |
return Ok(()); | ||
} | ||
|
||
// we need to assert that the callback is only called once | ||
let call_count = Arc::new(Mutex::new(0)); | ||
let cb_call_count = call_count.clone(); | ||
|
||
let mut opts = ClientOptions::parse("mongodb://localhost/?authMechanism=MONGODB-OIDC").await?; | ||
opts.credential = Credential::builder() | ||
.mechanism(AuthMechanism::MongoDbOidc) | ||
.oidc_callback(oidc::Callback::human(|_| { | ||
.oidc_callback(oidc::Callback::human(move |_| { | ||
let call_count = cb_call_count.clone(); | ||
*call_count.lock().unwrap() += 1; | ||
async move { | ||
Ok(oidc::IdpServerResponse { | ||
access_token: tokio::fs::read_to_string("/tmp/tokens/test_user1").await?, | ||
|
@@ -83,5 +98,6 @@ async fn human_flow() -> anyhow::Result<()> { | |
.collection::<Document>("test") | ||
.find_one(None, None) | ||
.await?; | ||
assert_eq!(1, *(*call_count).lock().unwrap()); | ||
Ok(()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem that cfg is allowed within a pattern, unfortunately.