hoodiehq / hoodie-client

:dog: Client API for the Hoodie server
Apache License 2.0
34 stars 25 forks source link

hoodie.account options are not applied #145

Closed flootr closed 7 years ago

flootr commented 7 years ago

I tried to set the cache property for the account api through the hoodie client constructor. Unfortunately, the following snippet doesn't work:

const accountCache = {/* get, set, unset */}
const hoodie = new Hoodie({
  url: 'arbitrary-hoodie-url',
  account: {
    cache: accountCache
  }
});

It seems, that line 74 in @hoodie/client/lib/get-api.js does not consider the options provided by the user. However, according to the lodash docs for defaultsDeep this is correct behaviour:

// https://lodash.com/docs/#defaultsDeep
_.defaultsDeep({ 'a': { 'b': 2 } }, { 'a': { 'b': 1, 'c': 3 } });
// => { 'a': { 'b': 2, 'c': 3 } }

Option a.b from the second object isn't applied which would be stateOptions.cache in my case. I'm not sure, but I think is isn't correct, is it? Would maybe lodash/merge be a better fit (edit: or switch arguments)? Please let me know, if I got something wrong here.

gr2m commented 7 years ago

I think that we wouldn’t let the user set a differnt url or cache API when using the Hoodie constructor, all other options are passed trough though.

Could you explain your use case for the custom accountCache API?

flootr commented 7 years ago

Ah, didn't know this was intended.

Could you explain your use case for the custom accountCache API?

I thought of using cookies instead of localStorage to share a session between the server- and client-rendered part of an application. However, I'm not at the point where I could say that I'd definitely need this. I'd like to check if there is a valid hoodie session on the server and send different html based on that information.

gr2m commented 7 years ago

gotcha! We had this requirement in the past for people who were working on server-side rendering. I guess you are trying to do something similar?

I think back then we forked Hoodie and made it so that a sign in sets a cookie additionally to the current session handling. I thought we could maybe add an option for that to hoodie.account.signIn(). On sign out, we can always check server side if a session cookie is set and if it is, we can delete it.

There is some security consideration you have to make because of Hoodie has built-in CORS support, and we have to make sure that the cookie is only sent for requests that are coming from your app’s domain, not from any other website that might otherwise steal your session and access your data.

But long story short: we want to support server-side rendering, just didn’t think it trough yet

flootr commented 7 years ago

I guess you are trying to do something similar?

Yes, I want to render (parts of) an application on the server and for example send html which is tied to the authentication status of the user.

It may be sufficient for my case though to only store a cookie which indicates whether a user is logged in but without session information, because I currently don't want to send actual data (would this even be possible with hoodie at the moment?).

we want to support server-side rendering, just didn’t think it trough yet

👍 I'm, too, not quite through all my thoughts about what I need to consider, especially the security aspect you mentioned.

I'd close this issue as the original bug report isn't actually a bug but I'd like to follow up on discussions regarding sever side rendering and hoodie.