Design discussion about nextcloud-browser-storage

[sualko]

We need nearly the complete storage interface except for the storage . key ( n ) method and the length property.

[nickvergessen]

So lets do this as a V1 and then continue from there?

[ChristophWurst]

We should have a common/protected namespace for a guest username, so talk, deck, text,... Can all use the same guest name

👍 for scoped/namespaced storage per app.

What about clearing data? Anything sensitive should be wiped on logout. But apparently jsxc has data that should persist. Do we need two types of storage/values?

[nickvergessen]

For Talk we would mostlikely only add stuff that would be okay to persist like:
Guest name
Sidebar opened/closed
Microphone/Camera on/off (and in the future selection in case you have multiple)

[georgehrke]

For calendar it would be nice to keep a list of calendars. So something we definitely have to wipe on logout.

[ChristophWurst]

also cc @georgehrke @juliushaertl @skjnldsv :)

[nickvergessen]

We should have a common/protected namespace for a guest username, so talk, deck, text,... Can all use the same guest name

[sualko]

what are the basic operation you need for jsxc?

I need a way to store some data in a way that it's not deleted after logout, but I think in general Nextcloud should only delete data which was stored by an Nextcloud App (using this new api for example). Otherwise Nextcloud potentially messes with other applications running under the same domain, due to the fact that NC supports installation in a subdirectory.

[ChristophWurst]

Sure. As soon as we have our own abstraction that can scope the storage, it should be also trivial to scope that.

[sualko]

Something like the following should do the trick:

class Storage {
  private prefix = 'NC/';

  constructor(prefix, private backend = localStorage) {
    this.prefix += prefix + '/';
  }

  public setItem() {
    let args = [...arguments];

   // there is probably a smarter solution
   args[0] = this.prefix + args[0];

    this.backend.apply(this.backend, args);
  }

  // wrap all other methods

  public clear() {
     let prefixRegex = new RegExp('^' + this.prefix);
     let keys = Object.keys(this.backend);

    // we could also filter the array
    for (let key of keys) {
      if (prefixRegex.test(key)) {
         this.backend.removeItem(key);
      }
    }
  }
}

[ChristophWurst]

Let's leave implementation aside.

setItem and clear are the only operations you need? What about retrieving an element?

[ChristophWurst]

How do we want to handle the requirement app A wants data cleared on logout, app B doesn't? I started the implementation at #2. I used a similar API as we have for https://nextcloud.github.io/nextcloud-logger/. The builder already has an option for persist, but right now that just determines if you'll get session storage or local storage. There could be something similar that says I want persistend storage that survives logouts. It's just a matter of storing this flag internally and checking on clear.

[ChristophWurst]

Please see #2 :)

[sualko]

I was thinking a lot about this in the last few days and I think the whole idea to implement a session store on top of a persistent storage is crazy. If an app wants to store some information only for the current session, it should use the session storage (this could also be cleared on logout) and if an app wants to store information persistent it should use the local storage. This was probably the intention when those interfaces were specified.

In the case that NC really wants to clear the persistent storage, there should be a possibility for an app to opt-out. Therefore I propose the following:

If an app really wants to use a persistent store it should create a key in the form [nc prefix][preserve keyword][app name] (e.g. nc:preserve:ojsxc) with the prefix it is using (e.g. ojsxc:). Nextcloud could now search for those entries and preserve them. An implementation could look like the following:

let preservePrefixes = Object.keys(localStorage).filter(key => key.startsWith('nc:preserve:')).map(key => localStorage.getItem(key));

let regex = new Regex('^(' + preservePrefixes.join('|') + ')');
Object.keys(localStorage).forEach(key => !regex.test(key) ? localStorage.removeItem(key) : null);

That would be maybe slightly slower, but it's easy to implement and it doesn't require to change any app code.

[ChristophWurst]

This was probably the intention when those interfaces were specified.

Only partly true because a Nextcloud session can live longer than a browser session due to the remember-me feature.

An implementation could look like the following:

Yes, just like I implemented it here https://github.com/nextcloud/nextcloud-browser-storage/pull/2/files#diff-ce25cc9fe753b7439e6c13a3a30df75cR30-R34. This only clears the values saved through the abstraction.

[sualko]

Only partly true because a Nextcloud session can live longer than a browser session due to the remember-me feature.

I didn't consider that. You are right.