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

Add option to authenticate to secure assets #101

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

andgdk
Copy link
Contributor

@andgdk andgdk commented Nov 17, 2024

Motivation

Currently, for environments with secure assets delivery enabled, the resulting backup contains assets with a file size of 0 kB in the assets/ folder.

This is critical as for certain users, the backup is incomplete, and no error/warning is provided.

Preposition
Support could be added by providing the bearer token to the fetching of assets.

In addition, this PR adds warnings, if the exported asset's size does not match the expected size.
The result made me curious, as one of the assets from the backup.zip test reports a warning when testing with backupEnvironment (the size of the .jpg: expected 27_093 bytes, got 27_002 bytes).
Even though, this commit adds a warning for individual assets and reports a warning count as the result of the operation ("Entities from environment <ENV_ID> were backed up with X warning(s) into ...).
This commit takes the liberty to rephrase "backuped" -> "backed up".

Exporting entities from environment id <ENVIRONMENT_ID>

Exporting: collections
[...]
Exporting: assetFolders
Exporting: assets
Exporting: file kontent-app.webp.
Exporting: file kontent-calendar.png.
Exporting: file kontent-logo-2.avif.
Exporting: file kontent-ai.png.
Exporting: file kai-hp-highlight-3.jpg.
Size mismatch: expected 27093 bytes, got 27002 bytes.
Exporting: file kai-logo-ver-black-rgb.svg.
Exporting: file kai-logo-ver-neg-rgb.svg.
Exporting: webhooks
Exporting: webSpotlight

Entities from environment <ENVIRONMENT_ID> were backd up with 1 warning(s) into 2024-11-24-10-55-backup-<ENVIRONMENT_ID>.zip.

Only ran unit tests.

Checklist

  • Code follows coding conventions held in this repo
  • Automated tests have been added
  • Tests are passing
  • Docs have been updated (if applicable)
  • Temporary settings (e.g. variables used during development and testing) have been reverted to defaults

How to test

Set up a new environment and contact Kontent.ai support to enable secure asset delivery for at least the delivery API.

Partially resolves #100

This commit adds warnings, if the exported asset's size does not match the expected size.
The result made me curious, as one of the assets from the `backup.zip` test reports a warning when testing with `backupEnvironment` (the size of the `.jpg`: expected 27_093 bytes, got 27_002 bytes).
Even though, this commit adds a warning for individual assets, and reports a warning count as the result of the operatoin ("Entities from environment <ENV_ID> were backd up with X warning(s) into ...).
This commit takes the liberty to rephrase "backuped" -> "backed up".

```
Exporting entities from environment id <ENVIRONMENT_ID>

Exporting: collections
[...]
Exporting: assetFolders
Exporting: assets
Exporting: file kontent-app.webp.
Exporting: file kontent-calendar.png.
Exporting: file kontent-logo-2.avif.
Exporting: file kontent-ai.png.
Exporting: file kai-hp-highlight-3.jpg.
Size mismatch: expected 27093 bytes, got 27002 bytes.
Exporting: file kai-logo-ver-black-rgb.svg.
Exporting: file kai-logo-ver-neg-rgb.svg.
Exporting: webhooks
Exporting: webSpotlight

Entities from environment <ENVIRONMENT_ID> were backd up with 1 warning(s) into 2024-11-24-10-55-backup-<ENVIRONMENT_ID>.zip.
```

Partially fixes kontent-ai#100
@andgdk andgdk force-pushed the add-secure-assets-bearer-token branch from 753d141 to 2f880a6 Compare November 24, 2024 14:28
@andgdk
Copy link
Contributor Author

andgdk commented Nov 24, 2024

Investigating a way to surface warnings to users lead me to the following investigation:
d4d07406-adfd-404b-bbd5-58c5de6adfd2-kai-hp-highlight-3.jpg reports a warning when comparing the blob size with the size reported by it's metadata.

d4d07406-adfd-404b-bbd5-58c5de6adfd2-kai-hp-highlight-3.jpg

  • backup.zip: 27,093 bytes (baseline)
  • Kontent.ai app UI: 26.45 kB (26.45 kB: 27084.8 bytes; 26.46 kb: 27095.04 bytes) (probably rounding error)
  • Running backupEnvironment method:
    • CLI: 27,002 bytes (res.size) (nok)
      See PR snippet below1
  • 2024-11-24-10-21-backup-<UUID>.zip: 27,002 bytes (nok)

I uploaded some random .jpeg and .png from my disk. Running backupEnvironment with the additional files reports errors on those files as well. One with significant difference: Size mismatch: expected 385_657 bytes, got 291_500 bytes (.jpeg).

Because of those results, I have to question my approach in comparing the file size. However, my expectation would still be that res.size should always be equal to asset.size. Perhaps there is a compression step which is not reflected in the metadata?
What is your take on this @IvanKiral?

I am hesitant to invest in changing the unit tests without a strategy regarding error handling/warnings or suitable assertions.

Footnotes

  1. https://github.com/andgdk/kontent-ai-data-ops/blob/2f880a6bf05e925b98992eb7dcae83108f638093/src/modules/backupRestore/backupRestoreEntities/entities/assets.ts#L83-L102

@andgdk andgdk marked this pull request as ready for review November 24, 2024 14:42
@andgdk andgdk requested review from JiriLojda and a team as code owners November 24, 2024 14:42
@IvanKiral
Copy link
Contributor

Well, it seems that there is indeed a compression step. I investigated URLs, and they differ for downloading the asset and the public URL for the asset in the UI. Unfortunately, it seems that this is how a product works, and right now, backing up an asset using this tool would mean that it is being compromised (probably some metadata is missing:/). The best we can do is submit feedback on the product if you are interested.

About secure assets - the use case is not utilized by that many people as far as I know, however, we might merge your solution with slight changes if you would like to. It would mean reverting the last commit and probably in the documentation add some information about the newly added parameter, and probably a link to the Kontent. ai learn (https://kontent.ai/learn/docs/security/secure-access/javascript#a-retrieve-assets-securely). What do you think?

@andgdk
Copy link
Contributor Author

andgdk commented Nov 30, 2024

Hi @IvanKiral,
just pushed the requested changes, please have a look. Please be aware that I did not run integration tests for this PR.
Let me know if you won't squash the commits during merge, and if I should eliminate the commits regarding the asset assertion.

I did not add the result of your investigation regarding a potential compression/metadata modification to the readme.
However, the known limitation section at the end of the backupRestore readme might be suited to do so - I leave it up to you.

For enterprise customers I feel the situation regarding the compression should be investigated, at least this needs clarification from my point of view. So, I can contact the support, and mention you as well, if you don't intervene. I would be also happy if you were in the position to keep track of this.

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 this pull request may close these issues.

Support secure assets
2 participants