feat(services/onedrive): add signer to utilize the refresh token #5733

erickguan · 2025-03-10T20:39:30Z

Which issue does this PR close?

Closes #5653.
Part of #5677.
Part of #5702.

Rationale for this change

What changes are included in this PR?

This is a somewhat large PR. Reviewing it commit by commit is easier.

This PR extracts the OneDrive service into the "core" structure. Minor changes to names, how to stat, etc.
Uses context for an HTTP client.
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.

erickguan · 2025-03-10T20:48:17Z

Behavior tests

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

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!

Xuanwo

The other changes look great—thank you!

Xuanwo · 2025-03-11T02:56:24Z

core/src/services/onedrive/backend.rs

-    root: String,
-    access_token: String,
-    client: HttpClient,
+pub struct OneDriveBackend {


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.

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!

Xuanwo

Thank you, I really love this change!

emliunix · 2025-03-14T11:16:05Z

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" right at OneDriveSigner::refresh_tokens() 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:

edit my my application (that of the client_id) to include offline_access

2. requesting offline_access on initial oauth workflow when obtaining refresh_token (+ access_token).

erickguan · 2025-03-14T18:41:19Z

@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.

emliunix · 2025-03-15T18:42:58Z

@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.

erickguan · 2025-03-15T21:30:11Z

Looks good! Thanks @emliunix!

emliunix · 2025-03-17T13:56:32Z

@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

create Application: Microsoft Entra Admin Center -> Applications -> App Registrations
in the application -> Authentication, add platform config of your choice
in the application -> manifest, make sure "signInAudience": "AzureADandPersonalMicrosoftAccount", this makes the client_id work with API calls with /common path segment
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 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.

Xuanwo · 2025-03-17T14:10:10Z

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

emliunix · 2025-03-17T15:42:24Z

@Xuanwo , sure, created PR #5792 to close #4715 I think.

erickguan added 3 commits March 10, 2025 21:03

chore(services/onedrive): extract OneDrive to core structure

dc2165c

Use http_client from OpenDAL's context

11030c8

feat(services/onedrive): add signer to utilize the refresh token

dd0152d

erickguan requested a review from Xuanwo as a code owner March 10, 2025 20:39

github-actions bot added the releases-note/feat The PR implements a new feature or has a title that begins with "feat" label Mar 10, 2025

fixup! chore(services/onedrive): extract OneDrive to core structure

95a6602

fixup! chore(services/onedrive): extract OneDrive to core structure

06b440d

Xuanwo reviewed Mar 11, 2025

View reviewed changes

Xuanwo mentioned this pull request Mar 11, 2025

docs: Explain the meaning of OneDrive access token #4715

Closed

fixup! chore(services/onedrive): extract OneDrive to core structure

85c6d97

erickguan requested a review from Xuanwo March 11, 2025 11:07

Xuanwo approved these changes Mar 11, 2025

View reviewed changes

Xuanwo merged commit 64a0f44 into apache:main Mar 11, 2025
94 checks passed

erickguan deleted the onedrive-core branch March 11, 2025 12:50

Xuanwo mentioned this pull request Mar 7, 2025

Tracking issue: Refactor services to use the backend.rs and core.rs convention #5702

Open

11 tasks

Xuanwo mentioned this pull request Mar 14, 2025

Migrate all services to context based #5677

Closed

31 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(services/onedrive): add signer to utilize the refresh token #5733

feat(services/onedrive): add signer to utilize the refresh token #5733

Uh oh!

erickguan commented Mar 10, 2025

Uh oh!

erickguan commented Mar 10, 2025

Uh oh!

Xuanwo left a comment

Uh oh!

Xuanwo Mar 11, 2025

Uh oh!

erickguan Mar 11, 2025 •

edited

Loading

Uh oh!

Xuanwo left a comment

Uh oh!

Uh oh!

emliunix commented Mar 14, 2025 •

edited

Loading

Uh oh!

erickguan commented Mar 14, 2025

Uh oh!

emliunix commented Mar 15, 2025

Uh oh!

erickguan commented Mar 15, 2025

Uh oh!

emliunix commented Mar 17, 2025

Uh oh!

Xuanwo commented Mar 17, 2025

Uh oh!

emliunix commented Mar 17, 2025

Uh oh!

Uh oh!

feat(services/onedrive): add signer to utilize the refresh token #5733

feat(services/onedrive): add signer to utilize the refresh token #5733

Uh oh!

Conversation

erickguan commented Mar 10, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

erickguan commented Mar 10, 2025

Uh oh!

Xuanwo left a comment

Choose a reason for hiding this comment

Uh oh!

Xuanwo Mar 11, 2025

Choose a reason for hiding this comment

Uh oh!

erickguan Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xuanwo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

emliunix commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erickguan commented Mar 14, 2025

Uh oh!

emliunix commented Mar 15, 2025

Uh oh!

erickguan commented Mar 15, 2025

Uh oh!

emliunix commented Mar 17, 2025

How I obtain the refresh token

client_secret consideration

Uh oh!

Xuanwo commented Mar 17, 2025

Uh oh!

emliunix commented Mar 17, 2025

Uh oh!

Uh oh!

erickguan Mar 11, 2025 •

edited

Loading

emliunix commented Mar 14, 2025 •

edited

Loading