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

Replace GroupACL with something more reasonable [$50 awarded] #1798

Closed
nijel opened this Issue Jan 17, 2018 · 17 comments

Comments

Projects
8 participants
@nijel
Member

nijel commented Jan 17, 2018

While the GroupACL is certainly better than having just group based access Django provides, it turns out to be pretty hard to configure properly due to permission filtering and layering of GroupACLs. Also the logic to evaluate this is unnecessarily complex.

Summary of this is available in the wiki: https://github.com/WeblateOrg/weblate/wiki/Access-control-design

Problems with current GroupACL:

  • Unclear layering of them, it's pretty much coded here, but ordering within same group is pretty much random:

# more specific rules are more important:
# subproject > project > language
acls.sort(reverse=True, key=lambda a: (
a.subproject is not None,
a.project is not None,
a.language is not None))
for acl in acls:
groupacls.append((
acl.groups.all() & user.groups.all(),
acl.permissions.values_list('id', flat=True)
))

  • The idea of permissions filtering is usually to complex to setup
  • Exposing all permissions in the admin interface adds unnecessary complexity as most of the automatically generated Django permissions are not used
  • The can_access permission can not be used with per language ACL as it's only applied to the translation (leading to confusing situations that user can not access /project/foo/ but can access /project/foo/bar/cs/)
  • The project and language ACL does not seem to work (this might be just bug in the current implementation)

We probably should focus on creating something simple which will cover most of the use cases and then potentially extend it.

What we need from the access control system:

  • Define access to individual projects, translations or languages (not sure about components)
  • Granular permissions (access project, translate, suggest, review, ...)
  • Interface to edit per project groups directly in Weblate
  • Automatic adding of users to groups

What can be probably omitted from current feature:

  • Restrict access by additional rules (currently GroupACL can be used to revoke permissions)
  • Defining permissions for individual users, having groups is good enough in most cases

Following use cases should work (some of them might be duplicate, but that's real world situations I've met so far):

  • Grant every user translation permission to a project
  • Grant project visibility to group only
  • Grant group review permission to a language in all projects
  • Grant group translation permission to a language in all projects
  • Grant group translation permission to a language in single project
  • Grant admin access to a project to a group

Bountysource


The $50 bounty on this issue has been claimed at Bountysource.

@nijel nijel added the enhancement label Jan 17, 2018

@nijel

This comment has been minimized.

Member

nijel commented Jan 17, 2018

List of related issues:

  • #1687 - Per language access control
  • #1640 - Billing access control

This will probably end up in completely replacing Django User/Group models, so it should address limits we're hitting there as well like maximal length of full name.

@nijel

This comment has been minimized.

Member

nijel commented Jan 17, 2018

Possible implementation:

Models

User

  • Username
  • Full name
  • E-mail
  • Password
  • Superuser flag (used also as staff flag)
  • Active flag (used to delete)
  • Timestamps (date joined, last login)
  • Groups (many to many)

Group

  • Name
  • Projects (many to many, defines projects this group has access to)
  • Language (many to many, defines languages this group has access to)
  • Permissions (many to many)
  • Flags [manual/automatic, all_projects/selected, all_languages/selected] (to allow automation for language/project selection and to distinguish Weblate managed groups from user managed ones)

Permission

  • ID
  • Verbose name (translatable)

Permission check

Access project

  • To check permission: user.groups.filter(project=x).exists()
  • To list allowed projects: Project.objects.filter(group__user=x)

Access translation

  • To check permission: user.groups.filter(project=x.component.project, language=x.language).exists()
  • To list allowed translations: component.translation_set.filter(language__group__in=user.groups.filter(project=x.component.project)

Individual permissions

  • On translation level user.groups.filter(project=x.component.project, language=x.language, permission=PERM).exists()
  • On project level user.groups.filter(project=x, permission=PERM).exists()
@lawmaestro

This comment has been minimized.

lawmaestro commented Jan 17, 2018

Agreed. The way the permissions currently work feels a bit overly complicated for the (relatively) simple use cases outlined. A simplified replacement of this with a basic interface on it would much better suit our needs (imo)

@michaelplatt

This comment has been minimized.

michaelplatt commented Jan 18, 2018

After meeting Michal last week I have to admit that the GroupACL method still leaves me confused.

My feeling is that the whole thing can be simplified a great deal as follows:

  • Use Groups to manage the list of specific rights applicable for that set of users.
  • As part of Project/Component admin, specify which groups can access it.
  • Specify Languages at the group level to enable access to those languages for the projects they can access.

If necessary, individual users can be given additional rights, languages, and project/component access beyond that provided by the group(s) they belong to.

In general, I believe access rights should always be additive and a user should have the aggregate rights of their specific settings plus any groups they belong to.

@stanislav-brabec

This comment has been minimized.

stanislav-brabec commented Jan 18, 2018

Group ACL is a work-around of the limitations of Django Groups. Django groups assign permissions globally, for all projects. It cannot assign permission for a particular language, project, component or list. Group ACL excludes non-members of group from work on a project.

Ideal new permission system should integrate:

  • Good granularity: per project, per component, per language, per list
  • Positive permissions: Each user in group Czech Reviewers can review Czech strings.
  • Negative permissions: Don't apply the above for Foo. Nobody else than Foo Translation Team can access.
  • Intersection: User in groups Czech Reviewers and Foo Translation Team can review Foo/Czech.
  • Priorities: Each user in group Foo Wizards is allowed to do everything in the Foo/Czech even without membership in Czech Reviewers group.
  • Transitivity: User in group Foo Admins can dedicate selected permissions to other users.

Negative permissions can be turned into a positive permissions: The Project, Component, List requires membership in one of these Groups.

Permissions for groups of projects could be integrated with component lists, so it could be easily kept in sync. It could be also integrated with system-wide glossaries (glossaries per list, #1158).

@nijel

This comment has been minimized.

Member

nijel commented Jan 19, 2018

  • Good granularity: I'm not so sure about the per component permissions
  • Positive permissions: In above proposal having group Czech Reviewers with rewiew permission, Czech language and all projects would do that
  • Negative permissions: just remove Foo project from all other groups
  • Intersection: That would need separate group Foo Czech Reviewers
  • Priorities: IMHO this again adds too much complexity
  • Transitivity: Admin permission will be there
@stanislav-brabec

This comment has been minimized.

stanislav-brabec commented Jan 19, 2018

Assigning projects/components/lists to groups sounds like a good idea.

Per component permissions use case

  • master branch: community version, open for anybody
  • maintained branch: commercially supported version, open for contracted translators only

just remove Foo project from all other groups
Imagine that you have 50 language groups for community reviewers, and you want to exclude all those reviewers from an access to a certain project/component without making the project private. It would require editing of 50 groups. Group ACL can do it in a single line.

That would need separate group Foo Czech Reviewers
But more probably that would require 50 separate groups Foo * Reviewers, and regular sync of user list between Czech Reviewers and Foo Czech Reviewers. Group ACL can do it in a single line.

IMHO this again adds too much complexity
I can imagine an ordered list of rules that are tested

@nijel

This comment has been minimized.

Member

nijel commented Jan 22, 2018

Group ACL can do it in a single line.

Yes, but at costs that you can not do many other things which are needed way more. No Group ACL is not a way to go (I think you've learned that yourself as well).

I can imagine an ordered list of rules that are tested

Certainly it can be tested, that's not a problem. Problem is that you can not test all possible combinations (what can be clearly seen at current Group ACL code - it's quite well tested, but it's not that hard to come up with configuration that will just block any access). Also any complexity you add makes it harder to setup by users and that is important as well.

@davidnorthetal

This comment has been minimized.

davidnorthetal commented Jan 25, 2018

I'm very much for restricting groups to individual components within a project.

The idea would be to have variation components of a shared global component (think forks), then restrict users to these forks, and within the same group they would only see their component and the shared component between them and can take suggestions from the shared component.

@nijel nijel added this to the 3.0 milestone Jan 30, 2018

@ieugen

This comment has been minimized.

Contributor

ieugen commented Jan 31, 2018

Hi,

Is using groups the right thing? Having permissions is great. In other systems, they are grouped in Roles. Does django support that?

Whenever I have to create new group for language, I have to setup all permissions again and again. Managing this is very error prone.

Having roles as a way to group permissions is a nice thing.

Wdyt?

@nijel

This comment has been minimized.

Member

nijel commented Jan 31, 2018

The Django permission system does not work well for us for several reasons. The attempt to adapt it to our needs with Group ACL didn't seem to go well. So in the end I'm convinced we need custom permission system that will fit our needs. I've looked at several existing ACLs for Django and I didn't find anybody what would fit.

Having roles might be useful, however I was rather considering simplification of permissions, so that setting up them would not be that hard. The question is whether existing privileges really can be reasonably reduced...

@yarons

This comment has been minimized.

Contributor

yarons commented Feb 1, 2018

Is there a list showing the entire feature list for each and every considered solution so we can create a pros and cons list from?

@nijel

This comment has been minimized.

Member

nijel commented Feb 1, 2018

Not yet, the only detailed think so far is spec I've drafted in #1798 (comment). That doesn't include component level permissions and some other features requested in #1798 (comment).

I will probably work on updated proposal after releasing 2.19, so far I've added basic summary to wiki: https://github.com/WeblateOrg/weblate/wiki/Access-control-design

@nijel nijel self-assigned this Feb 1, 2018

@btkostner

This comment has been minimized.

btkostner commented Feb 2, 2018

While I can't comment about the code, I can comment a little bit on how @elementary handles it's permissions for translations. Right now we have a group for each language ('Japanese Translation Team') that has permissions to edit most of the stuff for that language. This goes across all projects and components on our instance. Right now this involves creating a group for every language manually and coping the permissions from one already setup.

While I wouldn't expect every setup to be the same, I think the biggest drawback the current implementation is having to select every permission individually. It would be really nice not having to do this. Also, just want to say thank you for taking the time to work on this. It's one of the pain points I have found with configuring weblate.

@nijel nijel added the bounty label Feb 7, 2018

nijel added a commit that referenced this issue Feb 8, 2018

Differently handle intermediate signals we trigger to create permissions
We need Django to generate permissions, but we can not yet execute own
code as all migrations are not yet applied. This can be removed once
new permissions (#1798) are implemented.

Issue #1838

Signed-off-by: Michal Čihař <michal@cihar.com>

@nijel nijel added this to To do in Access control Feb 15, 2018

@nijel nijel changed the title from Replace GroupACL with something more reasonable to Replace GroupACL with something more reasonable [$50] Mar 5, 2018

@nijel nijel changed the title from Replace GroupACL with something more reasonable [$50] to Replace GroupACL with something more reasonable May 3, 2018

@nijel nijel changed the title from Replace GroupACL with something more reasonable to Replace GroupACL with something more reasonable [$50] May 3, 2018

nijel added a commit to nijel/weblate that referenced this issue May 7, 2018

Initial structure for new authentication code
Issue WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 9, 2018

Initial implementation of groups assignment for project
It keeps previous syntax Project@Group to make thinkgs easier. There is
now additional Viewer global role which is used to control public
visibility.

Issure WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 9, 2018

Initial structure for new authentication code
Issue WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 9, 2018

Initial models for new authentication
Issue WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 9, 2018

Add initial migration for new auth model
This has to define just the user model to avoid circular dependency on
other models while using project or langauge in the group definition.

Issue WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 9, 2018

Switch to custom user model
Data is not yet migrated, tests will fail at this point.

Issue WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 9, 2018

Remove GroupACL model
Issue WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 9, 2018

Initial implementation of groups assignment for project
It keeps previous syntax Project@Group to make thinkgs easier. There is
now additional Viewer global role which is used to control public
visibility.

Issure WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>
@nijel

This comment has been minimized.

Member

nijel commented May 10, 2018

Code should be more or less ready in #2002, so if somebody is brave enough to test this, any feedback is welcome. What is still missing there is documentation update and a little bit of polishing.

nijel added a commit to nijel/weblate that referenced this issue May 14, 2018

Initial structure for new authentication code
Issue WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 14, 2018

Initial models for new authentication
Issue WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 14, 2018

Add initial migration for new auth model
This has to define just the user model to avoid circular dependency on
other models while using project or langauge in the group definition.

Issue WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 14, 2018

Switch to custom user model
Data is not yet migrated, tests will fail at this point.

Issue WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 14, 2018

Remove GroupACL model
Issue WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 14, 2018

Initial implementation of groups assignment for project
It keeps previous syntax Project@Group to make thinkgs easier. There is
now additional Viewer global role which is used to control public
visibility.

Issure WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 14, 2018

Update documentation to new authentication model
Issue WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 15, 2018

Initial structure for new authentication code
Issue WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 15, 2018

Initial models for new authentication
Issue WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 15, 2018

Add initial migration for new auth model
This has to define just the user model to avoid circular dependency on
other models while using project or langauge in the group definition.

Issue WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 15, 2018

Switch to custom user model
Data is not yet migrated, tests will fail at this point.

Issue WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 15, 2018

Remove GroupACL model
Issue WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 15, 2018

Initial implementation of groups assignment for project
It keeps previous syntax Project@Group to make thinkgs easier. There is
now additional Viewer global role which is used to control public
visibility.

Issure WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 15, 2018

Update documentation to new authentication model
Issue WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 15, 2018

Update documentation to new authentication model
Issue WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/weblate that referenced this issue May 16, 2018

Update documentation to new authentication model
Issue WeblateOrg#1798

Signed-off-by: Michal Čihař <michal@cihar.com>
@nijel

This comment has been minimized.

Member

nijel commented May 16, 2018

Thank you for your report, the issue you have reported has just been fixed by merging #2002.

  • In case you see problem with the fix, please comment on this issue.
  • In case you see similar problem, please open separate issue.
  • If you are happy with the outcome, consider supporting Weblate by donating.

@nijel nijel closed this May 16, 2018

Access control automation moved this from To do to Done May 16, 2018

@nijel nijel changed the title from Replace GroupACL with something more reasonable [$50] to Replace GroupACL with something more reasonable [$50 awarded] Oct 10, 2018

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