Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Permission Codenames Not Necessarily Unique #146

Closed
bdlamprecht opened this issue Jul 16, 2019 · 4 comments · Fixed by #148
Closed

Permission Codenames Not Necessarily Unique #146

bdlamprecht opened this issue Jul 16, 2019 · 4 comments · Fixed by #148
Labels
bug This issue describes a confirmed bug. pr There is a PR targeting this issue.

Comments

@bdlamprecht
Copy link
Contributor

Current Behavior

Prior to v2.6 of NetBox, the initializer files named users.yml and groups.yml worked perfectly with all Django models. However, in v2.6, the stock taggit model was changed into a custom taggit model (not sure of the specifics as to why).

With this change, when trying to add the permissions programatically for anything related to tags, the container for netbox fails with the following error:

django.contrib.auth.models.MultipleObjectsReturned: get() returned more than one Permission -- it returned 2!

Issue #3345 was opened against the main NetBox project since where was a similar issue with interface-connections when v2.5 was getting ready to be released (see issue #2624 within the NetBox project).

In that new issue, @jeremystretch mentioned there the following:

Permission code names aren't necessarily unique on their own: only the set (content_type, codename) must be unique. NetBox doesn't currently have any duplicate model names, but this might not always be the case: #2199 for instance would result in two models named "Role." It's a good idea to get into the habit now of not assuming model names to be unique.

In short, the current code for adding permissions for users and groups in the startup_scripts directory need to be adjusted to reflect that guidance from the main developer of NetBox.

Expected Behavior

To have the permissions for tags be automatically applied correctly upon starting the netbox container instead of crashing with the above error.
...

Debug Information

The output of docker-compose version: N/A
The output of docker version: N/A
The output of git rev-parse HEAD: N/A
The command you used to start the project: N/A

The output of docker-compose logs netbox:
N/A

The output of docker-compose logs nginx:
N/A

@LBegnaud
Copy link
Contributor

LBegnaud commented Jul 26, 2019

Unless we want to more fundamentally change these scripts, seems like this is the change that would need to be made:

          for permission_codename in user_details.get('permissions', []):
            permission = Permission.objects.get(codename=permission_codename)
            user.user_permissions.add(permission)

changed to

          for permission_codename in user_details.get('permissions', []):
            for permission in Permission.objects.filter(codename=permission_codename):
              user.user_permissions.add(permission)

and similarly, for groups

        for permission_codename in group_details.get('permissions', []):
          permission = Permission.objects.get(codename=permission_codename)
          group.permissions.add(permission)

changed to

        for permission_codename in group_details.get('permissions', []):
          for permission in Permission.objects.filter(codename=permission_codename):
            group.permissions.add(permission)

Edit: would be happy to do a PR if this is the route to take

@bdlamprecht
Copy link
Contributor Author

This looks great to me!

From my experience, I don't think very many people seem to use these initializers (though I could be horribly wrong 😄) and wouldn't care if they were altered like this.

It would be great if you could open a PR with these changes as I don't think @cimnine has much time any more since changing jobs this past spring.

@LBegnaud
Copy link
Contributor

In my case, I'm using them as an alternative to using AUTH_LDAP_MIRROR_GROUPS but definitely like the idea of being able to define the permissions as config files instead of through the UI.

Will get that PR done here shortly

@cimnine cimnine added bug This issue describes a confirmed bug. pr There is a PR targeting this issue. labels Jul 29, 2019
@cimnine
Copy link
Collaborator

cimnine commented Jul 29, 2019

Thank you guys for reporting this and also for the suggestions on how to fix this. The PR already looks promising.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue describes a confirmed bug. pr There is a PR targeting this issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants