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

feat: store only user ID in session #1849

Merged
merged 5 commits into from
Jun 9, 2021
Merged

Conversation

jia1
Copy link
Member

@jia1 jia1 commented May 10, 2021

Closes #212

@karrui
Copy link
Contributor

karrui commented May 11, 2021

Might want to fix tests! Check that we do not retrieve the user id in session with the id modifier and stick to _id (but type checking should catch)

EDIT: I checked, no usages of id anywhere for the session user, now we just need to fix the tests

Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

actually, in src/app/loaders/express/logging.ts, we have a getter for session.user._id for logging and that may break if we don't update that too

Don't need to do this, since we are just removing everything from session.user except for _id

src/types/vendor/express.d.ts Outdated Show resolved Hide resolved
src/types/vendor/express.d.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

should be ok now, just need to fix the build error (remove the user.email call (since session doesn't contain the full user anymore) and replace it with _id i guess))

also search the codebase for instances of TODO(#212) and remove them! :D

@jia1
Copy link
Member Author

jia1 commented Jun 8, 2021

@karrui Do you mean the line containing: message: Successfully logged in user ${user._id},? Reference to .email was removed but pipeline still fails. Travis build gives me an empty screen.

@jia1 jia1 requested a review from karrui June 8, 2021 10:36
@jia1 jia1 self-assigned this Jun 8, 2021
@jia1 jia1 added the contribute free for contributors to pick up label Jun 8, 2021
@karrui
Copy link
Contributor

karrui commented Jun 9, 2021

@karrui Do you mean the line containing: message: Successfully logged in user ${user._id},? Reference to .email was removed but pipeline still fails. Travis build gives me an empty screen.

@jia1 I've fixed it, can check out my commit for more info!

@karrui karrui marked this pull request as ready for review June 9, 2021 02:45
Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

lgtm

@karrui karrui merged commit f381be8 into develop Jun 9, 2021
@karrui karrui deleted the feat/use-only-userid-in-sessions branch June 9, 2021 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribute free for contributors to pick up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should only store userId in sessions instead of the entire user object
2 participants