aws-amplify / amplify-swift

A declarative library for application development using cloud services.
Apache License 2.0
456 stars 198 forks source link

Downloading file with data returns a 403. Downloading to a local url works, generating a url works too. #3860

Closed ipodishima closed 2 months ago

ipodishima commented 2 months ago

Describe the bug

Hey

I noticed something really weird which took me a while to figure out (I didn't required to save the data to a file, so I was looking for other issues like misconfiguration).

After uploading a file, if you download it as data, you get a 403.

▿ StorageError: Received HTTP Response status code 403 Forbidden
Recovery suggestion: Make sure the user has access to the key before trying to download/upload it.
  ▿ accessDenied : 3 elements
    - .0 : "Received HTTP Response status code 403 Forbidden"
    - .1 : "Make sure the user has access to the key before trying to download/upload it."
    - .2 : nil

If I download the same file, same key, to a local url or if I generate a presigned url and download it, it works perfectly.

Steps To Reproduce

1.

Define storage like

import { defineStorage } from "@aws-amplify/backend";

export const projectsStorage = defineStorage({
  name: "project-files",
  access: (allow) => ({
    'projectData/*': [
      allow.authenticated.to(['read', 'write', 'delete']),
    ],
  })
});
  1. Upload a file
let dataString = "My Data"
let data = Data(dataString.utf8)
let uploadTask = Amplify.Storage.uploadData(
    path: .fromString("projectData/myFile.data"),
    data: data
)
  1. Download the file
let downloadTask = Amplify.Storage.downloadData(path: .fromString("projectData/myFile.data"))
let data = try await downloadTask.value
print("Completed: \(data)")

You'll get

▿ StorageError: Received HTTP Response status code 403 Forbidden
Recovery suggestion: Make sure the user has access to the key before trying to download/upload it.
  ▿ accessDenied : 3 elements
    - .0 : "Received HTTP Response status code 403 Forbidden"
    - .1 : "Make sure the user has access to the key before trying to download/upload it."
    - .2 : nil
  1. Use file
let downloadToFileName = FileManager.default.urls(
    for: .documentDirectory,
    in: .userDomainMask
)[0].appendingPathComponent("myFile.txt")

let downloadTask = Amplify.Storage.downloadFile(
    path: .fromString("projectData/myFile.data"),
    local: downloadToFileName,
    options: nil
)
try await downloadTask.value
print("Completed")

And it work perfectly.

NB: My workaround for now is

func downloadData(for key: String,
                  downloadProgressHandler: @escaping (Progress) -> Void) async throws -> Data {
    Logger.amplify.debug("Downloading data at \(key)")
    // For some reasons, this return a 403
    // let downloadTask = Amplify.Storage.downloadData(key: key)
    let destinationURL = URL(fileURLWithPath: NSTemporaryDirectory()).appendingPathComponent(UUID().uuidString)
    let downloadTask = Amplify.Storage.downloadFile(path: .fromString(key),
                                                    local: destinationURL)

    for await progress in await downloadTask.progress {
        Logger.amplify.trace("Download progress for \(key): \(progress)")
        downloadProgressHandler(progress)
    }
    try await downloadTask.value
    let data = try Data(contentsOf: destinationURL)
    Logger.amplify.trace("Download completed for \(key)")
    return data
}

### Expected behavior

Downloading data should not return a 403 when the resource is accessible

### Amplify Framework Version

2.39.0

### Amplify Categories

Storage

### Dependency manager

Swift PM

### Swift version

5.10 (swiftlang-5.10.0.13 clang-1500.3.9.4)

### CLI version

npx ampx --version 1.2.5

### Xcode version

Xcode 15.4 Build version 15F31d

### Relevant log output

```shell
<details>
<summary>Log Messages</summary>

INSERT LOG MESSAGES HERE



### Is this a regression?

No

### Regression additional context

I don't know, first time using it.

### Platforms

iOS

### OS Version

17.2

### Device

Simulator

### Specific to simulators

No

### Additional context

_No response_
ruisebas commented 2 months ago

Hi @ipodishima, thanks for opening this issue.

I quickly tried on a simple project and was not able to reproduce this issue following your steps: downloadData(path:) completed successfully.

However, looking at your code snippets I noticed some things that might be causing your issue:

After calling uploadData(path:data:), you are not waiting for the task to complete

let uploadPath = try await uploadTask.value

If you immediately call downloadData(path:) after creating the uploadTask, the actual upload might have not finished yet, so you'd get an error. However this would likely give you a 404 error, so perhaps it's not the actual problem.

In your workaround function, it seems you were previously calling the deprecated downloadData(key:) function instead of downloadData(path:), which is the one you should use since you're using Gen2 and you've also uploaded the data using path instead of key. This also can likely explain the 403 error, as using key will attempt to access the public/ folder by default, for which you did not define permissions in your backend.

Could you please confirm if either of these is the culprit? If the error still persists, then could you please provide the verbose logs when you perform the steps? You can enable it by doing

Amplify.Logging.logLevel = .verbose

Thanks!

ipodishima commented 2 months ago

Heyyyy

Thanks for the really quick response.

Yes, I do wait for the upload to complete, just forgot to paste it.

Tested with

        let downloadTask = Amplify.Storage.downloadData(path: .fromString(key))

And I don't get a 403!

However

Source code doesn't flag this as deprecated nor does the documentation says anything about public which I think is confusing

Screenshot 2024-09-12 at 22 16 21 Screenshot 2024-09-12 at 22 15 56

I just checked the documentation and actually my bad, did not saw that auto completion selected key instead of path and I failed here ahah

ipodishima commented 2 months ago

Oh. Funny

In StorageCategoryBehavior protocol, it's flagged as deprecated

Screenshot 2024-09-12 at 22 21 00

Maybe that

public static internal(set) var Storage = StorageCategory()

should be

public static internal(set) var Storage: StorageCategoryBehavior = StorageCategory()

To get the compiler deprecation help?

[EDIT] I confirmed with using private let storage: StorageCategoryBehavior = Amplify.Storage instead of Amplify.Storagedirectly BUT you're losing default parameters

ruisebas commented 2 months ago

Great to hear the issue's been solved!

And thanks for bringing up the function not being marked as deprecated on Xcode. It's probably related to what you've said and we just need to mark the actual implementation as deprecated as well.

Regarding the public/ folder, in case you're interested, that's just one of the folders that Amplify Gen1 defines by default. When using keys in the Amplify APIs, these folders were not meant to be included as part of the key but rather configured as accessLevels in the API options, with .guest being the default. But all of these is now deprecated and path is the way to go :)

We will take a look at ensuring these old APIs appear as deprecated when attempted to use, but in the meantime I'll close this issue as the actual error has been resolved.

Thanks!

github-actions[bot] commented 2 months ago

This issue is now closed. Comments on closed issues are hard for our team to see. If you need more assistance, please open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.