Module 4 Feedback

[louisrli]

1 Securing a Backend App

• I'm not really sure this section looks that much different from the last
  time I gave feedback on the PR, so I'll link what I wrote last time and reiterate
• In general, I feel this section has a really weird logical flow.
• The "Database security" section will most likely mean absolutely nothing to a beginning student at the bootcamp ("basic isolation", "protect and encrypt" means what?, "DBaaS"?). "API Throttling" has no explanation of DDos and DoS, etc, there's a lot of "term dropping" in this section and not much explaining.

1.1 Defining Authentication Layer

• The word Database is randomly capitalized
• You wouldn't normally put the hash function on the model itself, or if it did, it should be a private function. In a real codebase, it would be more appropriate for hash to either be in a library/helper file or as a private method, as it's not really a detail that needs to be exposed in the interface of the model.
• Basically at line 166 it would just call a function private to the model file.
• Line 185 constants are better defined outside of the function they're called,
  so const SALT_ROUNDS = 10;, this is pretty standard across all codebases.
• "Usually, the registration logic is inside a controller, but it can also sit with the model for encapsulation. Here it was defined in the model with very simple validation." I agree with this statement, which is why I feel this should definitely be implemented with best practices. I wouldn't encourage students to violate putting logic in the right places until they have a better grasp of why such patterns exist. Basically, I'm saying that this code should be written with the logic inside a controller and less logic in the model, so that you can reinforce the concept of MVC, rather than showing students code with poorly allocated responsibilities.
• "hash the password before saving in the database". This and the sentence after again seem to say a lot of things that will not make sense to the student when they read it (since the terms are not explained). It will make sense after the next section, but the goal should be for the reader to understand the sentence as they read it. It's unpleasant for the reader to read a sentence, not understand it at all, then have to return to it later.
• I don't think a hash function is ever explained, or if it was in the first section, perhaps a one sentence refresher (it takes an input and maps it to an output in some finite n-character space)
• In the part about the attacker determining that you are not using salts...if I'm not mistaken, salt still hasn't been explained yet. I see it being explained in the next section, but see above about how you want readers to understand sentences when they read it, not go back afterwards.
• "hard to crack due to being super slow" Which part is slow, the algorithm itself, or the attempt to crack it?
• The part about the breakdown segments of the hash output should IMO definitely be in the beginning -- that's a good diagram to explain how passwords are stored. I would suggest starting off by showing a database with bcrypt generated passwords, then explaining each part.
• What is a "time-safe comparison"? Again, it's explained in the next paragraph, but at the time of reading it, the reader won't know what it means.

1.2 Blog Auth

Looks fine generally, you might save time by simply telling them to read the tests rather than writing what the tests do (this is also dangerous as it can get out of sync if you change the assignment)

1.3 Authentication Persistence

• Were environmental variables explained in any previous section? There is a reference to them when discussing the APP_SECRET. I'd emphasize not to put it inline in a code variable (which they will be more likely to understand)
• Instead of hardcoding cookie.secure to false, you can also use the node env environmental variable, though this might be too complicated for this example
• The thing about the "Remember Me" function should be in the intro. Again, give the reader a concrete example so that they have a concrete idea of what they're about to learn about ("oh, sessions can be used to do things like remember me")
• state-less -> stateless
• There is a warning about the SECRET and defined inline. Agree with the warning. And rather than doing this in the example code, just use an environmental variable in the example code.
• "Compromised Secret Key" -- is this really a con specific to JWT? Almost all systems have a single "passcode" or "secret", if people are compromising any of these, your application is probably screwed anyways :)
• "Data Overhead" -- is this also a real con? As you say, you need to add more claims, but in 99.9% of cases JWT is not going to be the core issue in any response-size problems. I don't like including this because it encourages premature optimization from students.

1.4 Adding Authorization Layer

• I don't think the "Authorization" and "Authentication" sections need to be bullet pointed. They are basically just paragraphs. If there were parallel bullet points between the two sections, it might make more sense, but otherwise consider just making them normal paragraphs.
• "passwords may be required in some cases but not in others" -- perhaps refresh reader on other types of authorization?
• It says the app has 3 access levels, and then there are four bullet points. Consider making three bullet points with Moderator:, Anonymous user:, etc prefixed it, so that the information is communicated very clearly.

1.5 Authorization through Middleware

• What is a "mount path"? Terminology being used before being explained.
• middlware -- typo
• sentry -- lowercased
• Section is generally fine.

1.6 Blog Auth Guard

• persis -- typo
• Looks fine

2 Auth Schemes

• In the section about "the login flow usually looks like this", consider interspersing concrete examples. e.g., the Identity Provider might be Facebook.
• As I continued reading, the paragraph that starts with "Let's take Github's as an example" is perfect, and IMO should be put earlier in the section to explain.
• I suggest defining the word "scope" upon its first usage

3 Other Vulnerabilities

• CSRF -- it's fine except the part about preventing CSRF attacks doesn't actually explain why the token prevents such an attack. Why does the token prevent the request from going through? Or how?
• Databse -- typo
• "Storing Database Credentials" -- not sure why there is a random reference to web.config encryption in ASP.NET, without any further links, etc. If you are like me and know nothing about ASP.net, this sentence is not very helpful
• ecommerse -- typo
• fascilitate -- typo
• The E2EE section seems out of place. This module was supposed to be about vulnerabilities.
• effectivley -- typo

4 Awesome TODOs lab

Looks fine