Add an option to download flagged files for business users - #3823
ArcticLampyrid wants to merge 8 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
fce8665 to
60e39e5
Compare
|
Please can you resolve the conflicts for this PR |
60e39e5 to
0709077
Compare
|
Thanks for working on this feature. The overall approach is sound: the existing behaviour remains opt-in, malware detection is based on the There are, however, a few changes I would like made before this can be merged. 1. Preserve the existing normal download behaviourThe current /content?AVOverride=1This has existed in the client for many years and was originally introduced specifically to deal with antivirus-related download behaviour. This PR currently changes the ordinary download URL to: /contentThat is outside the scope of this feature request and changes behaviour for every normal file download. Please preserve the existing URL for normal downloads: const(char)[] url = driveByIdUrl ~ driveId ~ "/items/" ~ itemId ~ "/content?AVOverride=1";and only replace it with the beta endpoint when all of the following are true:
The desired behaviour should therefore remain: There should be no behavioural change for existing users who do not enable this new option. 2. Handle Personal accounts explicitlyThe PR documentation correctly states that this mechanism is not applicable to OneDrive Personal accounts. The implementation should reflect that rather than attempting a beta Please add an explicit account-type check so that when: and a malware-flagged download is requested with 3. Validate this using the authentication model supported by this clientThe rclone implementation referenced by the issue demonstrated successful malware downloads primarily using application permissions / This client currently uses delegated user OAuth authentication. Before this is merged, please provide real-world validation using this PR against an actual OneDrive Business tenant using the authentication flow supported by this client. At minimum I would like validation of:
Please include debug evidence showing the relevant Graph request and resulting behaviour. 4. Strengthen the documentation around the beta APIPlease make it explicit in the configuration documentation that this functionality depends on a Microsoft Graph beta endpoint. Microsoft states that beta APIs are subject to change and are not supported for production use in the same way as the stable This is acceptable here because the feature is explicitly opt-in, but users enabling it should understand that limitation. 5. TestingThe metadata propagation changes for Business Shared Files look appropriate, particularly because Graph Search responses may omit facets which are present on the authoritative DriveItem. However, this feature currently has no focused regression coverage. Please add applicable focused test coverage where practical, particularly around the decision logic: The objective here is to ensure this feature remains completely isolated from the normal download path. I do not think the overall implementation needs redesigning. The main requirement is to keep this change tightly scoped: preserve the existing normal download behaviour, explicitly handle unsupported Personal accounts, and provide evidence that |
Partial close #3821
Sadly
Prefer: forceInfectedDownloadhas no effect on personal accounts. Only enterprise environment can use...