-
Notifications
You must be signed in to change notification settings - Fork 319
Authentication errors include HTTP responses #3302
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -42,6 +42,39 @@ This error contains several pieces of information: | |||||
|
|
||||||
| - __Correlation ID and Timestamp__: The correlation ID and timestamp identify the request in server-side logs. This information can be useful to support engineers diagnosing unexpected Microsoft Entra ID failures. | ||||||
|
|
||||||
| Many credential errors also carry the HTTP response that motivated them. The example below demonstrates how to access that response in such a case. | ||||||
|
Member
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.
Suggested change
Member
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. But they still get the message you want by default, right? This is just to get more details for...why? You might put that in the comments and even explain in what scenarios they may want to drill into details. Ideally, the message you formatted above is what they see if they just print the error e.g.,
Member
Author
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. Yes; printing the error produces a message like the above which probably contains everything developers need. The raw response is useful mainly because it includes headers. I'll refocus this example on that to avoid implying the response is more generally useful. Really I just want to publish this snippet so that the rare developer who wants the response has some guidance toward getting it
Member
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 think the snippet is fine. I just wanted to make it clear that they probably don't need to, and maybe telling them for what reasons they might do so, like getting headers, like you said. You might just keep the code as-is, but explain a reason they might want to do this i.e., it shouldn't be the norm. |
||||||
|
|
||||||
| ```rust | ||||||
| use azure_core::error::ErrorKind; | ||||||
|
|
||||||
| let result = client.method().await; | ||||||
| if let Err(err) = result { | ||||||
| match err.kind() { | ||||||
| // ErrorKind::Credential indicates an authentication problem | ||||||
| ErrorKind::Credential => { | ||||||
| // a credential error may wrap another error having an HTTP response | ||||||
| if let Some(inner) = err.downcast_ref::<azure_core::Error>() { | ||||||
| if let ErrorKind::HttpResponse { | ||||||
| raw_response: Some(response), | ||||||
| status, | ||||||
| .. | ||||||
| } = inner.kind() | ||||||
| { | ||||||
| let headers = response.headers(); | ||||||
| let body = String::from_utf8_lossy(response.body()); | ||||||
| eprintln!("status: {status}"); | ||||||
| eprintln!("headers: {headers:?}"); | ||||||
| eprintln!("body: {body}"); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| _ => { | ||||||
| // TODO: handle other kinds of error | ||||||
|
Member
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. Tip: idiomatic thing here is |
||||||
| } | ||||||
| } | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| <a id="client-secret"></a> | ||||||
| ## Troubleshoot ClientSecretCredential authentication issues | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -163,16 +163,20 @@ impl ClientAssertion for Client { | |
| }), | ||
| ) | ||
| .await?; | ||
| if resp.status() != StatusCode::Ok { | ||
| let status_code = resp.status(); | ||
| let status = resp.status(); | ||
| if status != StatusCode::Ok { | ||
| let err_headers: ErrorHeaders = resp.headers().get()?; | ||
|
|
||
| return Err( | ||
| azure_core::Error::with_message( | ||
| ErrorKind::HttpResponse { status: status_code, error_code: Some(status_code.canonical_reason().to_string()), raw_response: None }, | ||
| format!("{status_code} response from the OIDC endpoint. Check service connection ID and pipeline configuration. {err_headers}"), | ||
| ) | ||
| let message = format!( | ||
|
Member
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'm assuming this is because I had to spend a couple of seconds to figure that out so it might be worth a comment. |
||
| "{status} response from the OIDC endpoint. Check service connection ID and pipeline configuration. {err_headers}" | ||
| ); | ||
| return Err(azure_core::Error::with_message( | ||
| ErrorKind::HttpResponse { | ||
| status, | ||
| error_code: Some(status.canonical_reason().to_string()), | ||
| raw_response: Some(Box::new(resp)), | ||
| }, | ||
| message, | ||
| )); | ||
| } | ||
|
|
||
| let assertion: Assertion = resp.into_body().json()?; | ||
|
|
@@ -226,9 +230,9 @@ impl fmt::Display for ErrorHeaders { | |
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use crate::{env::Env, TSG_LINK_ERROR_TEXT}; | ||
| use crate::env::Env; | ||
| use azure_core::{ | ||
| http::{AsyncRawResponse, ClientOptions, Transport}, | ||
| http::{AsyncRawResponse, ClientOptions, RawResponse, Transport}, | ||
| Bytes, | ||
| }; | ||
| use azure_core_test::http::MockHttpClient; | ||
|
|
@@ -254,24 +258,25 @@ mod tests { | |
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn error_headers() { | ||
| let mock_client = MockHttpClient::new(|req| { | ||
| async fn error_response() { | ||
| let expected_status = StatusCode::Forbidden; | ||
| let body = Bytes::from_static(b"content"); | ||
| let mut headers = Headers::new(); | ||
| headers.insert(MSEDGE_REF, "foo"); | ||
| headers.insert(VSS_E2EID, "bar"); | ||
| let expected_response = | ||
| RawResponse::from_bytes(expected_status, headers.clone(), body.clone()); | ||
| let headers_for_mock = headers.clone(); | ||
| let body_for_mock = body.clone(); | ||
| let mock_client = MockHttpClient::new(move |req| { | ||
| assert_eq!( | ||
| req.url().as_str(), | ||
| "http://localhost/get_token?api-version=7.1&serviceConnectionId=c" | ||
| ); | ||
| let mut headers = Headers::new(); | ||
| headers.insert(MSEDGE_REF, "foo"); | ||
| headers.insert(VSS_E2EID, "bar"); | ||
| let headers = headers_for_mock.clone(); | ||
| let body = body_for_mock.clone(); | ||
|
|
||
| async move { | ||
| Ok(AsyncRawResponse::from_bytes( | ||
| StatusCode::Forbidden, | ||
| headers, | ||
| Vec::new(), | ||
| )) | ||
| } | ||
| .boxed() | ||
| async move { Ok(AsyncRawResponse::from_bytes(expected_status, headers, body)) }.boxed() | ||
| }); | ||
| let options = AzurePipelinesCredentialOptions { | ||
| credential_options: ClientAssertionCredentialOptions { | ||
|
|
@@ -285,25 +290,35 @@ mod tests { | |
| &[(OIDC_VARIABLE_NAME, "http://localhost/get_token")][..], | ||
| )), | ||
| }; | ||
| let credential = | ||
| AzurePipelinesCredential::new("a".into(), "b".into(), "c", "d", Some(options)) | ||
| .expect("valid AzurePipelinesCredential"); | ||
| let err = credential | ||
| let err = AzurePipelinesCredential::new("a".into(), "b".into(), "c", "d", Some(options)) | ||
| .expect("credential") | ||
| .get_token(&["default"], None) | ||
| .await | ||
| .expect_err("expected error"); | ||
| assert!(matches!( | ||
| err.kind(), | ||
| ErrorKind::HttpResponse { status, .. } | ||
| if *status == StatusCode::Forbidden && | ||
| err.to_string().contains("foo") && | ||
| err.to_string().contains("bar"), | ||
| )); | ||
| assert!( | ||
| err.to_string() | ||
| .contains(&format!("{TSG_LINK_ERROR_TEXT}#apc")), | ||
| "expected error to contain a link to the troubleshooting guide, got '{err}'", | ||
|
|
||
| assert!(matches!(err.kind(), ErrorKind::Credential)); | ||
| assert_eq!( | ||
| r#"AzurePipelinesCredential authentication failed. 403 response from the OIDC endpoint. Check service connection ID and pipeline configuration. Headers { x-msedge-ref: "foo", x-vss-e2eid: "bar" } | ||
| To troubleshoot, visit https://aka.ms/azsdk/rust/identity/troubleshoot#apc"#, | ||
| err.to_string(), | ||
| ); | ||
| match err | ||
| .downcast_ref::<azure_core::Error>() | ||
| .expect("returned error should wrap an azure_core::Error") | ||
| .kind() | ||
| { | ||
| ErrorKind::HttpResponse { | ||
| error_code: Some(reason), | ||
| raw_response: Some(response), | ||
| status, | ||
| .. | ||
| } => { | ||
| assert_eq!(status.canonical_reason(), reason.as_str()); | ||
| assert_eq!(&expected_response, response.as_ref()); | ||
| assert_eq!(expected_status, *status); | ||
| } | ||
| err => panic!("unexpected {:?}", err), | ||
| }; | ||
| } | ||
|
|
||
| #[tokio::test] | ||
|
|
||
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.
"motivated". Perhaps "caused"? Even if that's an arcane but valid use - which I normally appreciate - I imagine users might be scratching their heads as well.