Amplify-js: Storage using private by default

[LosD]

Describe the bug
It seems that since #2898 was merged, the default storage level is now private. I _think_ that the issue is not making the change on a copy, but directly on the configuration coming from StorageClass (see "Addition context" for details).

To Reproduce
Steps to reproduce the behavior:

1. Configure Amplify with storage enabled
2. Get or store something with the Storage object.

Expected behavior
With no special configuration, public level should be used (unless Storage.vault is used).

Actual behavior
The private level is used.

Desktop (please complete the following information):

• OS: Linux
• Browser Chrome
• Version 1.1.26 (Storage object is gotten through the Amplify-Angular service, version 2.1.12 but I don't think that's really related)

Additional context
The relevant code seems to be here (from packages/storage/src/index.ts)

        logger.debug('storage configure called');
        const vaultConfig = old_configure.call(_instance, options);

        // set level private for each provider for the vault
        Object.keys(vaultConfig).forEach((providerName) => {
            if (typeof vaultConfig[providerName] !== 'string') {
                vaultConfig[providerName] = { ...vaultConfig[providerName], level: "private" };
            }
        });
        logger.debug('storage vault configure called');
        _instance.vault.configure(vaultConfig);

When stepping through , it's pretty clear that _instance._config is updated along with the one used for the vault. AFAIK, _instance is the actual Storage object (which would explain why everything is loaded using the private level since I updated to the latest version).

[Michael-372]

I had the same issue frustrating me for a day or two. Based on your discovery, I removed aws-amplify from my project and ran :
npm install [email protected]
The private level is no longer configured as default. And resources can be used as normal. Thanks!

[LosD]

@TheIceBreaker7 you're very welcome.

I should note that it's easy to work around, once found. Just provide your own config with level set to public/protected to the call. However, I do not believe that configuring the Storage instance would help, as the chosen level would be overwritten by the buggy code.

[Michael-372]

Yeah, if you configure your storage object, it will get overwritten. You need to add the level to every call to Storage for the latest version to work. Or just rollback and wait for this to be resolved :D

[LosD]

Luckily, I'm using a helper class, so the change was pretty easy for me, but yeah a rollback would probably be easier in many cases.

[elorzafe]

@LosD or @TheIceBreaker7 can you add a code snippet of your app that is using AmplifyService with Storage?

[LosD]

Sure, though it'll have to wait until tomorrow. I might do better and make a testcase and a fix. Should be simple.

[elorzafe]

@LosD I have tested [email protected] and it works as expected, the default level is public, it only changes to private if I do Storage.configure( { level: 'private' } )

[elorzafe]

I have this very simple react-app

import React, { Component } from 'react';
import logo from './logo.svg';
import './App.css';
import Amplify, { Storage } from 'aws-amplify';
import awsmobile from './aws-exports'

Amplify.configure(awsmobile);

class App extends Component {

  async componentDidMount() {
    console.log(await Storage.get('test.txt')); // the url has public prefix
  }

  render() {


    return (
      <div className="App">

      </div>
    );
  }
}

export default App;

[LosD]

@elorzafe That's because you're not using the global Amplify.Storage object.

Here's a test showing the problem:

        test('do not break public when using global Amplify.Storage instance', () => {
            const storage = Amplify.Storage;

            Amplify.configure({
                Storage: {
                    region: 'region',
                    bucket: 'bucket',
                },
            });
            expect(storage['_config']).toEqual({
                AWSS3: {
                    bucket: 'bucket',
                    region: 'region'
                }
            });
        });

... Which yields:

Expected value to equal:
  {"AWSS3": {"bucket": "bucket", "region": "region"}}
Received:
  {"AWSS3": {"bucket": "bucket", "level": "private", "region": "region"}}

[LosD]

As expected, changing line 31 of packages/storage/src/index.ts from:

        const vaultConfig = old_configure.call(_instance, options);

to

        const vaultConfig = { ...old_configure.call(_instance, options)};

Fixes the problem. I'll make the test a bit more fitting, e.g. not pollute the tests by using the Amplify service registry to get the global Storage instance, then I'll make a PR.

[LosD]

Okay, I've created a PR.

@elorzafe By the way, wouldn't instantiating Storage directly as you did when attempting to reproducing leave the instance with an undefined vault field? In the long term, it might be better not to do magic when instantiating the global instance, and instead configure the vault directly from inside StorageClass. Different behaviour from the global instance and local instances seems... Not so good.

Edit: Ah, I get why it was done this way, it would have taken quite some refactoring to do it in the StorageClass when the vault is the same class. Hmmm... Not sure what the best way would be. Maybe some kind of wrapper class (though not the prettiest of solutions)?

[LosD]

@elorzafe @manueliglesias Could I get someone to look at this, and possibly the PR?

At least to me, it's a pretty critical regression that potentially breaks for everyone using the registered/global instance (which includes those using the Angular service), to make it worse, it is in a way that will not be clear until later when the files are needed, so a code change is not enough, it requires actively fixing the storage to get out of the mess.

It's a bit of a mitigating factor that including the level with individual requests will entirely avoid the issue, but still.

[elorzafe]

@LosD thank you so much for finding this, I could reproduce it after calling configure a second time. You can test this using the @unstable tag.