diff --git a/backend/conftest.py b/backend/conftest.py index 5239a242..48eb1428 100644 --- a/backend/conftest.py +++ b/backend/conftest.py @@ -1,6 +1,8 @@ from allauth.account.models import EmailAddress import pytest -from graphene.test import Client +from graphene.test import Client as GrapheneClient +from typing import Generator +from django.test import Client as APIClient from case_study.models import CaseStudy from letter.models import LetterDescription @@ -12,7 +14,6 @@ from space.models import SpaceDescription from graphql_app.schema import schema - @pytest.fixture() def user_data(): return { @@ -40,7 +41,7 @@ def user(db, user_data): @pytest.fixture -def user_client(client, user): +def user_client(client, user) -> Generator[APIClient, None, None]: client.force_login(user) yield client client.logout() @@ -145,5 +146,5 @@ def case_study(db): @pytest.fixture() def graphql_client(): - client = Client(schema) + client = GrapheneClient(schema) return client diff --git a/backend/core/tests/test_contributors.py b/backend/core/tests/test_contributors.py index 3379bb24..b98a8dc1 100644 --- a/backend/core/tests/test_contributors.py +++ b/backend/core/tests/test_contributors.py @@ -30,8 +30,8 @@ def setUpTestData(cls) -> None: cls.episode = Episode.objects.create( name="Death Star destroyed", source_id=source.pk ) - cls.user_1 = User.objects.create(username="Yoda") - cls.user_2 = User.objects.create(username="Obi-Wan") + cls.user_1 = User.objects.create(username="Yoda", is_contributor=True) + cls.user_2 = User.objects.create(username="Obi-Wan", is_contributor=True) def test_single_contributor_one_contribution(self): self.client.force_login(self.user_1) diff --git a/backend/graphql_app/middleware.py b/backend/graphql_app/middleware.py index 6b19dc72..49595842 100644 --- a/backend/graphql_app/middleware.py +++ b/backend/graphql_app/middleware.py @@ -1,21 +1,21 @@ -import json -from django.conf import settings +from django.contrib.auth.models import AnonymousUser +from user.models import User +from typing import Union +from graphene import ResolveInfo -class GraphQLAuthMiddleware: - def resolve(self, next, root, info, **kwargs): - request = info.context +def has_mutation_permission(user: Union[AnonymousUser, User]) -> bool: + return user.is_superuser or getattr(user, "is_contributor", False) + - # Allow introspection queries to pass through in development mode. - if settings.DEBUG and self.is_introspection_query(request): - return next(root, info, **kwargs) +def is_mutation(info: ResolveInfo) -> bool: + return info.parent_type.name == "Mutation" - if info.context.user.is_authenticated: - return next(root, info, **kwargs) - raise Exception("User is not authenticated") +class GraphQLAuthMiddleware: + + def resolve(self, next, root, info: ResolveInfo, **kwargs): + if is_mutation(info) and not has_mutation_permission(info.context.user): + raise Exception("User is not authorised to make mutations") - def is_introspection_query(self, request): - data = json.loads(request.body) - query = data.get("query") # type: str - return query.startswith("query IntrospectionQuery") + return next(root, info, **kwargs) diff --git a/backend/graphql_app/tests.py b/backend/graphql_app/tests.py deleted file mode 100644 index 51e20877..00000000 --- a/backend/graphql_app/tests.py +++ /dev/null @@ -1,58 +0,0 @@ -from graphene_django.utils.testing import GraphQLTestCase - -from source.models import Source -from user.models import User - - -class GraphQLAuthMiddleWareTestCase(GraphQLTestCase): - """ - Tests for the GraphQLAuthMiddleware - """ - - GRAPHQL_URL = "/api/graphql" - - SOURCES_QUERY = """ - query GET_SOURCES { - sources { - id - } - } - """ - - @classmethod - def setUpTestData(cls) -> None: - cls.user = User.objects.create( - username="test", email="test@test.nl", password="test" - ) - - cls.source = Source.objects.create( - name="Test Source", - medieval_title="Test Medieval Title", - medieval_author="Test Medieval Author", - edition_title="Test Edition Title", - edition_author="Test Edition Author", - ) - - return super().setUpTestData() - - def test_middleware_raises_exception_when_user_is_not_authenticated(self): - response = self.query(self.SOURCES_QUERY) - - self.assertResponseHasErrors(response) - - content = response.json() - - self.assertEqual(content["errors"][0]["message"], "User is not authenticated") - - def test_middleware_does_not_raise_exception_when_user_is_authenticated(self): - self.client.force_login(self.user) - - response = self.query(self.SOURCES_QUERY) - - self.assertResponseNoErrors(response) - - content = response.json() - - # The first Source in the list is the default MISSING_SOURCE. - self.assertEqual(len(content["data"]["sources"]), 2) - self.assertEqual(content["data"]["sources"][1]["id"], str(self.source.pk)) diff --git a/backend/graphql_app/tests/test_middleware.py b/backend/graphql_app/tests/test_middleware.py new file mode 100644 index 00000000..63566d37 --- /dev/null +++ b/backend/graphql_app/tests/test_middleware.py @@ -0,0 +1,63 @@ +from rest_framework.status import is_success +import json + +QUERY_EXAMPLE = """ + query TestQuery { + sources { + id + } + } +""" + +MUTATION_EXAMPLE = """ + mutation TestMutation { + createAgent(agentData: {name: "test", source: "1"}) { + ok + } + } +""" + + +def test_middleware_passes_queries(client, db): + response = client.post( + "/api/graphql", + { + "operationName": "TestQuery", + "query": QUERY_EXAMPLE, + }, + ) + + assert is_success(response.status_code) + data = json.loads(response.content) + assert not "errors" in data + + +def test_middleware_blocks_mutation_from_unauthorised_user(user_client, source): + response = user_client.post( + "/api/graphql", + { + "operationName": "TestMutation", + "query": MUTATION_EXAMPLE, + }, + ) + + assert is_success(response.status_code) + data = json.loads(response.content) + assert data["errors"][0]["message"] == "User is not authorised to make mutations" + + +def test_middleware_passes_mutation_from_authorised_user(user, user_client, source): + user.is_contributor = True + user.save() + + response = user_client.post( + "/api/graphql", + { + "operationName": "TestMutation", + "query": MUTATION_EXAMPLE, + }, + ) + + assert is_success(response.status_code) + data = json.loads(response.content) + assert not "errors" in data diff --git a/backend/user/admin.py b/backend/user/admin.py index 26d25b24..37a60e2d 100644 --- a/backend/user/admin.py +++ b/backend/user/admin.py @@ -4,4 +4,5 @@ @admin.register(models.User) class UserAdmin(auth_admin.UserAdmin): - pass + fieldsets = auth_admin.UserAdmin.fieldsets + fieldsets[2][1]["fields"] = list(fieldsets[2][1]["fields"]) + ["is_contributor"] diff --git a/backend/user/migrations/0004_user_is_contributor.py b/backend/user/migrations/0004_user_is_contributor.py new file mode 100644 index 00000000..7b8cc7fa --- /dev/null +++ b/backend/user/migrations/0004_user_is_contributor.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.7 on 2024-10-01 13:24 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('user', '0003_sitedomain'), + ] + + operations = [ + migrations.AddField( + model_name='user', + name='is_contributor', + field=models.BooleanField(default=False, help_text='Whether this user is a contributor on the project; this enables them to enter or edit research data.'), + ), + ] diff --git a/backend/user/models.py b/backend/user/models.py index 28e3ccd1..4d513bba 100644 --- a/backend/user/models.py +++ b/backend/user/models.py @@ -13,3 +13,9 @@ class User(django_auth_models.AbstractUser): class Meta: db_table = "auth_user" + + is_contributor = models.BooleanField( + default=False, + help_text="Whether this user is a contributor on the project; this enables them " + "to enter or edit research data.", + ) diff --git a/backend/user/serializers.py b/backend/user/serializers.py index 4886ee4c..c7ec4a98 100644 --- a/backend/user/serializers.py +++ b/backend/user/serializers.py @@ -13,5 +13,6 @@ class Meta(UserDetailsSerializer.Meta): "first_name", "last_name", "is_staff", + "is_contributor", ) - read_only_fields = ["is_staff", "id", "email"] + read_only_fields = ["is_staff", "id", "email", "is_contributor"] diff --git a/backend/user/tests/test_user_views.py b/backend/user/tests/test_user_views.py index babe923b..fba8584b 100644 --- a/backend/user/tests/test_user_views.py +++ b/backend/user/tests/test_user_views.py @@ -11,6 +11,7 @@ def test_user_details(user_client, user_data): "first_name": user_data["first_name"], "last_name": user_data["last_name"], "is_staff": False, + "is_contributor": False, } @@ -28,10 +29,11 @@ def test_user_updates(user_client, user_data): assert response.status_code == 200 assert details()["username"] == "NewName" - # is_staff is readonly, so nothing should happen + # is_staff and is_contributor are readonly, so nothing should happen response = user_client.patch( route, - {"is_staff": True}, + {"is_staff": True, "is_contributor": True}, content_type="application/json", ) assert not details()["is_staff"] + assert not details()["is_contributor"] diff --git a/frontend/src/app/app.module.ts b/frontend/src/app/app.module.ts index 0273a042..80bc362b 100644 --- a/frontend/src/app/app.module.ts +++ b/frontend/src/app/app.module.ts @@ -1,7 +1,7 @@ import { NgModule } from '@angular/core'; import { AppComponent } from './app.component'; -import { AppRoutingModule } from './app-routing.module'; +import { AppRoutingModule } from './routing/app-routing.module'; import { HomeComponent } from './home/home.component'; import { SharedModule } from './shared/shared.module'; diff --git a/frontend/src/app/core/menu/menu.component.html b/frontend/src/app/core/menu/menu.component.html index 90dd5d68..cf1e6b27 100644 --- a/frontend/src/app/core/menu/menu.component.html +++ b/frontend/src/app/core/menu/menu.component.html @@ -19,7 +19,7 @@ Home -
  • +
  • Data entry diff --git a/frontend/src/app/core/menu/menu.component.ts b/frontend/src/app/core/menu/menu.component.ts index ced28885..28e710d6 100644 --- a/frontend/src/app/core/menu/menu.component.ts +++ b/frontend/src/app/core/menu/menu.component.ts @@ -1,5 +1,6 @@ import { Component } from '@angular/core'; import { AuthService } from '@services/auth.service'; +import { map } from 'rxjs'; @Component({ selector: 'lc-menu', @@ -8,7 +9,9 @@ import { AuthService } from '@services/auth.service'; }) export class MenuComponent { public burgerActive = false; - public isAuthenticated$ = this.authService.isAuthenticated$; + public isContributor$ = this.authService.currentUser$.pipe( + map(user => user?.isContributor || false) + ); constructor(private authService: AuthService) {} diff --git a/frontend/src/app/core/menu/user-menu/user-menu.component.spec.ts b/frontend/src/app/core/menu/user-menu/user-menu.component.spec.ts index 6874516c..3a413225 100644 --- a/frontend/src/app/core/menu/user-menu/user-menu.component.spec.ts +++ b/frontend/src/app/core/menu/user-menu/user-menu.component.spec.ts @@ -12,7 +12,8 @@ const fakeUserResponse: UserResponse = { email: "frodo@shire.me", first_name: "Frodo", last_name: "Baggins", - is_staff: false + is_staff: false, + is_contributor: true, } const fakeAdminResponse: UserResponse = { @@ -21,7 +22,8 @@ const fakeAdminResponse: UserResponse = { email: "gandalf@istari.me", first_name: "Gandalf", last_name: "The Grey", - is_staff: true + is_staff: true, + is_contributor: false, } diff --git a/frontend/src/app/app-routing.module.ts b/frontend/src/app/routing/app-routing.module.ts similarity index 100% rename from frontend/src/app/app-routing.module.ts rename to frontend/src/app/routing/app-routing.module.ts diff --git a/frontend/src/app/routing/contributor.guard.ts b/frontend/src/app/routing/contributor.guard.ts new file mode 100644 index 00000000..6581687b --- /dev/null +++ b/frontend/src/app/routing/contributor.guard.ts @@ -0,0 +1,30 @@ +import { inject } from "@angular/core"; +import { CanActivateFn, Router } from "@angular/router"; +import { AuthService } from "@services/auth.service"; +import { ToastService } from "@services/toast.service"; +import { filter, map } from "rxjs"; + + +export const ContributorGuard: CanActivateFn = () => { + const authService = inject(AuthService); + const toastService = inject(ToastService); + const router = inject(Router); + + return authService.currentUser$.pipe( + filter((user) => user !== undefined), + map((user) => { + if (user?.isContributor) { + return true; + } else { + let body = 'You do not have permission to view this page.'; + if (!user) { body += ' Do you need to sign in?'; } + toastService.show({ + type: 'danger', + header: 'Not authorised', + body, + }); + return router.createUrlTree(['/']); + } + }), + ); +}; diff --git a/frontend/src/app/routing/contributor.guards.spec.ts b/frontend/src/app/routing/contributor.guards.spec.ts new file mode 100644 index 00000000..acf93dcf --- /dev/null +++ b/frontend/src/app/routing/contributor.guards.spec.ts @@ -0,0 +1,18 @@ +import { TestBed } from "@angular/core/testing"; +import { CanActivateFn } from "@angular/router"; +import { ContributorGuard } from "./contributor.guard"; + + + +describe("LoggedOnGuard", () => { + const executeGuard: CanActivateFn = (...guardParameters) => + TestBed.runInInjectionContext(() => ContributorGuard(...guardParameters)); + + beforeEach(() => { + TestBed.configureTestingModule({}); + }); + + it("should be created", () => { + expect(executeGuard).toBeTruthy(); + }); +}); diff --git a/frontend/src/app/shared/logged-on.guard.spec.ts b/frontend/src/app/routing/logged-on.guard.spec.ts similarity index 100% rename from frontend/src/app/shared/logged-on.guard.spec.ts rename to frontend/src/app/routing/logged-on.guard.spec.ts diff --git a/frontend/src/app/shared/logged-on.guard.ts b/frontend/src/app/routing/logged-on.guard.ts similarity index 56% rename from frontend/src/app/shared/logged-on.guard.ts rename to frontend/src/app/routing/logged-on.guard.ts index 2f04b67f..d168d552 100644 --- a/frontend/src/app/shared/logged-on.guard.ts +++ b/frontend/src/app/routing/logged-on.guard.ts @@ -1,18 +1,26 @@ import { inject } from "@angular/core"; import { ActivatedRouteSnapshot, CanActivateFn, Router } from "@angular/router"; import { AuthService } from "@services/auth.service"; +import { ToastService } from "@services/toast.service"; import { filter, map } from "rxjs"; export const LoggedOnGuard: CanActivateFn = (route: ActivatedRouteSnapshot) => { const authService = inject(AuthService); + const toastService = inject(ToastService); const router = inject(Router); return authService.currentUser$.pipe( filter((user) => user !== undefined), map((user) => { if (user === null) { - router.navigate(["/login"], { queryParams: { next: route.url } }); - return false; + toastService.show({ + type: 'danger', + header: 'Not signed in', + body: 'You must be signed in to view this page.' + }); + return router.createUrlTree(['/login'], { + queryParams: { next: route.url } + }); } return true; }) diff --git a/frontend/src/app/routes.ts b/frontend/src/app/routing/routes.ts similarity index 65% rename from frontend/src/app/routes.ts rename to frontend/src/app/routing/routes.ts index b378bb9d..8bf084b1 100644 --- a/frontend/src/app/routes.ts +++ b/frontend/src/app/routing/routes.ts @@ -1,24 +1,25 @@ import { Routes } from '@angular/router'; -import { HomeComponent } from './home/home.component'; -import { LoginComponent } from './user/login/login.component'; -import { VerifyEmailComponent } from './user/verify-email/verify-email.component'; -import { RegisterComponent } from './user/register/register.component'; -import { PasswordForgottenComponent } from './user/password-forgotten/password-forgotten.component'; -import { ResetPasswordComponent } from './user/reset-password/reset-password.component'; -import { UserSettingsComponent } from './user/user-settings/user-settings.component'; -import { LoggedOnGuard } from '@shared/logged-on.guard'; -import { SourcesComponent } from './data-entry/sources/sources.component'; -import { LocationFormComponent } from './data-entry/location-form/location-form.component'; -import { GiftFormComponent } from './data-entry/gift-form/gift-form.component'; -import { LetterFormComponent } from './data-entry/letter-form/letter-form.component'; -import { AgentFormComponent } from './data-entry/agent-form/agent-form.component'; -import { SourceComponent } from './data-entry/source/source.component'; +import { HomeComponent } from '../home/home.component'; +import { LoginComponent } from '../user/login/login.component'; +import { VerifyEmailComponent } from '../user/verify-email/verify-email.component'; +import { RegisterComponent } from '../user/register/register.component'; +import { PasswordForgottenComponent } from '../user/password-forgotten/password-forgotten.component'; +import { ResetPasswordComponent } from '../user/reset-password/reset-password.component'; +import { UserSettingsComponent } from '../user/user-settings/user-settings.component'; +import { LoggedOnGuard } from './logged-on.guard'; +import { SourcesComponent } from '../data-entry/sources/sources.component'; +import { LocationFormComponent } from '../data-entry/location-form/location-form.component'; +import { GiftFormComponent } from '../data-entry/gift-form/gift-form.component'; +import { LetterFormComponent } from '../data-entry/letter-form/letter-form.component'; +import { AgentFormComponent } from '../data-entry/agent-form/agent-form.component'; +import { SourceComponent } from '../data-entry/source/source.component'; import { agentFormTitleResolver, giftFormTitleResolver, letterFormTitleResolver, pageTitle, SITE_NAME, sourceFormTitleResolver, spaceFormTitleResolver -} from './titles'; -import { EpisodeFormComponent } from './data-entry/episode-form/episode-form.component'; +} from '../titles'; +import { EpisodeFormComponent } from '../data-entry/episode-form/episode-form.component'; +import { ContributorGuard } from './contributor.guard'; const routes: Routes = [ @@ -55,11 +56,12 @@ const routes: Routes = [ { path: 'user-settings', title: pageTitle('Settings'), + canActivate: [LoggedOnGuard], component: UserSettingsComponent }, { path: 'data-entry', - canActivate: [LoggedOnGuard], + canActivate: [ContributorGuard], children: [ { path: 'agents/:id', diff --git a/frontend/src/app/user/models/user.ts b/frontend/src/app/user/models/user.ts index 567eac22..c69a311f 100644 --- a/frontend/src/app/user/models/user.ts +++ b/frontend/src/app/user/models/user.ts @@ -5,6 +5,7 @@ export interface UserResponse { first_name: string; last_name: string; is_staff: boolean; + is_contributor: boolean; } export class User { @@ -14,8 +15,9 @@ export class User { public email: string, public firstName: string, public lastName: string, - public isStaff: boolean - ) {} + public isStaff: boolean, + public isContributor: boolean, + ) { } } export interface UserRegistration { diff --git a/frontend/src/app/user/user-settings/user-settings.component.spec.ts b/frontend/src/app/user/user-settings/user-settings.component.spec.ts index 1844a71a..adf2a995 100644 --- a/frontend/src/app/user/user-settings/user-settings.component.spec.ts +++ b/frontend/src/app/user/user-settings/user-settings.component.spec.ts @@ -16,7 +16,8 @@ const fakeUser: User = { firstName: 'Frodo', lastName: 'Baggins', username: 'frodo', - isStaff: false + isStaff: false, + isContributor: true, } @Injectable({ providedIn: 'root' }) diff --git a/frontend/src/app/user/utils.spec.ts b/frontend/src/app/user/utils.spec.ts index 34649e6b..271cbcbc 100644 --- a/frontend/src/app/user/utils.spec.ts +++ b/frontend/src/app/user/utils.spec.ts @@ -39,6 +39,7 @@ describe("User utils", () => { first_name: "Test", last_name: "User", is_staff: true, + is_contributor: true, }; const user = parseUserData(result); expect(user).toBeInstanceOf(User); @@ -48,6 +49,7 @@ describe("User utils", () => { expect(user?.firstName).toBe("Test"); expect(user?.lastName).toBe("User"); expect(user?.isStaff).toBe(true); + expect(user?.isContributor).toBe(true); }); }); @@ -59,7 +61,6 @@ describe("User utils", () => { email: "test@example.com", firstName: "Test", lastName: "User", - isStaff: true, }; const encoded = encodeUserData(data); expect(encoded).toEqual({ @@ -68,9 +69,20 @@ describe("User utils", () => { email: "test@example.com", first_name: "Test", last_name: "User", - is_staff: true, }); }); + + it('should omit is_staff and is_contributor', () => { + const data: Partial = { + id: 1, + isStaff: true, + isContributor: true, + }; + const encoded = encodeUserData(data); + expect(encoded).toEqual({ + id: 1, + }); + }) }); describe("setErrors", () => { diff --git a/frontend/src/app/user/utils.ts b/frontend/src/app/user/utils.ts index 7d02b5c6..a8a98e6f 100644 --- a/frontend/src/app/user/utils.ts +++ b/frontend/src/app/user/utils.ts @@ -21,6 +21,7 @@ export const parseUserData = (result: UserResponse | null): User | null => { result.first_name, result.last_name, result.is_staff, + result.is_contributor, ); }; @@ -39,7 +40,6 @@ export const encodeUserData = (data: Partial): Partial => { email: data.email, first_name: data.firstName, last_name: data.lastName, - is_staff: data.isStaff, }; return _.omit(encoded, _.isUndefined); };