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

Restore cookieExpirationTime from cookie value #1257

Merged
merged 5 commits into from
Mar 16, 2017

Conversation

stevenwu
Copy link
Collaborator

When AdaptiveStore is using a CookieStore, the cookie values (name, domain, path) are only accessible through AdaptiveStore._cookies. I don't think they are restored, so proxyToInternalStore cannot access them. Do they need to be set in init as well?

/cc @marcoow

Copy link
Member

@marcoow marcoow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! I think we can actually simplify this a bit though as we can simply always override the expiration time with a value (potentially) read from a cookie.


let expirationCookieName = `${this.get('cookieName')}-expiration_time`;
let expirationTime = parseInt(options._cookies.read(expirationCookieName), 10);
this.set('cookieExpirationTime', expirationTime);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this here actually. When the cookie service is instantiated it will restore its own cookieExpirationTime from the cookie anyways.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure whether to add this, or change the test at https://github.com/simplabs/ember-simple-auth/pull/1257/files#diff-37b831f2713acdc8f0acf654aa735d70R310.

If the store is an Adaptive store, should we do something like

expect(store.get('_cookies.cookieExpirationTime')).to.equal(expirationTime);

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because the adaptive store's cookieExpirationTime property is already an alias for the underlying cookie store's cookieExpirationTime property anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test fails without this code. Maybe there's a bug elsewhere?

@@ -159,6 +159,11 @@ export default BaseStore.extend({
init() {
this._super(...arguments);

if (!this.get('cookieExpirationTime')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we actually always want to override cookieExpirationTime. The idea here is that if you define your application session store as

export default Cookie.extend({
  cookieExpirationTime: 60 * 60
});

but then have a remember-me action in the UI that updates the expiration time like

rememberMe() {
  this.set('session.store.cookieExpirationTime', 60 * 60 * 24);
}

you'll want that value of 60 * 60 * 24 to be applied after the next reload although the initial value of 60 * 60 would be set in this case as well.

@marcoow
Copy link
Member

marcoow commented Mar 14, 2017

Hm, there's only one thing that I just though of - if we always override the expiration time with the value restored from the cookie you could actually never change the expiration time later on.

store = this._createStore(Cookie, options);
this.set('cookieExpirationTime', store.get('cookieExpirationTime'));
Copy link
Collaborator Author

@stevenwu stevenwu Mar 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If cookieExpirationTime is not set here, then it stays null because the setter in proxyToInternalStore is never called.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense 👍

@marcoow marcoow added the bug label Mar 16, 2017
@marcoow marcoow merged commit f2a8268 into mainmatter:master Mar 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants