From 6326dbb22d556dac7cf53a70f559c55155eccc08 Mon Sep 17 00:00:00 2001 From: Derek Ditch Date: Tue, 25 Aug 2026 11:48:39 -0500 Subject: [PATCH] fix: import NetBox's own users.models.Group, not django.contrib.auth's NetBox 4.x replaces Django's stock auth.Group entirely with its own users.models.Group (separate model/table; User.groups points there). Importing the wrong one produced instances NetBox's own User.groups field can't resolve a pk from, surfacing at real login as: TypeError: Field 'id' expected a number but got . Also drops the is_staff assignment -- NetBox's User model has no such field, so it was a silent no-op. Test suite now runs against a small testapp/users stand-in that mirrors NetBox's actual Group/User shape (not django.contrib.auth's), so this exact bug is caught going forward. --- README.md | 4 +-- docs/configuration.md | 4 +-- docs/index.md | 11 +++++- pyproject.toml | 13 +++++-- src/netbox_oidc_group_sync/__version__.py | 2 +- src/netbox_oidc_group_sync/pipeline.py | 9 +++-- testapp/users/__init__.py | 0 testapp/users/models.py | 43 +++++++++++++++++++++++ tests/django_settings.py | 3 ++ tests/test_pipeline.py | 6 +--- 10 files changed, 80 insertions(+), 15 deletions(-) create mode 100644 testapp/users/__init__.py create mode 100644 testapp/users/models.py diff --git a/README.md b/README.md index be09210..e3224b9 100644 --- a/README.md +++ b/README.md @@ -1,7 +1,7 @@ # netbox-oidc-group-sync A [python-social-auth](https://github.com/python-social-auth/social-core) pipeline step that syncs NetBox -Django groups and `is_superuser`/`is_staff` status from an OIDC `groups` claim. +Django groups and `is_superuser` status from an OIDC `groups` claim. ## Why this exists @@ -64,7 +64,7 @@ already set: | `REMOTE_AUTH_GROUP_SYNC_ENABLED` | Master on/off switch. `sync_groups` no-ops entirely when falsy. | | `REMOTE_AUTH_GROUP_HEADER` | The key to look up in the OIDC claims/userinfo `response` dict for the user's group list. Named for its original HTTP-header use case; repurposed here as a claim key, which doesn't conflict with anything since it has no effect on social-auth logins upstream. | | `REMOTE_AUTH_AUTO_CREATE_GROUPS` | Create a Django `Group` for a claimed group name that doesn't exist yet, instead of skipping it with a logged error. | -| `REMOTE_AUTH_SUPERUSER_GROUPS` | Group names that grant `is_superuser`/`is_staff` when present in the user's synced claim groups. | +| `REMOTE_AUTH_SUPERUSER_GROUPS` | Group names that grant `is_superuser` when present in the user's synced claim groups. | | `REMOTE_AUTH_SUPERUSERS` | Usernames that are always superusers, regardless of group membership. | Group membership is a **full sync**, not additive: a user's Django groups are set to exactly what the claim diff --git a/docs/configuration.md b/docs/configuration.md index 6093ccf..e340da9 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -8,7 +8,7 @@ would set -- so there's nothing new to learn if you've configured NetBox's remot | `REMOTE_AUTH_GROUP_SYNC_ENABLED` | `bool` | Master on/off switch. `sync_groups` no-ops entirely when falsy -- no group changes, no superuser changes. | | `REMOTE_AUTH_GROUP_HEADER` | `str` | The key to look up in the OIDC claims/userinfo `response` dict for the user's group list. Named for its original HTTP-header use case in `RemoteUserBackend`; repurposed here as a claim key. This doesn't conflict with anything, since the setting has no effect on social-auth logins upstream. | | `REMOTE_AUTH_AUTO_CREATE_GROUPS` | `bool` | When `true`, a claimed group name that doesn't exist yet is created. When `false`, it's skipped with a logged error and the user isn't added to it. | -| `REMOTE_AUTH_SUPERUSER_GROUPS` | `list[str]` | Group names that grant `is_superuser` and `is_staff` when present among the user's synced claim groups. | +| `REMOTE_AUTH_SUPERUSER_GROUPS` | `list[str]` | Group names that grant `is_superuser` when present among the user's synced claim groups. | | `REMOTE_AUTH_SUPERUSERS` | `list[str]` | Usernames that are always superusers, regardless of group membership. | ## Semantics @@ -38,5 +38,5 @@ REMOTE_AUTH_SUPERUSER_GROUPS = ["netbox-admins"] A user in Authentik's `netbox-admins` group logs in, `sync_groups` runs as part of [the pipeline](installation.md), NetBox creates a Django `netbox-admins` group if it doesn't already exist, -adds the user to it, and sets `is_superuser = True` / `is_staff = True`. A user removed from that group in +adds the user to it, and sets `is_superuser = True`. A user removed from that group in Authentik loses superuser status the next time they log in. diff --git a/docs/index.md b/docs/index.md index e9f7f11..3fc0c69 100644 --- a/docs/index.md +++ b/docs/index.md @@ -1,7 +1,7 @@ # netbox-oidc-group-sync A [python-social-auth](https://github.com/python-social-auth/social-core) pipeline step that syncs NetBox -Django groups and `is_superuser`/`is_staff` status from an OIDC `groups` claim. +Django groups and `is_superuser` status from an OIDC `groups` claim. ## The gap @@ -33,3 +33,12 @@ configuration surface, no fork of NetBox itself. See [Installation](installation.md) to get it running, and [Configuration](configuration.md) for the settings it reads. + +## A NetBox-specific gotcha + +NetBox 4.x doesn't use Django's stock `django.contrib.auth.models.Group` -- it defines its own +`users.models.Group`, a completely separate model/table, and `User.groups` points there instead. +`sync_groups` imports from `users.models`, not `django.contrib.auth.models`; getting this wrong produces a +`TypeError: Field 'id' expected a number but got ` at login time, since Django's M2M machinery +can't resolve a pk from an instance of the wrong model. Also unlike Django's stock `auth.Group`-based +`AbstractUser`, NetBox's `User` model has no `is_staff` field at all -- only `is_superuser`. diff --git a/pyproject.toml b/pyproject.toml index 157f0a6..6dbe600 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -75,8 +75,9 @@ typecheck = "mypy src" [tool.pytest.ini_options] DJANGO_SETTINGS_MODULE = "tests.django_settings" -pythonpath = ["."] +pythonpath = [".", "testapp"] testpaths = ["tests"] +addopts = "--no-migrations" # --------------------------------------------------------------------------- # Coverage @@ -110,6 +111,13 @@ warn_unreachable = true module = "django.*" ignore_missing_imports = true +[[tool.mypy.overrides]] +# users.models is NetBox's own internal app (its custom Group/User models, +# not Django's stock auth ones) -- only resolvable inside a real NetBox +# installation, so no stubs exist for it standalone. +module = "users.*" +ignore_missing_imports = true + # --------------------------------------------------------------------------- # Ruff # --------------------------------------------------------------------------- @@ -141,7 +149,8 @@ ignore = [ convention = "google" [tool.ruff.lint.per-file-ignores] -"tests/**" = ["D"] # no docstring requirements in tests +"tests/**" = ["D"] # no docstring requirements in tests +"testapp/**" = ["D"] # test-only stand-in for NetBox's users app, not public API [tool.ruff.lint.isort] known-first-party = ["netbox_oidc_group_sync"] diff --git a/src/netbox_oidc_group_sync/__version__.py b/src/netbox_oidc_group_sync/__version__.py index f1b630c..09220b5 100644 --- a/src/netbox_oidc_group_sync/__version__.py +++ b/src/netbox_oidc_group_sync/__version__.py @@ -1,3 +1,3 @@ """Single-source version, read dynamically by hatchling's build backend.""" -__version__ = "0.1.0" +__version__ = "0.1.1" diff --git a/src/netbox_oidc_group_sync/pipeline.py b/src/netbox_oidc_group_sync/pipeline.py index 6b4b5a8..33b9715 100644 --- a/src/netbox_oidc_group_sync/pipeline.py +++ b/src/netbox_oidc_group_sync/pipeline.py @@ -32,7 +32,13 @@ from typing import Any from django.conf import settings -from django.contrib.auth.models import Group + +# NetBox 4.x doesn't use Django's stock auth.Group at all -- it defines its own +# users.models.Group (a separate model/table; User.groups points there, not at +# django.contrib.auth.models.Group). Importing the wrong one produces instances +# NetBox's own User.groups field doesn't recognize (Django raises "Field 'id' +# expected a number but got " when you try to .set() them). +from users.models import Group logger = logging.getLogger("netbox_oidc_group_sync.pipeline") @@ -99,5 +105,4 @@ def sync_groups( user.is_superuser = user.username in superusers or bool( {group.name for group in group_list} & superuser_groups ) - user.is_staff = user.is_superuser user.save() diff --git a/testapp/users/__init__.py b/testapp/users/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/testapp/users/models.py b/testapp/users/models.py new file mode 100644 index 0000000..87170f9 --- /dev/null +++ b/testapp/users/models.py @@ -0,0 +1,43 @@ +"""A minimal stand-in for NetBox's real `users` app, used only in tests. + +Mirrors the parts of NetBox 4.x's actual `users.models.Group`/`users.models.User` +shape that `netbox_oidc_group_sync.pipeline.sync_groups` depends on: `Group` is +a standalone model (NOT `django.contrib.auth.models.Group`), and `User.groups` +is a `ManyToManyField` pointing at it. Getting this wrong in the real package +is exactly the bug this test app exists to catch. +""" + +from typing import ClassVar + +from django.contrib.auth.base_user import AbstractBaseUser +from django.contrib.auth.models import PermissionsMixin +from django.db import models + + +class Group(models.Model): + name = models.CharField(max_length=150, unique=True) + + class Meta: + app_label = "users" + + def __str__(self) -> str: + return self.name + + +class User(AbstractBaseUser, PermissionsMixin): + username = models.CharField(max_length=150, unique=True) + email = models.EmailField(blank=True) + is_active = models.BooleanField(default=True) + + groups = models.ManyToManyField( + to="users.Group", + blank=True, + related_name="users", + related_query_name="user", + ) + + USERNAME_FIELD = "username" + REQUIRED_FIELDS: ClassVar[list[str]] = [] + + class Meta: + app_label = "users" diff --git a/tests/django_settings.py b/tests/django_settings.py index 4a85eca..fefe6d6 100644 --- a/tests/django_settings.py +++ b/tests/django_settings.py @@ -3,8 +3,11 @@ INSTALLED_APPS = [ "django.contrib.auth", "django.contrib.contenttypes", + "users", ] +AUTH_USER_MODEL = "users.User" + DATABASES = { "default": { "ENGINE": "django.db.backends.sqlite3", diff --git a/tests/test_pipeline.py b/tests/test_pipeline.py index b207cbf..4f116ff 100644 --- a/tests/test_pipeline.py +++ b/tests/test_pipeline.py @@ -1,6 +1,6 @@ import pytest -from django.contrib.auth.models import Group, User from pytest_django.fixtures import Settings +from users.models import Group, User from netbox_oidc_group_sync.pipeline import sync_groups @@ -41,7 +41,6 @@ def test_assigns_existing_groups(user: User) -> None: user.refresh_from_db() assert [g.name for g in user.groups.all()] == ["netbox-users"] assert user.is_superuser is False - assert user.is_staff is False @pytest.mark.django_db @@ -76,7 +75,6 @@ def test_grants_superuser_for_matching_group(settings: Settings, user: User) -> user.refresh_from_db() assert user.is_superuser is True - assert user.is_staff is True @pytest.mark.django_db @@ -93,7 +91,6 @@ def test_grants_superuser_for_username_allowlist(settings: Settings, user: User) @pytest.mark.django_db def test_revokes_superuser_when_no_longer_in_group(settings: Settings, user: User) -> None: user.is_superuser = True - user.is_staff = True user.save() settings.REMOTE_AUTH_SUPERUSER_GROUPS = ["netbox-admins"] @@ -101,7 +98,6 @@ def test_revokes_superuser_when_no_longer_in_group(settings: Settings, user: Use user.refresh_from_db() assert user.is_superuser is False - assert user.is_staff is False @pytest.mark.django_db