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

Super users should not be granted permission by default #61

Closed
filipeximenes opened this issue Feb 17, 2017 · 9 comments
Closed

Super users should not be granted permission by default #61

filipeximenes opened this issue Feb 17, 2017 · 9 comments

Comments

@cinnabot
Copy link

Hi, I have some suggestions to help solving this issue!

Here is a list of similar issues:

  1. Better document permission handling

  2. Exclusive use of Groups

  3. Revoke permission function

Also, below are some developers who made PRs that close similar issues:

Please let me know if this was useful:

Cheers,
Cinnabot.

PS: Liked my suggestions? Install me in your repository! Visit: getcinnamon.io.

@kavdev
Copy link
Contributor

kavdev commented Mar 2, 2017

^ Interesting bot.

@filipeximenes: Are you thinking a settings variable? It's fairly useful to not have to add all roles to a superuser, but I can see instances (for testing, mainly) where being able to turn this functionality off is useful.

@filipeximenes
Copy link
Contributor Author

@kavdev Yes, that's my plan. I still not sure if granting all permissions to admin should be the default behavior or if it should require changing the settings variable. My personal opinion now a days is that admins should not have access by default. Just because it is confusing and error prone. No users should have default access to any permissions unless they are explicitly assigned. At the same time I'm leaning twards keeping the current behavior the default just for the sake of not breaking the API.

When you say that it would be usefull 'for testing', do you mean automated testing or manually tweaking the interface? If you are refering to the later we are probably dealing with the same situation.

Thoughts?

@kavdev
Copy link
Contributor

kavdev commented Mar 2, 2017

@filipeximenes Either way works for me. My thought is that if you set someone as a superuser, most times that user is a developer and has a way to gain access to everything regardless.

I mean manually tweaking the interface.

Superuser default pros: You don't need to manually assign roles to the superuser in order to view sections of the app that are role-restricted. If a user is having an issue, you can jump right in and take a look at the page they're on. Also, you don't need to switch roles as a developer to check out the new ui you just built.

Superuser default cons: This breaks down if the interface is changed based on your role and there is no specific interface layout for a superuser. Things get confusing because there's no way to see exactly what a user in a particular role sees.

I can see the benefits of either choice; on one hand, a superuser by definition should have access to everything. On the other hand, in some situations that user might also need to check something that switches based on role. That said, you could use a package like django-hijack to help with the latter case.

I think a settings variable is the way to go. Default access as a superuser seems like a better approach solely because it will work fine in the majority cases. In the cases that that behavior needs to change, the dev can just switch the settings flag. Also, not breaking the API is always nice.

@powderflask
Copy link
Contributor

Suggestion: if you are adding a settings option to enable / disable all permissions for super-user, do the same for staff users. In my use-case (and I suspect many), users marked as staff should have all permissions. It is a bit of a headache keeping that up-to-date with a separate role for staff.

For backwards compatibility, all-permissions for superuser should default to True, for staff should default to False.

Happy to contribute code i that helps.

@iurisilvio
Copy link
Contributor

Anyone working on it?

It is a real problem here, I can create a setting to change this behaviour.

@kavdev
Copy link
Contributor

kavdev commented May 27, 2020

@iurisilvio I think that would be awesome. I'm no longer working on the project that uses this library, but I'll be happy to give it a review. I might be integrating this into a new project soon.

@iurisilvio
Copy link
Contributor

iurisilvio commented Jun 10, 2020

The pull request is here #119. :shipit:

@filipeximenes
Copy link
Contributor Author

Thanks folks! PR has been merged and it's on PyPI under version 3.1.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants