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

New lesson: Session-based authentication #29082

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

MaoShizhong
Copy link
Contributor

Because

As part of the Node revamp's 2nd milestone

This PR

Issue

Closes #28847

Additional Information

Pull Request Requirements

  • I have thoroughly read and understand The Odin Project curriculum contributing guide
  • The title of this PR follows the location of change: brief description of change format, e.g. Intro to HTML and CSS lesson: Fix link text
  • The Because section summarizes the reason for this PR
  • The This PR section has a bullet point list describing the changes in this PR
  • If this PR addresses an open issue, it is linked in the Issue section
  • If any lesson files are included in this PR, they have been previewed with the Markdown preview tool to ensure it is formatted correctly
  • If any lesson files are included in this PR, they follow the Layout Style Guide

'Persisting logins' was a glorified intro anyway
Only up to the original login process
Reordered setup code blocks for easier content flow.
Decided to introduce the session store and secret explanations here
instead of their own sections later on (not really needed there).
New order allows for a more natural way of explaining how
express-session populates req.session.
That can be explained at the start and options explained to reflect
that, instead of explaining options in a bit of a black hole then
provide the context later in the lesson.
Made more sense to talk about logging out after logging in, then talk
about password hashing afterwords.
Use subsections for better content organisation and linking
@github-actions github-actions bot added the Content: NodeJS Involves the NodeJS course label Nov 13, 2024
"Session-based authentication" is a more appropriate title for the
lesson than just "Sessions", given that sessions are not exclusively
used for auth purposes.
@01zulfi 01zulfi self-requested a review November 13, 2024 16:19
Makes sense to showcase them directly rather than just via text example after the fact

#### Session store

Let's talk about our session config which we apply to every incoming request (by mounting it on `app`). Firstly, we use the [connect-pg-simple](https://www.npmjs.com/package/connect-pg-simple) library to make express-session store session data in our database (creating a "session" table if it does not already exist). Without this, sessions would be stored in memory by default which would not persist through any server restarts!
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there pedagogical benefit to having the learner make their own sessions table and writing their own helper functions to do CRUD ops on them? If the aim of the lesson is to get to grips with sessions, I'm not sure it's a good idea to abstract away the actual persistent session info

Copy link
Contributor Author

@MaoShizhong MaoShizhong Nov 16, 2024

Choose a reason for hiding this comment

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

Possibly. My understanding from prior discussion (which may be flawed) was to stay with express-session but handle it manually rather than as part of passport and its strategies. But if @01zulfi thinks it'll be even better to do this lesson with properly manual session management then the lesson can be amended accordingly.

Edit: I think the reason I'd personally prefer to stick with express-session is that to go full manual, users would also need to implement additional things like session secrets, signing and verifying. I feel like that's something that makes express-session okay to use for this lesson. Perhaps it may be appropriate to mention that users can view the session table itself in the db to see what gets stored there? But I'm not 100% convinced we have to have users do everything complete from scratch (including stuff like using session secrets) to get the right lesson across.

Copy link
Member

Choose a reason for hiding this comment

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

We want our learners to experience setting up the session table from scratch, insert and read from it, set up cookies and set up middlewares for auth. That's it, no other fancy stuff. That's where we introduce these libraries.

But... now that I've skimmed through the lesson, it does a great job teaching session auth. I'll need to come back to this comment with concrete thoughts.

Copy link
Contributor Author

@MaoShizhong MaoShizhong Nov 25, 2024

Choose a reason for hiding this comment

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

No problem at all. Let me know which direction you'd prefer the lesson to go in and I can adjust if necessary. Going a full manual approach would mean implementation would likely be a lot more simple (no secrets and stuff - I feel manually implementing that would be out of the scope of such a lesson, but also I feel the topic mention is key enough to sessions in general.

At the bare minimum, the current lesson can do with exploring what sessions would look like in the table.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content: NodeJS Involves the NodeJS course
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Lesson: Sessions
3 participants