[DAPS-1578] Device Auth Flow #1681
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…scanning 1216 daps feature add improved ci scanning
Update nlhoman json version
* Add additional params to DatabaseAPI::userSetAccessToken to pass message with addtional info on to database API. * Add convenience method with old type signature of DatabaseAPI::userSetAccessToken for compatibility * Conditionally add other token options param when calling dbGetRaw. * Pass token type along to Database API. * Pass token type from message. * Extract logic for building URL from dbPost and dbGetRaw into separate function for easier testing and debugging * Extract url building logic from dbGet * Add some comments for moved code, address a TODO, add new TODO for AccessToken Message type field * Address default on token type, formatting * Add new AccessTokenType entries for a sentinel value and Globus default in SDMS.proto, use as desired in DatabaseAPI.cpp * Change query param to match backend changes * Restore formatting of SDMS.proto * Make built URL const where appropriate in DatabaseAPI.cpp and DatabaseAPI.hpp * Change default AccessTokenType for UserSetAccessTokenRequest in SDMS_Auth.proto. * Remove logging of potentially senitive data * Adjust AccessTokenType ACCESS_SENTINEL to have value of uint8 max. --------- Co-authored-by: Anthony Ramirez <[email protected]>
[DLT-1110] Add controller unit tests [DLT-1110] Remove debug [DLT-1110] Correct import statements, endpoint list [DLT-1110] Refactor out into MVC [DLT-1110] Add debug [DLT-1110] Functioning modal, needs refactoring [DLT-1110] Refactor dlg start transfer using OOO programming. Should be MVC cased on what I'm seeing but we'll see
…ransfer request when update or get
…ollection-endpoint-browse Revert "[DLT-1110] Mapped Collection Endpoint Browse (1/4)"
1209 feature add jsdoc linter
Co-authored-by: Anthony Ramirez <[email protected]>
…s to lib/permissions (#1695)
…tadata and globus (#1697) Co-authored-by: Aaron Perez <[email protected]>
…m, add RepoAllocationCreateResponse (#1719)
…ional fields when creating repo to support metadat… (#1714)
…ptional in repoCreateRequest (#1716)
…outer create (#1706) Co-authored-by: Aaron Perez <[email protected]>
…3 globus_sdk version pinned (#1689)
…uilding the container image from incorrect sha (#1732)
Co-authored-by: Joshua S Brown <[email protected]> Co-authored-by: JoshuaSBrown <[email protected]>
Co-authored-by: Joshua S Brown <[email protected]> Co-authored-by: JoshuaSBrown <[email protected]>
Co-authored-by: Joshua S Brown <[email protected]> Co-authored-by: JoshuaSBrown <[email protected]>
…bus credentials for web service (#1731) Co-authored-by: Joshua S Brown <[email protected]> Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Contributor
Reviewer's GuideThis PR adds end-to-end OAuth2/OIDC device authorization support to the Python DataFed client and CLI, extends Docker Compose for an identity provider, and scaffolds a new Rust-based core API service with OIDC discovery, routing, error handling, and optional metrics. Sequence diagram for device authorization flow between CLI and core APIsequenceDiagram
actor User
participant CLI
participant "Core API"
User->>CLI: Start login with --device-auth
CLI->>"Core API": POST /auth/device (request device code)
"Core API"-->>CLI: verification_uri, code
CLI->>User: Display verification URI and code
User->>VerificationURI: Visit URI and enter code
loop Polling
CLI->>"Core API": POST /auth/device/poll (with code)
alt Not yet authorized
"Core API"-->>CLI: 400/401/429 (wait and retry)
else Authorized
"Core API"-->>CLI: access_token, refresh_token
end
end
CLI->>CLI: Store tokens, log in
CLI->>User: Show success
Entity relationship diagram for new and updated config optionserDiagram
CONFIG {
string client_token
string client_refresh_token
string device_auth_scope
string core_api_url
int core_api_port
bool allow_self_signed_certs
}
CONFIG ||--o| CLIENT : has
CONFIG ||--o| SERVER : has
CLIENT {
string client_token
string client_refresh_token
string device_auth_scope
bool allow_self_signed_certs
}
SERVER {
string core_api_url
int core_api_port
}
Class diagram for new Rust OIDC and API state typesclassDiagram
class OIDCConfig {
+discovery_url: String
+client_id: String
}
class OIDC {
+device_authorization_endpoint: String
+token_endpoint: String
+client_id: String
+issuer: String
+jwks_uri: String
+jwks_cache: Arc<RwLock<Option<JwkSet>>>
+new(oidc_config: OIDCConfig): OIDC
+cached_jwks(): Option<JwkSet>
+store_jwks(jwks: JwkSet)
+clear_jwks()
}
class ApiState {
+oidc: OIDC
}
OIDCConfig <|-- OIDC
ApiState o-- OIDC
Class diagram for new Rust AppConfig and related config typesclassDiagram
class AppConfig {
+rust_log: String
+api: ApiConfig
+loki: Option<LokiConfig>
+metrics: MetricsConfig
+oidc: OIDCConfig
}
class ApiConfig {
+url: String
+port: u16
}
class LokiConfig {
+url: String
+service_name: String
}
class MetricsConfig {
+url: String
+port: u16
}
class OIDCConfig {
+discovery_url: String
+client_id: String
}
AppConfig o-- ApiConfig
AppConfig o-- LokiConfig
AppConfig o-- MetricsConfig
AppConfig o-- OIDCConfig
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- This PR mixes substantial Python client changes with a new Rust core-API service—consider splitting it into separate PRs to keep reviews focused and maintain clear boundaries between components.
- The new device flow logic in CommandLib has grown large—extracting it into its own module or class would improve readability and keep the core command library more maintainable.
- There’s repeated HTTP request and error-parsing code (_core_api_request, _format_api_error, _parse_json_response) across the client methods—consolidate these into a shared helper to reduce duplication.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- This PR mixes substantial Python client changes with a new Rust core-API service—consider splitting it into separate PRs to keep reviews focused and maintain clear boundaries between components.
- The new device flow logic in CommandLib has grown large—extracting it into its own module or class would improve readability and keep the core command library more maintainable.
- There’s repeated HTTP request and error-parsing code (_core_api_request, _format_api_error, _parse_json_response) across the client methods—consolidate these into a shared helper to reduce duplication.
## Individual Comments
### Comment 1
<location> `crates/core-api/src/state.rs:20-29` </location>
<code_context>
+ pub async fn new(oidc_config: OIDCConfig) -> Result<Self, reqwest::Error> {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider more robust error handling for missing OIDC config fields.
Defaulting to empty strings for missing required fields may cause issues downstream. Instead, return an error or log a warning when required fields are absent.
Suggested implementation:
```rust
let device_authorization_endpoint = config
.get("device_authorization_endpoint")
.and_then(Value::as_str)
.ok_or_else(|| {
reqwest::Error::new(
reqwest::StatusCode::BAD_REQUEST,
"Missing required field: device_authorization_endpoint",
)
})?
.to_string();
```
```rust
let token_endpoint = config
.get("token_endpoint")
.and_then(Value::as_str)
.ok_or_else(|| {
reqwest::Error::new(
reqwest::StatusCode::BAD_REQUEST,
"Missing required field: token_endpoint",
)
})?
.to_string();
let client_id = config
.get("client_id")
.and_then(Value::as_str)
.ok_or_else(|| {
reqwest::Error::new(
reqwest::StatusCode::BAD_REQUEST,
"Missing required field: client_id",
)
})?
.to_string();
let issuer = config
.get("issuer")
.and_then(Value::as_str)
.ok_or_else(|| {
reqwest::Error::new(
reqwest::StatusCode::BAD_REQUEST,
"Missing required field: issuer",
)
})?
.to_string();
let jwks_uri = config
.get("jwks_uri")
.and_then(Value::as_str)
.ok_or_else(|| {
reqwest::Error::new(
reqwest::StatusCode::BAD_REQUEST,
"Missing required field: jwks_uri",
)
})?
.to_string();
```
- You may want to define a custom error type instead of using `reqwest::Error` for more precise error handling.
- If you prefer logging a warning before returning the error, you can add a log statement in each `ok_or_else` closure.
- Make sure to update the rest of the constructor to use the new variables (`client_id`, `issuer`, `jwks_uri`) as shown.
</issue_to_address>
### Comment 2
<location> `crates/core-api/src/errors.rs:20-33` </location>
<code_context>
+ SetupError,
+}
+
+impl IntoResponse for ApiError {
+ fn into_response(self) -> axum::response::Response {
+ // Mappings from Rust errors to standard HTTP status codes
+ match self {
+ Self::NotFound => StatusCode::NOT_FOUND,
+ Self::InternalServerError => StatusCode::INTERNAL_SERVER_ERROR,
+ Self::BadRequest => StatusCode::BAD_REQUEST,
+ Self::Unauthorized => StatusCode::UNAUTHORIZED,
+ Self::TooManyRequests => StatusCode::TOO_MANY_REQUESTS,
+ Self::SetupError => StatusCode::INTERNAL_SERVER_ERROR,
+ }
+ .into_response()
+ }
+}
</code_context>
<issue_to_address>
**suggestion:** Consider including error details in the response body for better client feedback.
Returning only a status code limits client error handling. Adding a JSON error message would make debugging easier for clients.
```suggestion
use axum::{
response::{IntoResponse, Response},
http::StatusCode,
Json,
};
use serde_json::json;
impl IntoResponse for ApiError {
fn into_response(self) -> Response {
let status = match self {
Self::NotFound => StatusCode::NOT_FOUND,
Self::InternalServerError => StatusCode::INTERNAL_SERVER_ERROR,
Self::BadRequest => StatusCode::BAD_REQUEST,
Self::Unauthorized => StatusCode::UNAUTHORIZED,
Self::TooManyRequests => StatusCode::TOO_MANY_REQUESTS,
Self::SetupError => StatusCode::INTERNAL_SERVER_ERROR,
};
let body = json!({
"error": format!("{:?}", self),
"message": self.to_string(),
});
(status, Json(body)).into_response()
}
}
```
</issue_to_address>
### Comment 3
<location> `crates/core-api/src/middleware.rs:8-12` </location>
<code_context>
+
+// Check for a correlation ID header, and if there is none, create one
+pub fn create_correlation_id<T>(req: &Request<T>) -> Uuid {
+ match req.headers().get("x-correlation-id") {
+ Some(header_value) => {
+ Uuid::from_str(header_value.to_str().unwrap_or_default()).unwrap_or_default()
+ }
+ None => Uuid::new_v4(),
+ }
+}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider using a more robust header parsing for correlation IDs.
Logging a warning or error when an invalid UUID is encountered will help identify issues with incoming requests and prevent silent failures.
</issue_to_address>
### Comment 4
<location> `src/core.rs:8-13` </location>
<code_context>
+
+#[tokio::main]
+async fn main() -> anyhow::Result<()> {
+ let app_config: AppConfig = toml::from_str(
+ tokio::fs::read_to_string("AppSettings.toml")
+ .await
+ .expect("could not read environment file")
</code_context>
<issue_to_address>
**suggestion:** Consider handling missing or malformed config files more gracefully.
The code panics if the config file is missing or invalid. Please add error handling to provide a clear message or fallback behavior in these cases.
```suggestion
let config_content = match tokio::fs::read_to_string("AppSettings.toml").await {
Ok(content) => content,
Err(e) => {
eprintln!("Error reading AppSettings.toml: {}", e);
eprintln!("Please ensure the configuration file exists and is readable.");
std::process::exit(1);
}
};
let app_config: AppConfig = match toml::from_str(config_content.as_str()) {
Ok(cfg) => cfg,
Err(e) => {
eprintln!("Error parsing AppSettings.toml: {}", e);
eprintln!("Please check the configuration file for syntax errors.");
std::process::exit(1);
}
};
```
</issue_to_address>
### Comment 5
<location> `src/core.rs:30` </location>
<code_context>
+ #[cfg(feature = "loki")]
+ let loki_task_controller = if let Some(ref config) = app_config.loki {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider handling errors from tracing_loki::builder more robustly.
If tracing_loki::builder returns an error due to invalid configuration, ensure the error is logged or handled to prevent silent failures.
</issue_to_address>
### Comment 6
<location> `python/datafed_pkg/datafed/CLI.py:3143` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 7
<location> `python/datafed_pkg/datafed/CLI.py:3168` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 8
<location> `python/datafed_pkg/datafed/CLI.py:3174` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 9
<location> `python/datafed_pkg/datafed/CLI.py:3183` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 10
<location> `python/datafed_pkg/datafed/CommandLib.py:191` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 11
<location> `python/datafed_pkg/datafed/CommandLib.py:223-225` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 12
<location> `python/datafed_pkg/datafed/CommandLib.py:235-238` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 13
<location> `python/datafed_pkg/datafed/CommandLib.py:259` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 14
<location> `python/datafed_pkg/datafed/CommandLib.py:267-269` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 15
<location> `python/datafed_pkg/datafed/CommandLib.py:298-300` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 16
<location> `python/datafed_pkg/datafed/CommandLib.py:304-308` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 17
<location> `python/datafed_pkg/datafed/CommandLib.py:315-317` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 18
<location> `python/datafed_pkg/datafed/CommandLib.py:354-356` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 19
<location> `python/datafed_pkg/datafed/CommandLib.py:370-372` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 20
<location> `python/datafed_pkg/datafed/CommandLib.py:401-405` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 21
<location> `python/datafed_pkg/datafed/CommandLib.py:439-441` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 22
<location> `python/datafed_pkg/datafed/CommandLib.py:449-451` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 23
<location> `python/datafed_pkg/datafed/CommandLib.py:453-457` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 24
<location> `python/datafed_pkg/datafed/CommandLib.py:465-467` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 25
<location> `python/datafed_pkg/datafed/CommandLib.py:515-519` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 26
<location> `python/datafed_pkg/datafed/CommandLib.py:217` </location>
<code_context>
def _get_core_api_base_url(self):
"""
Resolve the base URL for the core REST API.
"""
base_url = self.cfg.get("core_api_url")
if base_url:
base_url = base_url.strip()
if not base_url:
raise Exception(
"Core API base URL is empty. Set 'core_api_url' to a valid HTTP(S) URL."
)
if "://" not in base_url:
base_url = "https://" + base_url
return base_url.rstrip("/")
host = self.cfg.get("server_host")
port = self.cfg.get("core_api_port")
if host and port:
return "https://{}:{}".format(host, port)
raise Exception(
"Core API base URL is not configured. Set 'core_api_url' or provide both "
"'server_host' and 'core_api_port'."
)
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
- Replace call to format with f-string ([`use-fstring-for-formatting`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-formatting/))
- Use f-string instead of string concatenation ([`use-fstring-for-concatenation`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-concatenation/))
</issue_to_address>
### Comment 27
<location> `python/datafed_pkg/datafed/CommandLib.py:243` </location>
<code_context>
def _format_api_error(self, response):
body = response.text.strip()
if len(body) > 200:
body = body[:200] + "..."
return body or "empty response body"
</code_context>
<issue_to_address>
**suggestion (code-quality):** Use f-string instead of string concatenation ([`use-fstring-for-concatenation`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-concatenation/))
```suggestion
body = f"{body[:200]}..."
```
</issue_to_address>
### Comment 28
<location> `python/datafed_pkg/datafed/CommandLib.py:251-259` </location>
<code_context>
def _core_api_request(self, path, payload, method="POST", timeout=30):
"""
Issue an HTTP request to the DataFed core REST API.
"""
base_url = self._get_core_api_base_url()
url = "{}/{}".format(base_url, path.lstrip("/"))
verify = self._should_verify_tls()
try:
response = requests.request(
method, url, json=payload, timeout=timeout, verify=verify
)
except requests.RequestException as exc:
raise Exception("Device authorization request to {} failed: {}".format(url, exc))
return response
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Replace call to format with f-string [×2] ([`use-fstring-for-formatting`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-formatting/))
- Explicitly raise from a previous error ([`raise-from-previous-error`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/raise-from-previous-error/))
</issue_to_address>
### Comment 29
<location> `python/datafed_pkg/datafed/CommandLib.py:267-269` </location>
<code_context>
def _parse_json_response(self, response, context):
try:
return response.json()
except ValueError as exc:
raise Exception(
"Failed to parse {} response: {}".format(context, exc)
)
</code_context>
<issue_to_address>
**suggestion (code-quality):** We've found these issues:
- Replace call to format with f-string ([`use-fstring-for-formatting`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-formatting/))
- Explicitly raise from a previous error ([`raise-from-previous-error`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/raise-from-previous-error/))
```suggestion
raise Exception(f"Failed to parse {context} response: {exc}") from exc
```
</issue_to_address>
### Comment 30
<location> `python/datafed_pkg/datafed/CommandLib.py:272-273` </location>
<code_context>
def _compute_retry_after(self, response, fallback):
retry_after = response.headers.get("Retry-After")
if retry_after:
try:
wait_time = int(retry_after)
if wait_time > 0:
return wait_time
except ValueError:
pass
return fallback
</code_context>
<issue_to_address>
**suggestion (code-quality):** Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
```suggestion
if retry_after := response.headers.get("Retry-After"):
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+20
to
+29
| pub async fn new(oidc_config: OIDCConfig) -> Result<Self, reqwest::Error> { | ||
| let res = get(oidc_config.discovery_url).await?; | ||
| let config: Value = res.json().await?; | ||
|
|
||
| let device_authorization_endpoint = config | ||
| .get("device_authorization_endpoint") | ||
| .and_then(Value::as_str) | ||
| .unwrap_or_default() | ||
| .to_string(); | ||
|
|
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Consider more robust error handling for missing OIDC config fields.
Defaulting to empty strings for missing required fields may cause issues downstream. Instead, return an error or log a warning when required fields are absent.
Suggested implementation:
let device_authorization_endpoint = config
.get("device_authorization_endpoint")
.and_then(Value::as_str)
.ok_or_else(|| {
reqwest::Error::new(
reqwest::StatusCode::BAD_REQUEST,
"Missing required field: device_authorization_endpoint",
)
})?
.to_string(); let token_endpoint = config
.get("token_endpoint")
.and_then(Value::as_str)
.ok_or_else(|| {
reqwest::Error::new(
reqwest::StatusCode::BAD_REQUEST,
"Missing required field: token_endpoint",
)
})?
.to_string();
let client_id = config
.get("client_id")
.and_then(Value::as_str)
.ok_or_else(|| {
reqwest::Error::new(
reqwest::StatusCode::BAD_REQUEST,
"Missing required field: client_id",
)
})?
.to_string();
let issuer = config
.get("issuer")
.and_then(Value::as_str)
.ok_or_else(|| {
reqwest::Error::new(
reqwest::StatusCode::BAD_REQUEST,
"Missing required field: issuer",
)
})?
.to_string();
let jwks_uri = config
.get("jwks_uri")
.and_then(Value::as_str)
.ok_or_else(|| {
reqwest::Error::new(
reqwest::StatusCode::BAD_REQUEST,
"Missing required field: jwks_uri",
)
})?
.to_string();- You may want to define a custom error type instead of using
reqwest::Errorfor more precise error handling. - If you prefer logging a warning before returning the error, you can add a log statement in each
ok_or_elseclosure. - Make sure to update the rest of the constructor to use the new variables (
client_id,issuer,jwks_uri) as shown.
Comment on lines
+20
to
+33
| impl IntoResponse for ApiError { | ||
| fn into_response(self) -> axum::response::Response { | ||
| // Mappings from Rust errors to standard HTTP status codes | ||
| match self { | ||
| Self::NotFound => StatusCode::NOT_FOUND, | ||
| Self::InternalServerError => StatusCode::INTERNAL_SERVER_ERROR, | ||
| Self::BadRequest => StatusCode::BAD_REQUEST, | ||
| Self::Unauthorized => StatusCode::UNAUTHORIZED, | ||
| Self::TooManyRequests => StatusCode::TOO_MANY_REQUESTS, | ||
| Self::SetupError => StatusCode::INTERNAL_SERVER_ERROR, | ||
| } | ||
| .into_response() | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion: Consider including error details in the response body for better client feedback.
Returning only a status code limits client error handling. Adding a JSON error message would make debugging easier for clients.
Suggested change
| impl IntoResponse for ApiError { | |
| fn into_response(self) -> axum::response::Response { | |
| // Mappings from Rust errors to standard HTTP status codes | |
| match self { | |
| Self::NotFound => StatusCode::NOT_FOUND, | |
| Self::InternalServerError => StatusCode::INTERNAL_SERVER_ERROR, | |
| Self::BadRequest => StatusCode::BAD_REQUEST, | |
| Self::Unauthorized => StatusCode::UNAUTHORIZED, | |
| Self::TooManyRequests => StatusCode::TOO_MANY_REQUESTS, | |
| Self::SetupError => StatusCode::INTERNAL_SERVER_ERROR, | |
| } | |
| .into_response() | |
| } | |
| } | |
| use axum::{ | |
| response::{IntoResponse, Response}, | |
| http::StatusCode, | |
| Json, | |
| }; | |
| use serde_json::json; | |
| impl IntoResponse for ApiError { | |
| fn into_response(self) -> Response { | |
| let status = match self { | |
| Self::NotFound => StatusCode::NOT_FOUND, | |
| Self::InternalServerError => StatusCode::INTERNAL_SERVER_ERROR, | |
| Self::BadRequest => StatusCode::BAD_REQUEST, | |
| Self::Unauthorized => StatusCode::UNAUTHORIZED, | |
| Self::TooManyRequests => StatusCode::TOO_MANY_REQUESTS, | |
| Self::SetupError => StatusCode::INTERNAL_SERVER_ERROR, | |
| }; | |
| let body = json!({ | |
| "error": format!("{:?}", self), | |
| "message": self.to_string(), | |
| }); | |
| (status, Json(body)).into_response() | |
| } | |
| } |
Comment on lines
+8
to
+12
| match req.headers().get("x-correlation-id") { | ||
| Some(header_value) => { | ||
| Uuid::from_str(header_value.to_str().unwrap_or_default()).unwrap_or_default() | ||
| } | ||
| None => Uuid::new_v4(), |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Consider using a more robust header parsing for correlation IDs.
Logging a warning or error when an invalid UUID is encountered will help identify issues with incoming requests and prevent silent failures.
Comment on lines
+8
to
+13
| let app_config: AppConfig = toml::from_str( | ||
| tokio::fs::read_to_string("AppSettings.toml") | ||
| .await | ||
| .expect("could not read environment file") | ||
| .as_str(), | ||
| )?; |
Contributor
There was a problem hiding this comment.
suggestion: Consider handling missing or malformed config files more gracefully.
The code panics if the config file is missing or invalid. Please add error handling to provide a clear message or fallback behavior in these cases.
Suggested change
| let app_config: AppConfig = toml::from_str( | |
| tokio::fs::read_to_string("AppSettings.toml") | |
| .await | |
| .expect("could not read environment file") | |
| .as_str(), | |
| )?; | |
| let config_content = match tokio::fs::read_to_string("AppSettings.toml").await { | |
| Ok(content) => content, | |
| Err(e) => { | |
| eprintln!("Error reading AppSettings.toml: {}", e); | |
| eprintln!("Please ensure the configuration file exists and is readable."); | |
| std::process::exit(1); | |
| } | |
| }; | |
| let app_config: AppConfig = match toml::from_str(config_content.as_str()) { | |
| Ok(cfg) => cfg, | |
| Err(e) => { | |
| eprintln!("Error parsing AppSettings.toml: {}", e); | |
| eprintln!("Please check the configuration file for syntax errors."); | |
| std::process::exit(1); | |
| } | |
| }; |
|
|
||
| tokio::spawn(task); | ||
|
|
||
| tracing::subscriber::set_global_default(subscriber.with(loki_layer))?; |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Consider handling errors from tracing_loki::builder more robustly.
If tracing_loki::builder returns an error due to invalid configuration, ensure the error is logged or handled to prevent silent failures.
Comment on lines
+515
to
+519
| raise Exception( | ||
| "Token validation failed (HTTP {}): {}".format( | ||
| response.status_code, self._format_api_error(response) | ||
| ) | ||
| ) |
Contributor
There was a problem hiding this comment.
issue (code-quality): Raise a specific error instead of the general Exception or BaseException (raise-specific-error)
Explanation
If a piece of code raises a specific exception type rather than the generic [`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException) or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception), the calling code can:- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the built-in exceptions of the standard library.
- Define your own error class that subclasses
Exception.
So instead of having code raising Exception or BaseException like
if incorrect_input(value):
raise Exception("The input is incorrect")you can have code raising a specific error like
if incorrect_input(value):
raise ValueError("The input is incorrect")or
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")| def _format_api_error(self, response): | ||
| body = response.text.strip() | ||
| if len(body) > 200: | ||
| body = body[:200] + "..." |
Contributor
There was a problem hiding this comment.
suggestion (code-quality): Use f-string instead of string concatenation (use-fstring-for-concatenation)
Suggested change
| body = body[:200] + "..." | |
| body = f"{body[:200]}..." |
Comment on lines
+251
to
+259
| url = "{}/{}".format(base_url, path.lstrip("/")) | ||
| verify = self._should_verify_tls() | ||
|
|
||
| try: | ||
| response = requests.request( | ||
| method, url, json=payload, timeout=timeout, verify=verify | ||
| ) | ||
| except requests.RequestException as exc: | ||
| raise Exception("Device authorization request to {} failed: {}".format(url, exc)) |
Contributor
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Replace call to format with f-string [×2] (
use-fstring-for-formatting) - Explicitly raise from a previous error (
raise-from-previous-error)
Comment on lines
+267
to
+269
| raise Exception( | ||
| "Failed to parse {} response: {}".format(context, exc) | ||
| ) |
Contributor
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Replace call to format with f-string (
use-fstring-for-formatting) - Explicitly raise from a previous error (
raise-from-previous-error)
Suggested change
| raise Exception( | |
| "Failed to parse {} response: {}".format(context, exc) | |
| ) | |
| raise Exception(f"Failed to parse {context} response: {exc}") from exc |
Comment on lines
+272
to
+273
| retry_after = response.headers.get("Retry-After") | ||
| if retry_after: |
Contributor
There was a problem hiding this comment.
suggestion (code-quality): Use named expression to simplify assignment and conditional (use-named-expression)
Suggested change
| retry_after = response.headers.get("Retry-After") | |
| if retry_after: | |
| if retry_after := response.headers.get("Retry-After"): |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket
#1578
Description
Generic OAuth2/OIDC support for device auth flow, starting Rust API.
How Has This Been Tested?
N/A
Artifacts (if appropriate):
N/A
Tasks
Summary by Sourcery
Implement generic OAuth2/OIDC device authorization flow support in the Python client and integrate a new Rust-based core REST API with OIDC discovery, device auth endpoints, OpenAPI/Swagger UI, metrics, and graceful shutdown
New Features:
Enhancements:
Build:
Deployment: