Weird check for reserved IPs in mreg/api/permissions.py.

[terjekv]

In mreg/api/permissions.py we have the following construct:

    def has_destroy_permission(self, request, view, validated_serializer):
        import mreg.api.v1.views

        if user_is_superuser(request.user):
            return True
        obj = view.get_object()
        if isinstance(view, mreg.api.v1.views.HostDetail):
            pass
        elif hasattr(obj, 'host'):
            obj = obj.host
        else:
            raise exceptions.PermissionDenied(f"Unhandled view: {view}")
        if _deny_superuser_only_names(name=obj.name, view=view, request=request):
            return False
        if hasattr(obj, 'ipaddress'):
            if _deny_reserved_ipaddress(obj.ipaddress, request):
                return False
        if user_is_adminuser(request.user):
            return True
        return self.has_obj_perm(request.user, obj)

Note particularly the branch if hasattr(obj, 'ipaddress'). This is weird. First off, we check if the view is HostDetail (thus, obj is a Host), and if not we set the object to be obj.host. Ie, obj will always be a Host. But, Hosts do not have an ipaddress field. What they do have is a related_name, ip_addresses from the Ipaddress model: https://github.com/unioslo/mreg/blob/master/mreg/models.py#L398

This leads me to wonder what the logic is supposed to be here. Are we supposed to deny deletion of the object if the host is using any addresses that are reserved? If so, we want:

        if hasattr(obj, 'ipaddresses'):
            for ip in obj.ipaddresses:
                if _deny_reserved_ipaddress(ip.ipaddress, request):
                    return False

And we could then test this as such in api/v1/tests/tests_permission.py in the class class NetGroupRegexPermissionTestCaseAsAdmin(NetGroupRegexPermissionTestCase). Testing in TestIsGrantedNetGroupRegexPermission will give us a 204 due to the overall access given to superusers in the has_destroy_permission itself.

    # Superusers get to zap hosts, so this is for admins and down only.
    def test_403_from_reserved_ip(self):
        network = Network.objects.create(network='172.16.0.0/30', reserved=3, description='Tiny network')
        host = Host.objects.create(name='testhost')
        ip_address = Ipaddress.objects.create(ipaddress='172.16.0.1', host=host)
        self.assertTrue(is_reserved_ip(ip_address.ipaddress))
        self.assert_get(f'/hosts/{host.name}')
        self.assert_delete_and_403(f'/hosts/{host.name}')
        network.delete()
        host.delete()
        ip_address.delete()

Can someone explain this to me? :)

[oyvindhagberg]

git blame shows a lot of different commits contributing to this code. I suspect each addition might not have taken into consideration the purpose of the whole thing. We can only guess, but I think you're right: I think the code was supposed to deny deletion of the object if the host is using any addresses that are reserved (except to members of the NETWORK_ADMIN_GROUP, see _deny_reserved_ipaddress).

I would write the test first, verify that it fails, and then modify the code until all tests pass.

[oyvindkolbu]

Can someone explain this to me? :)

The API permissions works on serialized objects, not pure model objects. As HostSerializer is a quite fat one, the ipaddresses are available. https://github.com/unioslo/mreg/blob/8952b7973c142d90b2c35e37b64c665810237347/mreg/api/v1/serializers.py#L193