Csanád's review of chapter 19

chapter_19_spiking_custom_auth.asciidoc

@@ -91,6 +91,7 @@ but this is just a fun toy project so let's give it a go.
 To get this Magic Links project set up, the first thing I did was take a look at existing Python and Django authentication
 packages, like https://docs.allauth.org/en/latest/[django-allauth]
 and https://github.com/omab/python-social-auth[python-social-auth],
+// CSANAD: python-social-auth looks abandoned
 but both of them looked overcomplicated for this stage
 (and besides, it'll be more fun to code our own!).
 
@@ -109,9 +110,14 @@ and convince yourself that it really does work.
 
 ==== Starting a Branch for the Spike
 
+// CSANAD: although we introduced to expression "spike" in the 17th chapter, I
+// would still mention more explicitly that this is a quick and dirty hacking
+// around, which is not production code, and this is the reason we are not
+// writing tests just yet - we still need to see what and how to test.
+
 ((("spiking and de-spiking", "branching your VCS")))
 ((("Git", "creating branches")))
-This spike is going to be a bit more involved that the last one,
+This spike is going to be a bit more involved than the last one,
 so we'll be a little more rigorous with our version control.
 
 Before embarking on a spike it's a good idea to start a new branch,
@@ -178,18 +184,26 @@ and a logout link for users who are already authenticated:
 ((("Django framework", "sending emails", id="DFemail18")))
 ((("send_mail function", id="sendmail18")))
 ((("emails, sending from Django", id="emails18")))
-The login theory will be something like this:
+The login in theory will be something like this:
 
 - When someone wants to log in, we generate a unique secret token for them,
   store it in the database linked to their email, and send it to them.
+// CSANAD: although it should be obvious there is no "database linked to their
+// email", still, I suggest a different order of words for clarity:
+// "store it linked to their email in the database, (...)"
+// or
+// "store it linked in the database to their email, (...)"
 
 - The user then checks their email,
   which will have a link to a URL that includes that token.
 
-- When they click that link, we check whether the token exists in database,
+- When they click that link, we check whether the token exists in the database,
   and if so, they are logged in as the associated user.
+// CSANAD: what do you think about rephrasing it a little:
+// "...and if so, we authorize/approve the request to authenticate/log in as the
+// associated user"
 
-// https://docs.djangoproject.com/en/5.0/topics/auth/customizing/
+// https://docs.djangoproject.com/en/5.1/topics/auth/customizing/
 
 
 First let's prep an app for our accounts stuff:
@@ -296,7 +310,7 @@ with our 'base.html' in the real version.)
 
 The https://docs.djangoproject.com/en/5.1/topics/email/[django docs on email]
 explain how `send_mail()` works, as well as how you configure it
-by telling Django what email to server to use,
+by telling Django what email server to use,
 and how to authenticate with it.
 I'm just using my Gmailfootnote:[
 Didn't I just spend a whole intro banging on about the privacy implications
@@ -334,7 +348,9 @@ TIP: If you want to use Gmail as well,
 ((("", startref="DFemail18")))
 ((("", startref="SDemail18")))
 
-
+// CSANAD: I was only able to make it work with 2FA enabled and the
+// app-specific password set. If I have time, I'll try again with allowing
+// less secure apps too.
 
 ==== Another Secret, Another Environment Variable
 
@@ -461,9 +477,17 @@ OK so we've done the first half of "generate and recognise unique tokens":
 
 Before we can move on to recognising them and making the login work end-to-end though,
 we need to sort out user authentication in Django.
+// CSANAD: not sure if it's clear for the readers why we need to sort out the
+// authn first. Maybe we could just rephrase:
+//
+// "In order to move on to recognising them and making the login work end-to-end,
+// we need to sort out user authentication in Django.
+//
+// This way we aren't just stating what needs to be done first, but also the
+// reason behind it.
 The first thing we'll need is a user model.
 I took a dive into the
-https://docs.djangoproject.com/en/5.0/topics/auth/customizing[Django
+https://docs.djangoproject.com/en/5.1/topics/auth/customizing[Django
 auth documentation] and tried to hack in the simplest possible one:
 
 [role="sourcecode"]
@@ -523,6 +547,12 @@ class ListUserManager(BaseUserManager):
 ----
 ====
 
+// CSANAD: ListUserManager has to be defined before ListUser, since its
+// reference to ListUser isn't evaluated until `create_user` is called. This is
+// not the case the other way around, ListUser's reference to ListUserManager
+// is instantiated in the class definition. Maybe we could leave a note about
+// this?
+
 
 No need to worry about what a model manager is at this stage;
 for now we just need it because we need it, and it works.
@@ -542,7 +572,7 @@ $ pass:quotes[*python src/manage.py migrate*]
 Running migrations:
   Applying accounts.0002_listuser... OK
 ----
-/ch18l009-1
+//ch18l009-1
 
 
 ==== Finishing the Custom Django Auth
@@ -562,11 +592,14 @@ Hmm, we can't quite cross off anything yet.
 Turns out the steps we _thought_ we'd go through
 aren't quite the same as the steps we're _actually_ going through
 (this is not uncommon as I'm sure you know).
+// CSANAD: I find it vague like this. Maybe it would be helpful to clarify what
+// it is that "we are actually going through" and what it was that
+// "we thought we'd go through".
 
 Still, we're almost there--our last step will combine recognising the token
 and then actually logging the user in.
 Once we've done this,
-we'll be able to pretty much strike off all the items on our scratchpad:
+we'll be able to pretty much strike off all the items on our scratchpad.
 
 
 So here's the view that actually handles the click through from the link in the
@@ -605,6 +638,9 @@ def login(request):
 
 The `authenticate()` function invokes Django's authentication framework,
 which we configure using a "custom authentication backend",
+// CSANAD: why the quote marks? If it's because this is a reference to the
+// official Django docs, we could just make it a link instead to
+// https://docs.djangoproject.com/en/5.1/topics/auth/customizing/#writing-an-authentication-backend
 whose job it is to validate the UID and return a user with the right email.
 
 We could have done this stuff directly in the view,
@@ -968,6 +1004,8 @@ what information you want to track about users,
 from explicitly recording first name and last namefootnote:[
 A decision which you'll find prominent Django maintainers
 saying they now regret.  Not everyone has a first name and a last name.]
+// CSANAD: also, profile-related information has no business in auth-related
+// models.
 to forcing you to use a username.
 I'm a great believer in not storing information about users
 unless you absolutely must,