Conversation
TKussel
left a comment
TKussel
left a comment
There was a problem hiding this comment.
This PR deviates from current practice how we use API keys in HTTP headers. I am concerned of maintaining different semantics across our software packages and deviate from the established way only to simplify one CLI arg. Additionally, I see security issues. But that would be your call.
README.md
Outdated
| PROVIDER = "name" #EUCAIM provider name | ||
| PROVIDER_ICON = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABAQMAAAAl21bKAAAAA1BMVEUAAACnej3aAAAAAXRSTlMAQObYZgAAAApJREFUCNdjYAAAAAIAAeIhvDMAAAAASUVORK5CYII=" # Base64 encoded EUCAIM provider icon | ||
| AUTH_HEADER = "ApiKey XXXX" #Authorization header; if the endpoint type is Blaze or BlazeAndSql, this header is used for the Exporter target application, and the syntax is AUTH_HEADER = "XXXX" where "XXXX" is the API key | ||
| AUTH_HEADER = "ApiKey: XXXX" #Authorization header, in the format "Key: Value" |
There was a problem hiding this comment.
I would advise agains this. The current format AUTHORIZATION: ApiKey XXX follows the RFC 9110 with authorization method ApiKey. Additionally,, this is how Mainzelliste and MainSEL implements it.
If you want to use a non-standard-header, I would advise for x-api-key as recommended by OpenAPI.
src/config.rs
Outdated
|
|
||
| Some((header_name, header_value)) | ||
| } else { | ||
| return Err(FocusError::ConfigurationError(format!("Missing ':' in auth header value \"{}\"", auth_header))); |
There was a problem hiding this comment.
Maybe a description of the error logic is more useful? Something in the line of "Missing header key (the part before the colon)"?
src/config.rs
Outdated
| dbg!(cli_args.endpoint_url.clone()); | ||
| dbg!(cli_args.blaze_url.clone()); | ||
|
|
||
| let auth_header = { |
There was a problem hiding this comment.
This is not an auth header anymore, but an arbitrary header field, right? We should rename it then.
There was a problem hiding this comment.
Additinally, this imposes a security risk to be able to send an arbitrary header, potentially overriding exsisting headers if they are already present.
|
This PR provides an invaluable improvement on Focus' architecture and will be an important part of an re-engineering to support multiple endpoints and stores within the same Bridgehead. However, as it introduces a breaking change for sites that do not run focus inside a Bridgehead, such as EUCAIM providers, we need to coordinate with them regarding the time frame of the merge. In the meantime I have made the change necessary to avoid confusing Exporter API key with authorization header here: #191 |
|
I will convert this PR to a draft until the Focus' re-engineering is better specified |
3 participants
No description provided.