Add machine access control feature by Rikukar · Pull Request #622 · TampereHacklab/mulysa

Rikukar · 2025-12-01T05:36:43Z

Closes #592

Adds new role instructor
Adds a page where admins and instructors can choose which machines users have access to
Expands API
Adds a page for access logs which is accessible from profile

…feat/extending_api

…o feature/instructor-ui

Copilot

Pull request overview

This PR adds a comprehensive machine access control feature to the membership management system.

Purpose: Enable instructors and administrators to manage which machines members can access, and provide access logging functionality.

Key Changes:

Added is_instructor role and access_permissions M2M relationship to users
Implemented permission-based and service-based access control for devices
Created instructor tools page for managing member machine access
Added access logs page with pagination

Reviewed changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
users/models/custom_user.py	Added `is_instructor` field and `access_permissions` M2M for granular access control
api/models.py	Added `AccessPermission` model, device types, and permission/service-based access rules
api/views.py	Enhanced access logic to support both permission and service-based authorization
www/views.py	Added instructor tools, member search, permission update, and access logs views
www/decorators.py	Added `instructor_or_staff_member_required` decorator for authorization
www/templates/www/machine_access_control.html	Frontend for managing member machine permissions
www/templates/www/user_access_logs.html	Paginated access logs display
www/tests.py	Added tests for instructor role access control
api/tests.py	Added tests for permission-based and hybrid access scenarios

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-03T09:42:35Z

+@login_required
+@self_or_staff_member_required
+def useraccesslogs(request, id):
+    """
+    Show access logs related to a specific user.
+    """
+    user = get_object_or_404(CustomUser, id=id)
+    card_ids = list(user.nfccard_set.values_list("cardid", flat=True))
+    q = Q()
+    if user.phone:
+        q |= Q(method="phone", payload=user.phone)
+    if user.mxid:
+        q |= Q(method="mxid", payload=user.mxid)
+    if card_ids:
+        q |= Q(method="nfc", payload__in=card_ids)
+    q |= Q(nfccard__user=user) | Q(claimed_by=user)
+
+    logs = DeviceAccessLogEntry.objects.filter(q).order_by("-date")
+    paginator = Paginator(logs, 20)
+    page_number = request.GET.get("page")
+    page_obj = paginator.get_page(page_number)
+
+    return render(
+        request,
+        "www/user_access_logs.html",
+        {
+            "userdetails": user,
+            "page_obj": page_obj,
        },
    )


Missing test coverage for the useraccesslogs view which displays paginated access logs. Consider adding tests to verify pagination, query filtering, and access control for this new feature.

Copilot · 2025-12-03T09:42:35Z

+    allowed = list(user.access_permissions.values_list("id", flat=True))
+


The allowed variable is defined at line 260 but then redefined at line 267 with the same value. The first assignment should be removed as it's redundant.

Suggested change

allowed = list(user.access_permissions.values_list("id", flat=True))

Copilot · 2025-12-03T09:42:36Z

+            method: "POST",
+            headers: {
+                "Content-Type": "application/x-www-form-urlencoded",
+                "X-CSRFToken": "{{ csrf_token }}"


The CSRF token is embedded directly in the template without proper escaping context. While Django templates auto-escape by default, CSRF tokens in JavaScript should be retrieved from the CSRF cookie or a meta tag to avoid potential issues. Consider using getCookie('csrftoken') or adding a meta tag instead.

Copilot · 2025-12-03T09:42:36Z

+@login_required
+@instructor_or_staff_member_required
+def update_permission(request):
+    """
+    Updates a user’s machine access permissions when a checkbox is toggled in the UI.
+    """
+    user_id = request.POST.get("user_id")
+    device_id = request.POST.get("machine_id")
+    checked = request.POST.get("checked") == "true"
+
+    user = get_object_or_404(CustomUser, id=user_id)
+    device = get_object_or_404(AccessDevice, id=device_id)
+
+    # All permissions granted by this device
+    perms = device.allowed_permissions.all()
+
+    if checked:
+        user.access_permissions.add(*perms)
+    else:
+        user.access_permissions.remove(*perms)
+
+    return JsonResponse({"success": True})


The update_permission view lacks validation to ensure that the requesting user has permission to modify the target user's permissions. While the decorator checks for instructor/staff status, there's no check to prevent privilege escalation (e.g., instructors potentially granting permissions they don't have authority over). Consider adding authorization checks.

Copilot · 2025-12-03T09:42:36Z

+    fetch(`/www/machine-access-control/search/?member_number=${num}&last_name=${last}`)
+        .then(r => r.json())
+        .then(data => {
+            if (!data.found) {
+                document.getElementById("search_result").innerHTML =
+                    "<div class='alert alert-danger'>Member not found.</div>";
+                disableAll();
+                return;
+            }
+
+            selectedUserId = data.user_id;
+            document.getElementById("search_result").innerHTML =
+                `<div class='alert alert-success'>
+                    Member found: <b>${data.first_name} ${data.last_name}</b> – permissions (changes are saved automatically when you click a checkbox):
+                </div>`;
+
+            const userPermissions = data.allowed_permissions;
+
+            // Enable checkboxes and mark them according to user's permissions
+            document.querySelectorAll(".perm-check").forEach(c => {
+                c.disabled = false;
+                const devicePerms = c.dataset.permissions.split(",").filter(x => x.trim().length > 0).map(Number);
+                c.checked = devicePerms.every(p => userPermissions.includes(p));
+            });
+        });
+}
+
+// Send updates when checkbox changes
+document.querySelectorAll(".perm-check").forEach(cb => {
+    cb.addEventListener("change", function() {
+        if (!selectedUserId) return;
+
+        // Create POST data.
+        const formData = new URLSearchParams();
+        formData.append("user_id", selectedUserId);
+        formData.append("machine_id", this.dataset.machine);
+        formData.append("checked", this.checked);
+
+        // Send a POST request to the server.
+        fetch("/www/machine-access-control/update/", {
+            method: "POST",
+            headers: {
+                "Content-Type": "application/x-www-form-urlencoded",
+                "X-CSRFToken": "{{ csrf_token }}"
+            },
+            body: formData.toString()
+        });
+    });


The JavaScript doesn't handle errors from the fetch requests. If the search or update operations fail, the user won't receive any feedback. Add .catch() handlers to provide error messages to the user.

Copilot · 2025-12-03T09:42:37Z

+class TestInstructorAccessControl(TestCase):
+    def setUp(self):
+        self.user = get_user_model().objects.create_customuser(
+            first_name="Alice",
+            last_name="Member",
+            email="alice@example.com",
+            birthday=timezone.now(),
+            municipality="City",
+            nick="alice",
+            phone="+358111111",
+        )
+
+    def test_machine_access_control_requires_login(self):
+        url = reverse("machine-access-control")
+        response = self.client.get(url)
+        self.assertRedirects(response, f"/www/login/?next={url}")
+
+    def test_machine_access_control_denied_for_basic_user(self):
+        self.client.force_login(self.user)
+        url = reverse("machine-access-control")
+        response = self.client.get(url)
+        self.assertRedirects(response, f"/www/login/?next={url}")
+
+    def test_machine_access_control_allowed_for_instructor(self):
+        self.user.is_instructor = True
+        self.user.save()
+        self.client.force_login(self.user)
+        url = reverse("machine-access-control")
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, "Machine Access Control")
+
+    def test_machine_access_control_allowed_for_staff(self):
+        self.user.is_staff = True
+        self.user.save()
+        self.client.force_login(self.user)
+        url = reverse("machine-access-control")
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, "Machine Access Control")


Missing test coverage for the new search_member and update_permission views. While TestInstructorAccessControl tests the page access, it doesn't test the actual search and permission update functionality which are critical features of this PR.

Copilot · 2025-12-03T09:42:37Z

+
+    # Contains a list of ID numbers representing all access permissions the user has.
+    # The front-end can then use this list to mark the corresponding checkboxes as checked.
+    allowed = list(user.access_permissions.values_list("id", flat=True))


Variable allowed is not used.

Suggested change

allowed = list(user.access_permissions.values_list("id", flat=True))

Copilot · 2025-12-03T09:42:37Z

    ServiceSubscriptionCountFilter,
 )
 from .forms import CustomUserChangeForm, CustomUserCreationForm
+from api.models import AccessPermission


Import of 'AccessPermission' is not used.

Suggested change

from api.models import AccessPermission

…tch error handling

…e/instructor-ui

Rikukar and others added 14 commits November 16, 2025 15:07

Add instructor role

e73a9cd

Modified translations and emails to work with multiple devices

8d9a35a

Add new machine access control tab for admins and instructors

68f3221

Added AccessPermission model

e920a4e

Merge branch 'add-instructor-role' of github.com:Rikukar/mulysa into …

0f6b7b0

…feat/extending_api

Fixed merging issue in migrations

f91e615

Instructor UI

ebb3ef6

id removed from users/admin.py

765b4e6

Add access log page and show accessible machines

cdcd26a

Fixed api bug that allowed door to be opened with just AccessPermission

9f1715e

Merge branch 'feature/instructor-ui' of github.com:Rikukar/mulysa int…

31d393a

…o feature/instructor-ui

Instructor UI shows only education_required=True devices

0fec837

Add localizations

317d8db

Add and fix tests for machine access control page and instructor role

feaf0c1

Copilot AI review requested due to automatic review settings December 3, 2025 09:38

Copilot started reviewing on behalf of Rikukar December 3, 2025 09:39 View session

Copilot finished reviewing on behalf of Rikukar December 3, 2025 09:40

Copilot AI reviewed Dec 3, 2025

View reviewed changes

Rikukar and others added 10 commits December 6, 2025 13:53

Add tests for useraccesslogs

a803e22

Fixed email test when not allowed access

4659f55

More tests for api

5b18273

Fixed issue where variable was defined two times.

9675e53

Use meta tag to provide CSRF token to JavaScript

ff26a66

temp

acd37e4

Merge branch 'master' into feature/instructor-ui

624f4e8

Fix: remove redundant variable, secure CSRF, add authorization and fe…

a09b8a2

…tch error handling

Merge remote-tracking branch 'riku/feature/instructor-ui' into featur…

a7ee2f1

…e/instructor-ui

Add tests for search_member and update_permission functions

f787362

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add machine access control feature#622

Add machine access control feature#622
Rikukar wants to merge 24 commits intoTampereHacklab:masterfrom
Rikukar:feature/instructor-ui

Rikukar commented Dec 1, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		allowed = list(user.access_permissions.values_list("id", flat=True))

Conversation

Rikukar commented Dec 1, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants