You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "erickguan (via GitHub)" <gi...@apache.org> on 2025/03/10 20:39:32 UTC
[PR] feat(services/onedrive): add signer to utilize the refresh token [opendal]
erickguan opened a new pull request, #5733:
URL: https://github.com/apache/opendal/pull/5733
# Which issue does this PR close?
Closes #5653.
Part of #5677.
Part of #5702.
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
This is a somewhat large PR. Reviewing it commit by commit is easier.
1. This PR extracts the OneDrive service into the "core" structure. Minor changes to names, how to stat, etc.
2. Uses context for an HTTP client.
3. Adds a signer. This mostly follows the Google Drive service signer implementation.
# Are there any user-facing changes?
I added a few more refresh token settings for the OneDrive service. I added the documentation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/onedrive): add signer to utilize the refresh token [opendal]
Posted by "emliunix (via GitHub)" <gi...@apache.org>.
emliunix commented on PR #5733:
URL: https://github.com/apache/opendal/pull/5733#issuecomment-2724368125
Thanks for the ehancements, especially the addition of signer so that I can delete my custom token refresh logic.
The code looks expecting one of 1. access_token or 2. refresh_token + client_id + optional client_secret to work.
I tested the **refresh_token + client_id** case and found that I have to add scope **"offline_access"** to get the next refresh_token, otherwise it will be a json deserialize error complaining misssing refresh_token.
```
2025-03-14T11:08:06.537397Z ERROR request{method=PROPFIND uri=/zotero/ version=HTTP/1.1}: opendal::services: service=onedrive name= path=/ listed=0: List::next failed Unexpected (permanent) at List::next => deserialize json
Context:
service: onedrive
path: /
listed: 0
Source:
missing field `refresh_token` at line 1 column 1627
```
Tried following 2 workarounds but all failed:
1. edit my my application (that of the client_id) to include offline_access
<img width="830" alt="image" src="https://github.com/user-attachments/assets/99d117f6-dcea-472b-8ef5-e5e72ee739ac" />
2. requesting offline_access on initial oauth workflow when obtaining refresh_token (+ access_token).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/onedrive): add signer to utilize the refresh token [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5733:
URL: https://github.com/apache/opendal/pull/5733#discussion_r1988297708
##########
core/src/services/onedrive/backend.rs:
##########
@@ -16,111 +16,54 @@
// under the License.
use std::fmt::Debug;
+use std::fmt::Formatter;
use std::sync::Arc;
-use bytes::Buf;
-use bytes::Bytes;
-use http::header;
-use http::Request;
use http::Response;
use http::StatusCode;
-use super::delete::OnedriveDeleter;
+use super::core::OneDriveCore;
+use super::delete::OneDriveDeleter;
use super::error::parse_error;
-use super::graph_model::CreateDirPayload;
-use super::graph_model::ItemType;
-use super::graph_model::OneDriveUploadSessionCreationRequestBody;
-use super::graph_model::OnedriveGetItemBody;
-use super::lister::OnedriveLister;
+use super::lister::OneDriveLister;
use super::writer::OneDriveWriter;
use crate::raw::*;
use crate::*;
#[derive(Clone)]
-pub struct OnedriveBackend {
- info: Arc<AccessorInfo>,
- root: String,
- access_token: String,
- client: HttpClient,
+pub struct OneDriveBackend {
Review Comment:
The naming here needs to align with the scheme. Since our scheme is `onedrive` instead of `one_drive`, we must use `Onedrive` instead of `OneDrivew`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/onedrive): add signer to utilize the refresh token [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #5733:
URL: https://github.com/apache/opendal/pull/5733#discussion_r1988919908
##########
core/src/services/onedrive/backend.rs:
##########
@@ -16,111 +16,54 @@
// under the License.
use std::fmt::Debug;
+use std::fmt::Formatter;
use std::sync::Arc;
-use bytes::Buf;
-use bytes::Bytes;
-use http::header;
-use http::Request;
use http::Response;
use http::StatusCode;
-use super::delete::OnedriveDeleter;
+use super::core::OneDriveCore;
+use super::delete::OneDriveDeleter;
use super::error::parse_error;
-use super::graph_model::CreateDirPayload;
-use super::graph_model::ItemType;
-use super::graph_model::OneDriveUploadSessionCreationRequestBody;
-use super::graph_model::OnedriveGetItemBody;
-use super::lister::OnedriveLister;
+use super::lister::OneDriveLister;
use super::writer::OneDriveWriter;
use crate::raw::*;
use crate::*;
#[derive(Clone)]
-pub struct OnedriveBackend {
- info: Arc<AccessorInfo>,
- root: String,
- access_token: String,
- client: HttpClient,
+pub struct OneDriveBackend {
Review Comment:
Ooops, good catch. I didn't mean to change this struct name. Will push a fix.
I kept a few modules that are not "public interfaces" with `OneDriveXXX` because OneDrive is the product name. If you want to keep the same convention following the schema name, please ping me!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/onedrive): add signer to utilize the refresh token [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #5733:
URL: https://github.com/apache/opendal/pull/5733#discussion_r1988919908
##########
core/src/services/onedrive/backend.rs:
##########
@@ -16,111 +16,54 @@
// under the License.
use std::fmt::Debug;
+use std::fmt::Formatter;
use std::sync::Arc;
-use bytes::Buf;
-use bytes::Bytes;
-use http::header;
-use http::Request;
use http::Response;
use http::StatusCode;
-use super::delete::OnedriveDeleter;
+use super::core::OneDriveCore;
+use super::delete::OneDriveDeleter;
use super::error::parse_error;
-use super::graph_model::CreateDirPayload;
-use super::graph_model::ItemType;
-use super::graph_model::OneDriveUploadSessionCreationRequestBody;
-use super::graph_model::OnedriveGetItemBody;
-use super::lister::OnedriveLister;
+use super::lister::OneDriveLister;
use super::writer::OneDriveWriter;
use crate::raw::*;
use crate::*;
#[derive(Clone)]
-pub struct OnedriveBackend {
- info: Arc<AccessorInfo>,
- root: String,
- access_token: String,
- client: HttpClient,
+pub struct OneDriveBackend {
Review Comment:
Ooops, good catch. I didn't mean to change this struct name. Will push a fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/onedrive): add signer to utilize the refresh token [opendal]
Posted by "emliunix (via GitHub)" <gi...@apache.org>.
emliunix commented on PR #5733:
URL: https://github.com/apache/opendal/pull/5733#issuecomment-2729613015
@erickguan I tested client_secret and in short, it works fine.
Also I'd like to record my findings:
## How I obtain the refresh token
1. create Application: Microsoft Entra Admin Center -> Applications -> App Registrations
2. in the application -> Authentication, add platform config of your choice
3. in the application -> manifest, make sure `"signInAudience": "AzureADandPersonalMicrosoftAccount",` this makes the client_id work with API calls with `/common` path segment
4. follow the code grant flow or other flows (the minimum scopes are `offline_access Files.ReadWrite`). make sure the access token represents a user, because it's accessing the user's onedrive by requesting with "/me/drive" in URL path
## client_secret consideration
Microsoft [classifies](https://learn.microsoft.com/en-us/entra/identity-platform/msal-client-applications) application into either Public Client or Confidential Client. And that's determined by the platform you choose in step 2 above
And what it means is that:
* for Public Client, auth/token_refresh with client_secret will be rejected.
* for Confidential Client, auth/token_refresh without client_secret will be rejected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/onedrive): add signer to utilize the refresh token [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on PR #5733:
URL: https://github.com/apache/opendal/pull/5733#issuecomment-2711788804
<details>
<summary>Behavior tests</summary>
```
OPENDAL_TEST=onedrive cargo test behavior --features tests,services-onedrive -- --show-output
Compiling opendal v0.52.0 (/home/erickg/Dev/opendal/core)
Finished `test` profile [unoptimized + debuginfo] target(s) in 10.76s
Running unittests src/lib.rs (target/debug/deps/opendal-6a1470b1a6444062)
running 0 tests successes:
successes:
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 136 filtered out; finished in 0.00s
Running tests/behavior/main.rs (target/debug/deps/behavior-b979b2a41b98fab9)
running 104 tests
test behavior::test_delete_with_version ... ok
test behavior::test_delete_with_not_existing_version ... ok
test behavior::test_batch_delete ... ok
test behavior::test_batch_delete_with_version ... ok
test behavior::test_list_with_start_after ... ok
test behavior::test_list_files_with_versions ... ok
test behavior::test_list_with_versions_and_limit ... ok
test behavior::test_list_with_versions_and_start_after ... ok
test behavior::test_list_files_with_deleted ... ok
test behavior::test_reader_with_if_none_match ... ok
test behavior::test_reader_with_if_match ... ok
test behavior::test_read_with_if_unmodified_since ... ok
test behavior::test_read_with_if_modified_since ... ok
test behavior::test_reader_with_if_modified_since ... ok
test behavior::test_reader_with_if_unmodified_since ... ok
test behavior::test_read_with_override_cache_control ... ok
test behavior::test_read_with_override_content_type ... ok
test behavior::test_read_with_override_content_disposition ... ok
test behavior::test_read_with_not_existing_version ... ok
test behavior::test_read_with_if_none_match ... ok
test behavior::test_read_with_version ... ok
test behavior::test_read_with_if_match ... ok
test behavior::test_delete_not_existing ... ok
test behavior::test_read_not_exist ... ok
test behavior::test_create_dir ... ok
test behavior::test_stat_with_if_match ... ok
test behavior::test_stat_with_if_none_match ... ok
test behavior::test_stat_with_if_modified_since ... ok
test behavior::test_stat_with_if_unmodified_since ... ok
test behavior::test_stat_with_override_cache_control ... ok
test behavior::test_stat_with_override_content_disposition ... ok
test behavior::test_stat_with_override_content_type ... ok
test behavior::test_stat_root ... ok
test behavior::test_stat_with_version ... ok
test behavior::stat_with_not_existing_version ... ok
test behavior::test_list_non_exist_dir ... ok
test behavior::test_write_with_empty_content ... ok
test behavior::test_write_with_dir_path ... ok
test behavior::test_delete_empty_dir ... ok
test behavior::test_write_with_cache_control ... ok
test behavior::test_write_with_content_type ... ok
test behavior::test_write_with_content_disposition ... ok
test behavior::test_write_with_content_encoding ... ok
test behavior::test_write_with_if_none_match ... ok
test behavior::test_write_with_if_not_exists ... ok
test behavior::test_write_with_if_match ... ok
test behavior::test_write_with_user_metadata ... ok
test behavior::test_check ... ok
test behavior::test_writer_write ... ok
test behavior::test_read_with_dir_path ... ok
test behavior::test_writer_write_with_concurrent ... ok
test behavior::test_writer_sink ... ok
test behavior::test_writer_sink_with_concurrent ... ok
test behavior::test_stat_dir ... ok
test behavior::test_stat_not_exist ... ok
test behavior::test_writer_futures_copy ... ok
test behavior::test_writer_futures_copy_with_concurrent ... ok
test behavior::test_writer_return_metadata ... ok
test behavior::test_writer_abort ... ok
test behavior::test_read_range ... ok
test behavior::test_writer_abort_with_concurrent ... ok
test behavior::test_stat_with_special_chars ... ok
test behavior::test_stat_not_cleaned_path ... ok
test behavior::test_write_returns_metadata ... ok
test behavior::test_create_dir_existing ... ok
test behavior::test_blocking_read_not_exist ... ok
test behavior::test_read_with_special_chars ... ok
test behavior::test_blocking_create_dir ... ok
test behavior::test_delete_with_special_chars ... ok
test behavior::test_stat_nested_parent_dir ... ok
test behavior::test_blocking_write_with_dir_path ... ok
test behavior::test_stat_file ... ok
test behavior::test_write_with_special_chars ... ok
test behavior::test_delete_file ... ok
test behavior::test_write_only ... ok
test behavior::test_list_file_with_recursive ... ok
test behavior::test_blocking_create_dir_existing ... ok
test behavior::test_blocking_stat_not_exist ... ok
test behavior::test_read_full ... ok
test behavior::test_blocking_stat_dir ... ok
test behavior::test_blocking_delete_file ... ok
test behavior::test_list_dir_with_file_path ... ok
test behavior::test_list_dir ... ok
test behavior::test_blocking_stat_file ... ok
test behavior::test_blocking_read_range ... ok
test behavior::test_blocking_write_file ... ok
test behavior::test_list_prefix ... ok
test behavior::test_blocking_write_returns_metadata ... ok
test behavior::test_blocking_remove_one_file ... ok
test behavior::test_blocking_stat_with_special_chars ... ok
test behavior::test_blocking_read_full ... ok
test behavior::test_list_sub_dir ... ok
test behavior::test_remove_one_file ... ok
test behavior::test_reader ... ok
test behavior::test_blocking_write_with_special_chars ... ok
test behavior::test_writer_write_with_overwrite ... ok
test behavior::test_list_nested_dir ... ok
test behavior::test_list_dir_with_recursive ... ok
test behavior::test_list_dir_with_recursive_no_trailing_slash ... ok
test behavior::test_list_root_with_recursive ... ok
test behavior::test_remove_all ... ok
test behavior::test_list_rich_dir ... ok
test behavior::test_list_empty_dir ... ok
test behavior::test_delete_stream ... ok
test result: ok. 104 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 128.37s
```
</details>
I know the OneDrive behavior test is flaky. Fixing tests will take a few PRs and love. But it's also a good time to add new features:
- version
- caching if the work on the other side goes well
- etc
cc @emliunix Feel free to try this branch if you find time!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/onedrive): add signer to utilize the refresh token [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on PR #5733:
URL: https://github.com/apache/opendal/pull/5733#issuecomment-2727007844
Looks good! Thanks @emliunix!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/onedrive): add signer to utilize the refresh token [opendal]
Posted by "emliunix (via GitHub)" <gi...@apache.org>.
emliunix commented on PR #5733:
URL: https://github.com/apache/opendal/pull/5733#issuecomment-2730001433
@Xuanwo , sure, created PR #5792 to close #4715 I think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/onedrive): add signer to utilize the refresh token [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #5733:
URL: https://github.com/apache/opendal/pull/5733
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/onedrive): add signer to utilize the refresh token [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on PR #5733:
URL: https://github.com/apache/opendal/pull/5733#issuecomment-2725487117
@emliunix Thank you so much for troubleshooting and bug report. I created #5775.
It seems you also have a fully functional OpenDAL Rust. Can you help test the client_id and client_secret once I fix the "offline_access" scope? I appreciate your help.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/onedrive): add signer to utilize the refresh token [opendal]
Posted by "emliunix (via GitHub)" <gi...@apache.org>.
emliunix commented on PR #5733:
URL: https://github.com/apache/opendal/pull/5733#issuecomment-2726927492
@erickguan sure, look it's already fixed, I'll take a look once I got time.
For the case of client_id and client_secret, per my test, the auth flow only requires client_id. So my understanding is to test if Graph API will reject on the presence of wrong client_secret.
Because the code already requires refresh_token (implies the auth has completed beforehand), hence the secret is not used for auth flow here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/onedrive): add signer to utilize the refresh token [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #5733:
URL: https://github.com/apache/opendal/pull/5733#issuecomment-2729655090
Thanks a lot for this great notes! @emliunix
Would you like to start a PR to enrich them in our onedrives' guide?
See https://github.com/apache/opendal/blob/main/core/src/services/onedrive/docs.md
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org