feat: Contributor backend 21

[SKairinos]

This change is 

[faucomte97]

Reviewed 35 of 35 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @SKairinos)

---

codeforlife/models/abstract_base_user.py line 56 at r1 (raw file):

        """A flag designating if this contributor has authenticated."""
        try:
            return self.is_active and not self.session.is_expired

Same question here about whether is_active includes being verified?

---

codeforlife/forms.py line 64 at r1 (raw file):

                "Incorrect user class.",
                code="incorrect_user_class",
            )

This would allow an attacker to enumerate existing email addresses on our system.

Code quote:

        if not isinstance(user, self.get_user_class()):
            raise ValidationError(
                "Incorrect user class.",
                code="incorrect_user_class",
            )

---

codeforlife/forms.py line 69 at r1 (raw file):

                "User is not active",
                code="user_not_active",
            )

This is not as much of an issue since they're inactive accounts but it could still provide the attacker with a list of emails to exclude from their brute force attack.

In both of these cases, I feel like we should raise the same Error of "invalid login" to keep it vague. In the same way that currently, we raise the same error if the credentials are incorrect or if the user's email isn't verified yet.

Which leads me to this question: what if a user tries to log in with an unverified email address? Does that fall under not user.is_active? If not, we should also raise the same error if they are unverified still.

Code quote:

        if not user.is_active:
            raise ValidationError(
                "User is not active",
                code="user_not_active",
            )

[SKairinos]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @faucomte97)

---

codeforlife/forms.py line 64 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

This would allow an attacker to enumerate existing email addresses on our system.

I don't agree because in order for this error to be raised the user would have needed to first successfully logged in. At this point in the flow, we are just sanity checking they have logged in as the correct user type.

In theory, this error should NEVER be raise. If this error is raised it'll be because of a server/developer error (not user error) as it shouldn't be possible for the user to log in as the incorrect type. For example, a student user shouldn't be allowed to log in a teacher user.

---

codeforlife/forms.py line 69 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

This is not as much of an issue since they're inactive accounts but it could still provide the attacker with a list of emails to exclude from their brute force attack.

In both of these cases, I feel like we should raise the same Error of "invalid login" to keep it vague. In the same way that currently, we raise the same error if the credentials are incorrect or if the user's email isn't verified yet.

Which leads me to this question: what if a user tries to log in with an unverified email address? Does that fall under not user.is_active? If not, we should also raise the same error if they are unverified still.

See above. I don't agree because in order for this error to be raised the user would have needed to first successfully logged in.

Agreed that we should raise the same error if they are unverified.

---

codeforlife/models/abstract_base_user.py line 56 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Same question here about whether is_active includes being verified?

Fixing this issue but not in the abstract base user class. Instead, the fix belongs in codeforlife.user.models.User.is_authenticated (included in this PR).

[faucomte97]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SKairinos)

---

codeforlife/forms.py line 64 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

I don't agree because in order for this error to be raised the user would have needed to first successfully logged in. At this point in the flow, we are just sanity checking they have logged in as the correct user type.

In theory, this error should NEVER be raise. If this error is raised it'll be because of a server/developer error (not user error) as it shouldn't be possible for the user to log in as the incorrect type. For example, a student user shouldn't be allowed to log in a teacher user.

Fair point, I hadn't considered this would only happen after successful login 👍