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

Strict Translate Permissions #149

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

JohnMoutafis
Copy link

Implements initial idea for strict translate permissions as described in issue #147

JohnMoutafis and others added 8 commits November 29, 2018 11:14
Added configuration option for enabling strict translator privileges.
Updated hooks to allow restrictions to translator groups.
Added configuration option for enabling strict translator privileges.
Updated hooks to allow restrictions to translator groups.
@mikedingjan
Copy link
Member

Lets continue the discussion over here.

I've looked through your PR, still don't agree on having a setting which will "magically" add behavior in restricting which pages someone can see, especially since the languages the user has access to are collected from the groups it's member of (with string matching)..

How to determine which languages the user has access to is for me as well unclear, maybe a intermediate model between groups and languages, this way we can query with the actual language objects (no string splitting / matching). This would also make it possible to define read / write / delete permissions per language per group (for example: translator-be should be enough to allow nl_BE and fr_BE). A custom PermissionsTester can handle these checks for example.

Creating queries with actual Language objects should also perform a lot better, since you can replace language__code__in=[] which saves some joins.

Also checking for group name to determine if a user is "privileged" is greatly discouraged, since group names can change, maybe there is, or need to be, some custom permission to check for? Checking that way it will also be possible to add more "privileged" user groups by adding that permission to the group.

Any thoughts on this?

@JohnMoutafis
Copy link
Author

@mikedingjan
Your suggestions seem very reasonable.
I made this PR mostly to show you what we did as a workaround for our use-case.

Feel free to adjust the code as you see fit (and it goes without saying that I will be happy to help on that issue 😄)

@mikedingjan
Copy link
Member

I know, that's why I wanted to continue the discussion here, so we can all agree on an approach which works for all. :) Do you have any additional requirements with proposed solution?

Any other thoughts about this? @jjanssen maybe?

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

Successfully merging this pull request may close these issues.

None yet

3 participants