Server Error on permission checks

[janno42]

When an anonymous user (not logged in) requests a view that requires permission (e.g., grades.views.SemesterView), an AttributeError occurs because the AnonymousUser doesn't have the respective attribute (e.g., is_grade_publisher).

Instead, the permission decorator functions should always return False for anonymous users.

[niklasmohrin]

Hm, I would expect that this is caught by our RequireLoginMiddleware, I wonder if the behavior is the same for function- and class-based views

[richardebeling]

In

@no_login_required
class LegalNoticeView(TemplateView):
    # ...

the decorator code does

        class_or_function.dispatch.no_login_required = True

but dispatch here is a shared method between all the class views, so this is also applied to all other class based views (in this case, inheriting from the django View base class).

Funnily enough, this only triggers when the LegalNoticeView is executed first. In a test file, if we import any of the grades views, we will evaluate that first, and then the attribute will be missing (as there is some functools wrapping going on, which "freezes" the attributes at the time of definition by copying them from the wrapped element). So it's pretty easy to accidentally make this work in a test, although it doesn't work for users.

Django by now has a LoginRequiredMiddleware, but we need our specialization anyway to support the non-decorator exclusions for the OIDC views.

@niklasmohrin and me debated which approach we want to take in the future

• Mark views that don't require a login with an attribute. For class based views, this cannot be on the dispatch method, but needs to be on the class directly. This should be possible to do. Downside: Django might start interfering, if they use/set the same attribute for something else, more interference might happen. We could circumvent this by making our name evap-specific
• Instead of adding an attribute, keep a list of views that allow no login (by just keeping a WeakRef-Set of the functions/classes around). Problem: If they are wrapped by some other decorator afterwards, our "reference" becomes subtly wrong. We could circumvent this by making our middleware traverse the __wrapped__ chain.

[niklasmohrin]

I think I generally prefer the WeakSet approach because we can be sure that only we modify this set. Traversing the wrapped chain is a requirement, but that is fine I think, we don't construct overly long chains.

Still, to fix this issue either approach is fine. I agree that there isn't really a good way to write a regression test, so there is little churn to refactor it later. Either way is fine for me