refactor: flexible Authorization header type of HTTP request by rina23q · Pull Request #3190 · thin-edge/thin-edge.io

rina23q · 2024-10-16T17:49:12Z

Proposed changes

Pure refactoring PR to make a progress of #3036 easier.

Now C8YJwtRetriever returns HeaderMap, previously it was just JWT token (String).
Deprecate Auth enum in downloader so that downloader can accept any headers easily
Remove unused functions in 897c102

Why this refactoring is needed?
#3036 needs to introduce Basic authentication instead of JWT token authentication. However, the current implementation, especially HTTP APIs, are designed to use Authorization header together with JWT token. Since the JWT actor is actually a server trait, for basic authentication, we can just implement a server trait.

Question:
The JWT token inplementation in c8y_remote_access_plugin seems duplicated to C8YJwtRetriever implementation...

https://github.com/thin-edge/thin-edge.io/blob/main/plugins/c8y_remote_access_plugin/src/auth.rs
https://github.com/thin-edge/thin-edge.io/blob/main/crates/extensions/c8y_http_proxy/src/credentials.rs
=> I understand they are different level.

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
Documentation Update (if none of the other choices apply)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

#3036

Checklist

I have read the CONTRIBUTING doc
I have signed the CLA (in all commits with git commit -s)
I ran cargo fmt as mentioned in CODING_GUIDELINES
I used cargo clippy as mentioned in CODING_GUIDELINES
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

Further comments

github-actions · 2024-10-16T18:13:39Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
508	0	2	508	100	1h27m40.911290999s

codecov · 2024-10-16T18:46:39Z

Codecov Report

Attention: Patch coverage is 69.37500% with 49 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...rates/extensions/c8y_http_proxy/src/credentials.rs	6.06%	30 Missing and 1 partial ⚠️
crates/extensions/c8y_http_proxy/src/actor.rs	58.82%	3 Missing and 4 partials ⚠️
crates/extensions/c8y_http_proxy/src/tests.rs	89.74%	0 Missing and 4 partials ⚠️
crates/extensions/c8y_auth_proxy/src/tokens.rs	50.00%	0 Missing and 3 partials ⚠️
crates/extensions/c8y_auth_proxy/src/actor.rs	0.00%	2 Missing ⚠️
...rates/extensions/c8y_firmware_manager/src/tests.rs	88.88%	0 Missing and 1 partial ⚠️
...ates/extensions/c8y_firmware_manager/src/worker.rs	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

📢 Thoughts on this report? Let us know!

reubenmiller · 2024-10-17T06:55:38Z

@rina23q @didier-wenzek @albinsuresh Wouldn't it make more sense to have a generic "Add N Headers"...that way we could write providers to inject any required headers and not assume it was just be the "Authorization" header?

didier-wenzek

Question:
The JWT token implementation in c8y_remote_access_plugin seems duplicated to C8YJwtRetriever implementation...

https://github.com/thin-edge/thin-edge.io/blob/main/plugins/c8y_remote_access_plugin/src/auth.rs

https://github.com/thin-edge/thin-edge.io/blob/main/crates/extensions/c8y_http_proxy/src/credentials.rs

This is not a duplicate, just a difference on the API level which is used. The c8y_remote_access_plugin directly invokes C8yMqttJwtTokenRetriever::get_jwt_token() instead as wrapping this call behind an actor as done by C8YJwtRetriever.

=> The implication for that refactoring PR is that the difference between the misc auth method has to be addressed in the c8y_api with a facade over the ``C8yMqttJwtTokenRetrieverand aBasicAuthRetriver`. Both returning an authentication header ready to be used.

crates/common/tedge_config_macros/src/multi.rs

didier-wenzek · 2024-10-17T08:04:23Z

crates/extensions/c8y_http_proxy/src/credentials.rs

@@ -32,16 +32,17 @@ impl C8YJwtRetriever {

 #[async_trait]
 impl Server for C8YJwtRetriever {


Adding the new C8YBasicAuthProvider in this PR would really help to validate the refactoring, even if not use yet.

Sorry, I should have remove this comment which is conflicting with my other comment #3190 (review), telling that:

the difference between the misc auth method has to be addressed in the c8y_api with a facade over the C8yMqttJwtTokenRetriever and a BasicAuthRetriver. Both returning an authentication header ready to be used.

Anyway. This can be done in the next step.

crates/common/download/src/download.rs

rina23q · 2024-10-17T10:25:53Z

@rina23q @didier-wenzek @albinsuresh Wouldn't it make more sense to have a generic "Add N Headers"...that way we could write providers to inject any required headers and not assume it was just be the "Authorization" header?

Ah like returning ({header_key}, {header_value}), e.g. ("Authorization", "Bearer XXX")?

So it means C8YJwtRetriever returns Result<(String, String), HeaderError> instead of Result<String, AuthError>.

Or even getting a help from the http crate's HeaderName and HeaderValue, it can be Result<(HeaderName, HeaderValue), Error).

reubenmiller · 2024-10-17T11:57:47Z

@rina23q @didier-wenzek @albinsuresh Wouldn't it make more sense to have a generic "Add N Headers"...that way we could write providers to inject any required headers and not assume it was just be the "Authorization" header?

Ah like returning ({header_key}, {header_value}), e.g. ("Authorization", "Bearer XXX")?

So it means C8YJwtRetriever returns Result<(String, String), HeaderError> instead of Result<String, AuthError>.

Or even getting a help from the http crate's HeaderName and HeaderValue, it can be Result<(HeaderName, HeaderValue), Error).

Or even better, it results a vector of header name/value 's)...As some more complicated authenticated mechanisms need to set multiple header properties, for example AWS S3 API allows users to also add x-* headers to add some additional options.

rina23q · 2024-10-17T12:06:15Z

Or even better, it results a vector of header name/value 's)...As some more complicated authenticated mechanisms need to set multiple header properties, for example AWS S3 API allows users to also add x-* headers to add some additional options.

Then this HeaderMap is the way to go 👍

rina23q · 2024-10-20T18:52:30Z

Then this HeaderMap is the way to go 👍

This change is done in e7b8724.

Also, I removed unused code in 897c102. If they should stay in our code, please inform me. Then I can revert in that case.

didier-wenzek

Approved. This refactoring PR is a nice progress forward.

Also, I removed unused code in 897c102. If they should stay in our code, please inform me. Then I can revert in that case.

Removing dead code is always a good thing.

crates/common/download/src/download.rs

didier-wenzek · 2024-10-21T08:20:50Z

crates/extensions/c8y_http_proxy/src/credentials.rs

@@ -32,16 +32,17 @@ impl C8YJwtRetriever {

 #[async_trait]
 impl Server for C8YJwtRetriever {


Sorry, I should have remove this comment which is conflicting with my other comment #3190 (review), telling that:

the difference between the misc auth method has to be addressed in the c8y_api with a facade over the C8yMqttJwtTokenRetriever and a BasicAuthRetriver. Both returning an authentication header ready to be used.

Anyway. This can be done in the next step.

- Change JWT token retriever actor to return "Bearer {token}", previously it was just "{token}" - Deprecate Auth enum in downloader so that downloader can accept any authorization header value easily Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

HttpHeaderRetriever returns HeaderMap. So, C8YJwtRetriever now returns HeaderMap. The retrieved JWT token is included in the map. In the future, there will be C8YBasicAuthRetriever, which will also returns HeaderMap. Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

rina23q requested review from albinsuresh, didier-wenzek and jarhodes314 as code owners October 16, 2024 17:49

rina23q temporarily deployed to Test Pull Request October 16, 2024 17:49 — with GitHub Actions Inactive

rina23q temporarily deployed to Test Auto October 16, 2024 17:58 — with GitHub Actions Inactive

rina23q force-pushed the refactor/3036/jwt_retriever_returns_header_value branch from 95d782a to 02a3a44 Compare October 16, 2024 18:32

rina23q temporarily deployed to Test Pull Request October 16, 2024 18:32 — with GitHub Actions Inactive

rina23q temporarily deployed to Test Auto October 16, 2024 18:39 — with GitHub Actions Inactive

didier-wenzek reviewed Oct 17, 2024

View reviewed changes

rina23q had a problem deploying to Test Pull Request October 17, 2024 12:41 — with GitHub Actions Failure

rina23q temporarily deployed to Test Pull Request October 20, 2024 13:29 — with GitHub Actions Inactive

rina23q temporarily deployed to Test Auto October 20, 2024 13:34 — with GitHub Actions Inactive

rina23q requested a review from reubenmiller as a code owner October 20, 2024 17:58

rina23q temporarily deployed to Test Pull Request October 20, 2024 17:58 — with GitHub Actions Inactive

rina23q temporarily deployed to Test Auto October 20, 2024 18:06 — with GitHub Actions Inactive

rina23q requested a review from didier-wenzek October 20, 2024 18:50

didier-wenzek approved these changes Oct 21, 2024

View reviewed changes

rina23q added 3 commits October 21, 2024 12:16

refactor: remove unused C8YRestRequest items and functions

a34f6ad

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

rina23q force-pushed the refactor/3036/jwt_retriever_returns_header_value branch from e7b8724 to 9711f81 Compare October 21, 2024 10:16

rina23q temporarily deployed to Test Pull Request October 21, 2024 10:16 — with GitHub Actions Inactive

rina23q temporarily deployed to Test Auto October 21, 2024 10:30 — with GitHub Actions Inactive

rina23q added this pull request to the merge queue Oct 21, 2024

Merged via the queue into thin-edge:main with commit 30962e6 Oct 21, 2024

rina23q deleted the refactor/3036/jwt_retriever_returns_header_value branch August 8, 2025 11:58

		@@ -32,16 +32,17 @@ impl C8YJwtRetriever {

		#[async_trait]
		impl Server for C8YJwtRetriever {

Conversation

rina23q commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

Uh oh!

github-actions bot commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Robot Results

Uh oh!

codecov bot commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

reubenmiller commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

didier-wenzek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

didier-wenzek Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

didier-wenzek Oct 21, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rina23q commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reubenmiller commented Oct 17, 2024

Uh oh!

rina23q commented Oct 17, 2024

Uh oh!

rina23q commented Oct 20, 2024

Uh oh!

didier-wenzek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

didier-wenzek Oct 21, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rina23q commented Oct 16, 2024 •

edited

Loading

github-actions bot commented Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

reubenmiller commented Oct 17, 2024 •

edited

Loading

rina23q commented Oct 17, 2024 •

edited

Loading