Skip to content
This repository has been archived by the owner on Apr 11, 2024. It is now read-only.

Ensure session expirations are Dates #132

Merged
merged 1 commit into from
Mar 16, 2021
Merged

Conversation

paulomarg
Copy link
Contributor

@paulomarg paulomarg commented Mar 12, 2021

WHY are these changes introduced?

It was pointed out (Shopify/koa-shopify-auth#65 (comment)) that the CustomSessionStorage class may cause issues if the returned session expires field isn't a Date object.

WHAT is this pull request doing?

Making sure that we allow strings to be returned for the expiration date, and handling the conversion to Date.

Type of change

  • Patch: Bug (non-breaking change which fixes an issue)
  • Minor: New feature (non-breaking change which adds functionality)
  • Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have added a changelog entry, prefixed by the type of change noted above
  • I have added/updated tests for this change
  • I have documented new APIs/updated the documentation for modified APIs (for public APIs)

@paulomarg paulomarg requested a review from a team as a code owner March 12, 2021 16:48
@paulomarg paulomarg force-pushed the ensure_session_expires_is_date branch from ac0bc56 to 5da6add Compare March 12, 2021 17:06
Comment on lines 43 to +48
let session = new Session(result.id as string);
session = {...session, ...result};

if (session.expires && typeof session.expires === 'string') {
session.expires = new Date(session.expires);
}

Choose a reason for hiding this comment

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

Suggested change
let session = new Session(result.id as string);
session = {...session, ...result};
if (session.expires && typeof session.expires === 'string') {
session.expires = new Date(session.expires);
}
const session = Session.cloneSession(result, result.id);

and following part port to Session.cloneSession.

        if (session.expires && typeof session.expires === 'string') {
          session.expires = new Date(session.expires);
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I don't think that would work - we can't guarantee that result will be of type Session here, since it may have been loaded from something like a JSON.parse() call.

If we tried to call Session.clone with result it would break in Typescript, and we shouldn't expect the expires field to be a string there (it is typed as Date so it would have to be set when the session is created anyway).

Choose a reason for hiding this comment

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

@paulomarg I got it.
My suggestion is for the following reasons:

I wish I could improve the point that users have to convert the session.expires to Date type.
(This seems to be solved if session.isActive() is implemented)

session = {...session, ...result}; overwrites session.id with result.id.
If this assignment is OK, why does Session.cloneSession exist?

Copy link
Contributor

@thecodepixi thecodepixi left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for fixing this!

@paulomarg paulomarg force-pushed the ensure_session_expires_is_date branch from 5da6add to 240a8e7 Compare March 16, 2021 16:49
@paulomarg paulomarg merged commit cc67004 into main Mar 16, 2021
@paulomarg paulomarg deleted the ensure_session_expires_is_date branch March 16, 2021 16:51
@paulomarg paulomarg temporarily deployed to production March 16, 2021 17:41 Inactive
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants