why REMOTE_AUTH_SUPERUSERS tied to REMOTE_AUTH_GROUP_SYNC_ENABLED?

[cova-fe]

Hi, I activated GCP IaP jwt integration on my installation. I need to give some users SU/STAFF status, however jwt tokens does not provide any group or similar info, only email and ID. So, I used the REMOTE_AUTH_SUPERUSERS and REMOTE_AUTH_STAFF_USERS to achieve the result, but I had to hack the code because those two variables are applied only if REMOTE_AUTH_GROUP_SYNC_ENABLED is true. I don't see the reason to disregard REMOTE_AUTH_SUPERUSERS when remote auth does not provide group info, what I'm missing?

[mtinberg]

Maybe I misunderstand but those config variables should have the name of the remote group which should enable the superuser or staff privilege when the account is created in django and I'm not sure how it would work if there aren't remote groups being presented. Did you hack the source code to redefine these config variables as a list of userids to put in the staff and superuser group? If you don't have remote managed groups being presented by the webserver then you can just manually manage the groups, and the staff/superuser access, within the django admin portal or with a script that checks how the users are provisioned over the API and adds/removes the appropriate access, especially if you do have groups but they just aren't being presented by your authn system and you can query them in a script.

[cova-fe]

I'm not completely sure that this is the intended meaning for this variable, or at least what could be understood from documentation:

REMOTE_AUTH_SUPERUSERS

Default: [] (Empty list)

The list of users that get promoted to Superuser on Login. If user isn't present in list on next Login, the Role gets revoked. (Requires REMOTE_AUTH_ENABLED and REMOTE_AUTH_GROUP_SYNC_ENABLED )

So it seems that the required value is actually a list of users and not a group. Manually managing the groups will require some funky workaround, as using IAP auth every user will be logged in with standard privileges. Adding a script could be done, of course, like adding some code to manage in a more structured way the group creation (you can ask APIs to get more user info).
However, reading the documentation, it seems that REMOTE_AUTH_SUPERUSERS (and staff) could be a simple option, without having to go into script or additional features added.

[mtinberg]

That's what I get for not checking before I started to reply, sorry for the confusion.

https://github.com/netbox-community/netbox/blob/develop/netbox/netbox/authentication.py#L99

You've already been in there making changes, but it looks like it tries to either create the user or only modify the user once and the is_superuser/is_staff properties can be based on either the REMOTE_AUTH_SUPERUSERS list or membership in REMOTE_AUTH_SUPERUSERS_GROUPS which is checked after groups are added/removed from the user object, so there is one code path when modifying the user, and not a separate one that doesn't check for groups.

Maybe you could do something like this which just duplicates the relevant section (maybe we could make configure_group call configure_admin to remove the duplication)

        if self.user_can_authenticate(user):
            if settings.REMOTE_AUTH_GROUP_SYNC_ENABLED:
                if user is not None and not isinstance(user, AnonymousUser):
                    return self.configure_groups(user, remote_groups)
            elif settings.REMOTE_AUTH_SUPERUSERS or settings.REMOTE_AUTH_STAFF_USERS:
               if user is not None and not isinstance(user, AnonymousUser):
                    return self.configure_admin(user)
            else:
                return user
        else:
            return None

    def configure_admin(user):
        logger = logging.getLogger('netbox.authentication.RemoteUserBackend')
        user.is_superuser = self._is_superuser(user)
        logger.debug(f"User {user} is Superuser: {user.is_superuser}")
        logger.debug(
            f"User {user} should be Superuser: {self._is_superuser(user)}")

        user.is_staff = self._is_staff(user)
        logger.debug(f"User {user} is Staff: {user.is_staff}")
        logger.debug(f"User {user} should be Staff: {self._is_staff(user)}")
        user.save()

or gate the group assignment in configure_groups by whether remote groups are configured but I'm not sure that's possible, REMOTE_AUTH_GROUP_HEADER has a default value of HTTP_REMOTE_USER_GROUP so you can't just check to see if that is populated or not, I'm not sure you can tell the difference between no group environment variable in http or the user having no group membership, without maybe trying to reach into the web response itself since this is being handled by RemoteUserBackend and I'm not sure I want to dive in and see how that works.

[cova-fe]

This is how I hacked my way in the code, but I'm not really sure that it completely makes sense:
https://github.com/cova-fe/netbox/commit/59cac8bd6e2668824d526f1ff9c3d9bb7b62b08e

[mtinberg]

You wrote the required bits that are needed to make this work the way you want, I tried a little to match the style and control flow of the existing module. I think it's going to lead to some code duplication to avoid saving the user object twice (making two journal entries presumably) unless configure_admin puts and if statement around user.save() that checks in one way or another whether it was called from configure_groups or not (eg. whether REMOTE_AUTH_GROUP_SYNC_ENABLED is defined) which makes the behavior harder to follow.

I think at this point making a feature request in the issue tracker with the detailed description of the problem and the example solution, if the feature becomes accepted then you can make a pull request. If the feature doesn't get accepted then you'll need to rebase or patch your local fork to get this behavior.

https://github.com/netbox-community/netbox/blob/develop/CONTRIBUTING.md#feature-requests

[cova-fe]

I see your point, so I took you approach but making a couple of changes; I moved user.save away from configure_group and shuffled the logic around. Moreover, in your snippet the two settings are mutually exclusive, that would be a breaking change with the previous behavior. So the end result could be something like this:
https://github.com/cova-fe/netbox/commit/f3132d5b7499d442cc34fb862e5c3ff12ca6768c

Let me try to open an issue :)

[mtinberg]

You might also be able to remove some duplicate code by removing the is_superuser/is_staff setting from configure_groups and instead put a call to configure_admin after the call to configure_groups, so if group sync is enabled then call configure_groups and then configure_admin, or if static admins are defined then call configure_admins only or if neither are defined do nothing and return the user object unmodified.

still has an issue that neither function completes the transaction and expected the calling code to call save() so it's not done twice. otherwise you'd need to put the save back in the confgure_groups and configure_admin functions, do the early return and have duplicate is_superuser/is_staff code between the two functions, maybe with a comment pointing out that both cover some of the same ground, so the next time someone has to modify one, they can know to check the other.

or maybe not, we'll see what the lead developers say, I'm just spitballing possibilities