Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: "Headless" mode of the AliyunDrive implementation #4733

Closed
imWildCat opened this issue Jun 13, 2024 · 6 comments · Fixed by #4740
Closed

Proposal: "Headless" mode of the AliyunDrive implementation #4733

imWildCat opened this issue Jun 13, 2024 · 6 comments · Fixed by #4740

Comments

@imWildCat
Copy link
Contributor

While the implementation of Aliyun Drive works end to end. It is different from client-only / non-server workflows, where there's no `refresh_token' provided.
And client-only solution should NOT contain the "client_secret" for security reasons.

Thus, we might need a "headless" mode where the access token is not relying on the refresh token.

Before the actual code changes, I'd like to propose the potential modifications, i.e. the "headless" mode:

let mut aliyun_drive_builder = opendal::services::AliyunDrive::default();

aliyun_drive_builder.access_token(&aliyun_drive_config.access_token, drive_id);
// or:
aliyun_drive_builder.with_headless_mode(&aliyun_drive_config.access_token, drive_id);

let operator = opendal::Operator::new(aliyun_drive_builder)?
    .layer(opendal::layers::LoggingLayer::default())
    .finish();

We might want a different name rather than the headless mode.

In pub async fn get_token_and_drive(&self) -> Result<(Option<String>, String)>, we can check if it is the headless mode. If it is, we just use this configuration rather than requesting new access tokens

pub async fn get_token_and_drive(&self) -> Result<(Option<String>, String)> {

Background: #4585 (comment)

Trying to use this in production but this implementation is too different from that of OneDrive and Google Drive.

The major question is that: Why this OpenDAL implement requires refresh_token and client_id?

The AliyunDrive API supports OAuth. IMHO, we can just send an access_token to the operator and let the host app handle the auth part.

Not all users would have the refresh_token. By design, AliyunDrive does not generate refresh_token for non-server applications https://www.yuque.com/aliyundrive/zpfszx/eam8ls1lmawwwksv

Sample token redeem response:

{
  "token_type": "Bearer",
  "access_token": "ey...",
  "refresh_token": null,
  "expires_in": 2592000
}
@Xuanwo
Copy link
Member

Xuanwo commented Jun 14, 2024

Seems we can add access_token in config

@yuchanns
Copy link
Member

yuchanns commented Jun 14, 2024

I agree with you. I overlooked the client side and prefer to delegate the token generation part to the lib. However, it's crucial to keep the secret confidential for client apps.

IMO, all we need is an extra access_token. Users can configure it with either an access_token or a set of refresh_token, client_id, and client_secret. headless is abrupt, just throw it away : )

@imWildCat
Copy link
Contributor Author

thanks for your inputs!

Do you prefer me or yourself to do this? @yuchanns

@yuchanns
Copy link
Member

yuchanns commented Jun 14, 2024

I plan to submit a PR tomorrow UTC+8. I'm considering refactoring the Writer part of AliyunDrive to fully support the rapid upload, and then I will hitch a ride to add it.

@imWildCat
Copy link
Contributor Author

Awesome! Thank you so much!

@Xuanwo
Copy link
Member

Xuanwo commented Jun 14, 2024

I plan to submit a PR tomorrow UTC+8. I'm considering refactoring the Writer part of AliyunDrive to fully support the rapid upload, and then I will hitch a ride to add it.

Bravo! Our integration tests failed for #4657, maybe you also want to take a look 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants