Bug #16855

Enforce reviewer policy on Weblate

Added by intrigeri 2019-07-05 14:47:30 . Updated 2020-03-10 11:47:37 .

Status:
Resolved
Priority:
High
Assignee:
drebs
Category:
Infrastructure
Target version:
Start date:
Due date:
% Done:

0%

Feature Branch:
Type of work:
Sysadmin
Starter:
Affected tool:
Translation Platform
Deliverable for:
309

Description

I.e. enforce “one needs to have Reviewer status to be allowed to accept suggestions” and make sure that non-admin users can’t change this setting per-component.


Subtasks


Related issues

Related to Tails - Bug #16881: Puppetize critical Weblate configuration Confirmed
Related to Tails - Bug #16989: Enable voting for Weblate users Resolved
Blocks Tails - Feature #16326: Review the Weblate-based infrastructure that updates and pushes PO files Resolved 2019-01-08

History

#1 Updated by intrigeri 2019-07-05 14:47:57

Disabling suggestion_autoaccept=2 for every component may be part of the solution. It might not be enough.

#2 Updated by hefee 2019-07-05 14:49:38

we need also update files/weblate/scripts/tailsWeblate.py for the new components.

#3 Updated by intrigeri 2019-07-05 14:52:54

  • Assignee set to hefee

#4 Updated by intrigeri 2019-07-05 14:53:39

  • Description updated

#5 Updated by hefee 2019-07-05 15:03:26

  • Description updated

#6 Updated by hefee 2019-07-05 15:08:43

  • Affected tool set to Translation Platform

#7 Updated by intrigeri 2019-07-05 15:11:11

  • blocks Feature #16326: Review the Weblate-based infrastructure that updates and pushes PO files added

#8 Updated by Anonymous 2019-07-05 15:15:09

#9 Updated by intrigeri 2019-07-05 15:21:21

  • Priority changed from Elevated to High

#10 Updated by hefee 2019-07-06 22:35:11

We want the Dedicated Reviewers concept:

https://docs.weblate.org/en/latest/workflows.html#dedicated-reviewers

for switching all components

sudo -u weblate bash
python3 /usr/local/share/weblate/manage.py /tmp/file.py

cat /tmp/file.py

<code class="python">
from weblate.trans.models import *
todos = SubProject.objects.all()

for i in todos:
  i.suggestion_voting = False
  i.suggestion_autoaccept = 0
  i.enable_suggestions = True
  i.save()
</code>

#11 Updated by hefee 2019-07-06 23:10:35

  • Assignee changed from hefee to intrigeri
  • Feature Branch set to https://salsa.debian.org/hefee/puppet-tails/commits/hefee/16855-dedicated-reviewers

I executed the script I mentioned before.

For the new components I need to update tailsWeblate.py see my feature branch. After merging please execute the script again, to make sure, that new components added from now until merge have the same policy.

I created three users testuser1,testuser2 and testreviewer1 for test purpose, the passwords you find in keyringer.

My tests so far:

  • if testuser1 creates a suggestion testuser2 can’t accept it anymore, as Weblate returns a error, which is not understandable - maybe we trigger an internal error, but I don’t have the permission to look closer to the system.
  • testreviewer1 can’t accept the suggestion, to same error message is presented -> that’s why I think we triggered an internal issue.

@intrigeri: please test our usecases. You may find this interessting:
https://docs.weblate.org/en/latest/admin/translating.htmll#suggestion-voting
https://docs.weblate.org/en/latest/workflows.html

#12 Updated by Anonymous 2019-07-08 10:09:56

Any administrator on the weblate interface can change this value per component manually. How do we ensure that they don’t do that? Can we execute your script regularly?

#13 Updated by hefee 2019-07-08 22:33:29

u wrote:
> Any administrator on the weblate interface can change this value per component manually. How do we ensure that they don’t do that? Can we execute your script regularly?

@u: Yes we could run this as cron job, but why we should do that? Adminstrator permission always comes with great responsibility to not try to break the system. If we can’t trust weblate adminstrator, than we have serious other issues. E.g. an administror can always trigger a push, regardless of the settings. We never talked about enforcing settings - Why now?

IMO adminstrator permissions are only needed for fixing stuff. Aka only 2 or 3 people have this permission and should know about the power. All others should only get the permission they really need like “review” and/or “add/delete users from/to groups”.

#14 Updated by intrigeri 2019-07-09 08:19:13

  • Target version changed from Tails_3.15 to Tails_3.16

#15 Updated by Anonymous 2019-07-09 08:37:21

@hefee I agree with your reasoning. It was merely a question I am asking, not something I think we should absolutely do. My concern is more about privilege escalation - but then again, we surely do have other problems than that.

#16 Updated by intrigeri 2019-07-09 13:38:03

> For the new components I need to update tailsWeblate.py see my feature branch.

Merged.

> After merging please execute the script again, to make sure, that new components added from now until merge have the same policy.

I’ve tried and failed:

$ python3 /usr/local/share/weblate/manage.py /tmp/file.py
Unknown command: '/tmp/file.py'
Type 'manage.py help' for usage.

@hefee, what shall I do?

> intrigeri: please test our usecases. You may find this interessting:
> https://docs.weblate.org/en/latest/admin/translating.htmll#suggestion-voting
> https://docs.weblate.org/en/latest/workflows.html

Will do later today or tomorrow, unless I’m needed on other, more urgent translation platform matters.

#17 Updated by intrigeri 2019-07-09 14:07:16

  • Status changed from Confirmed to Needs Validation

#18 Updated by intrigeri 2019-07-09 14:08:27

hefee told me that the correct command was: python3 /usr/local/share/weblate/manage.py shell < /tmp/test.py.

#19 Updated by intrigeri 2019-07-12 19:36:35

@hefee, is there anything left to do here?

#20 Updated by intrigeri 2019-07-12 21:52:22

My bad, I’m still supposed to test our use cases. Forget my last question :)

#21 Updated by intrigeri 2019-07-13 19:44:12

  • Assignee changed from intrigeri to hefee

> I created three users testuser1,testuser2 and testreviewer1 for test purpose, the passwords you find in keyringer.

First, I had to review the ACLs and membership for every group, to ensure that we are effectively enforcing our chosen policy. I’ve found some surprising settings.

First of all, members of the “Users” group are allowed to do tons of things. It seems that we put all users in this group so that’s probably not what we want. I’ve just fixed that so that non-reviewer users can only do what I understand we want them to do.

Similarly, the \@Translate group had elevated privileges, equivalent to reviewer status, and we have 9 users in that group, so I’ve changed its ACLs to match what we have for “Users”. I don’t know what this group / ACL was supposed to be about nor how folks were added to it. I’ve found no doc about this in our team repo. We should probably ask whoever gave these folks such access (emmapeel?) whether it’s fine to give them reviewer status right away. What do you think?

Then, members of the \@VCS and Managers groups were allowed to do things we do not want Weblate to do, e.g. pull directly from the remote repo, bypassing our integration layer, which is a recipe for disaster. I’ve fixed that.

Then, on the reviewing front, there are 3 potentially relevant groups: https://translate.tails.boum.org/admin/auth/group/26/change/, https://translate.tails.boum.org/admin/auth/group/27/change/, https://translate.tails.boum.org/admin/auth/group/12/change/. The all have the same ACLs. So far only https://translate.tails.boum.org/admin/auth/group/12/change/ was used so I moved testreviewer1 there. I was tempted to delete 2 of these 3 groups but I would have needed to do lots more studying and reverse engineering so I skipped that for now.

@hefee, please review these updated group ACLs.

Now, one thing I’ve not seen yet is a way to grant reviewer status for one specific language. This was not explicitly part of the spec and I guess we can do without it. What do you think?

> My tests so far:
> * if testuser1 creates a suggestion testuser2 can’t accept it anymore, as Weblate returns a error, which is not understandable - maybe we trigger an internal error, but

I’ve created a suggestion as testuser1. As testuser2 I see no way to even try to accept it. I believe you saw that UI because back when you tested, members of the Users group were allowed to accept suggestions, which is not the case anymore. So I think we’re good wrt. non-reviewer users.

> * testreviewer1 can’t accept the suggestion, to same error message is presented -> that’s why I think we triggered an internal issue.

As testreviewer1, I could successfully accept testuser1’s suggestion. So I think the error you’ve seen was temporary. Please report this issue in more details if it happens again.

> I don’t have the permission to look closer to the system.

You now have access to internal Weblate errors: /var/log/weblate/weblate.log.

> You may find this interessting:
> https://docs.weblate.org/en/latest/admin/translating.htmll#suggestion-voting
> https://docs.weblate.org/en/latest/workflows.html

Thanks! I think access control changed a lot in newer Weblate versions that what we’re running, though, so perhaps we should look at the 2.20 version of the doc, as you pointed me to yesterday :)

#22 Updated by hefee 2019-07-14 00:03:55

intrigeri wrote:
> > I created three users testuser1,testuser2 and testreviewer1 for test purpose, the passwords you find in keyringer.
>
> First, I had to review the ACLs and membership for every group, to ensure that we are effectively enforcing our chosen policy. I’ve found some surprising settings.
>
> First of all, members of the “Users” group are allowed to do tons of things. It seems that we put all users in this group so that’s probably not what we want. I’ve just fixed that so that non-reviewer users can only do what I understand we want them to do.

I think a good start. Maybe those are also may be interesting in future:

- (trans| translation | Upload a translation) as this gives you the possibility to translate offline and upload a po file later on. At summit the Italian ask about that feature.
- (trans|suggestion|can change suggestion) - to be able to fix suggestions of others.

> Similarly, the \@Translate group had elevated privileges, equivalent to reviewer status, and we have 9 users in that group, so I’ve changed its ACLs to match what we have for “Users”. I don’t know what this group / ACL was supposed to be about nor how folks were added to it. I’ve found no doc about this in our team repo. We should probably ask whoever gave these folks such access (emmapeel?) whether it’s fine to give them reviewer status right away. What do you think?

Seems fine sofar. Me don’t know anything about ACLs.
We should definitely get in contact about this with emmapeel, as they are mastering the Weblate interface.

> Then, members of the \@VCS and Managers groups were allowed to do things we do not want Weblate to do, e.g. pull directly from the remote repo, bypassing our integration layer, which is a recipe for disaster. I’ve fixed that.

great.

> Then, on the reviewing front, there are 3 potentially relevant groups: https://translate.tails.boum.org/admin/auth/group/26/change/, https://translate.tails.boum.org/admin/auth/group/27/change/, https://translate.tails.boum.org/admin/auth/group/12/change/. The all have the same ACLs. So far only https://translate.tails.boum.org/admin/auth/group/12/change/ was used so I moved testreviewer1 there. I was tempted to delete 2 of these 3 groups but I would have needed to do lots more studying and reverse engineering so I skipped that for now.

make sense.

> @hefee, please review these updated group ACLs.

done.

> Now, one thing I’ve not seen yet is a way to grant reviewer status for one specific language. This was not explicitly part of the spec and I guess we can do without it. What do you think?

I’m on your page.

> > My tests so far:
> > * if testuser1 creates a suggestion testuser2 can’t accept it anymore, as Weblate returns a error, which is not understandable - maybe we trigger an internal error, but
>
> I’ve created a suggestion as testuser1. As testuser2 I see no way to even try to accept it. I believe you saw that UI because back when you tested, members of the Users group were allowed to accept suggestions, which is not the case anymore. So I think we’re good wrt. non-reviewer users.

okay fine.

> > * testreviewer1 can’t accept the suggestion, to same error message is presented -> that’s why I think we triggered an internal issue.
>
> As testreviewer1, I could successfully accept testuser1’s suggestion. So I think the error you’ve seen was temporary. Please report this issue in more details if it happens again.

okay.

> > I don’t have the permission to look closer to the system.
>
> You now have access to internal Weblate errors: /var/log/weblate/weblate.log.

yepp thanks. (Bug #16877)

> > You may find this interessting:
> > https://docs.weblate.org/en/latest/admin/translating.htmll#suggestion-voting
> > https://docs.weblate.org/en/latest/workflows.html
>
> Thanks! I think access control changed a lot in newer Weblate versions that what we’re running, though, so perhaps we should look at the 2.20 version of the doc, as you pointed me to yesterday :)

Well on suggestion-voting and workflows nothing had changed. I checked that before posting those links.

#23 Updated by hefee 2019-07-14 00:05:01

  • Status changed from Needs Validation to In Progress
  • Assignee changed from hefee to intrigeri

#24 Updated by Anonymous 2019-07-14 08:31:32

intrigeri: wow, thank you for going through this! hefee: I would not add the po file upload as a feature to that platform. People can still do that using Git, and if they translate the files using poedit we might have issues with headers and line wraps and whatnot. So I’d rather not enable that.

As for the permissions: we need to document them and ask everyone who has the right to administer this interface not to change them in the interface. Can we enforce this somehow?

#25 Updated by hefee 2019-07-14 11:08:26

u wrote:
> hefee: I would not add the po file upload as a feature to that platform. People can still do that using Git, and if they translate the files using poedit we might have issues with headers and line wraps and whatnot. So I’d rather not enable that.

@u: As I said - not need to look at it atm - this may be a further task. But I think let users upload their po files to Weblate will may introduce less issues, than doing this via git. Keep in mind line wraps and headers are no issue are more. And may be more an issue, if you use git directly ;D Within Weblate I hope they parse and format the po files consistently. Also there is a permission called “Can overwrite with translation upload”, that sounds like we don’t want to enable that. That’s why I think “Upload translation” also keeps into account, that others have already translated parts. But anyways this is wild guessing.

> As for the permissions: we need to document them and ask everyone who has the right to administer this interface not to change them in the interface. Can we enforce this somehow?

1. we should not give Administrators permission to many people and better create groups without the permission to change Group permissions
2. we can automate a reset via cron, sure. But is this efficient in cost/need relation?

#26 Updated by intrigeri 2019-07-14 13:53:09

Dropping a note without having read replies above: we also need to decide what permissions non-logged in visitors get. Then, implement our decision, and if needed, adjust the message one gets on the homepage when not logged in (“You need to be logged in to translate, otherwise you can only make suggestions”).

#27 Updated by intrigeri 2019-07-14 18:55:32

hefee wrote:
> u wrote:
>> As for the permissions: we need to document them and ask everyone who has the right to administer this interface not to change them in the interface. Can we enforce this somehow?

> 1. we should not give Administrators permission to many people and better create groups without the permission to change Group permissions

Agreed. All admins MUST be made well aware that whatever ACLs we set up are here for a reason and MUST NOT be changed without discussion and a security review of the proposed changes.

> 2. we can automate a reset via cron, sure. But is this efficient in cost/need relation?

It bothers me since our sprint that great parts of our setup were implemented so far by clicking stuff in the admin web UI, which:

  • Makes it hard to audit & review both the current state of things and future changes.
  • Implies we have no way to set up another, identical instance of this platform, which is problematic:
    • It’s hard to replicate the setup locally for dev/test purposes.
    • If we ever screw up the database hopelessly, we can’t easily reset it and restore the settings we really want to keep.

These are additional points in favour of encoding in Puppet the settings we really care about. I doubt we’ll be able to do so as part of our current project but perhaps it’s worth a ticket?

#28 Updated by intrigeri 2019-07-14 20:47:17

> we also need to decide what permissions non-logged in visitors get. Then, implement our decision, and if needed, adjust the message one gets on the homepage when not logged in (“You need to be logged in to translate, otherwise you can only make suggestions”).

Current status: when not logged in, one can suggest translations. If I understand https://docs.weblate.org/en/weblate-2.20/admin/access.html correctly, this is configured via permissions granted to the Guests group. I think we can try it like this; that’s easy to change later if needed, without impacting anything else. What do you think?

Let’s make this clear when we’ll send a proposal to -l10n@.

#29 Updated by hefee 2019-07-14 23:05:51

intrigeri wrote:

> These are additional points in favour of encoding in Puppet the settings we really care about. I doubt we’ll be able to do so as part of our current project but perhaps it’s worth a ticket?

I would say - create a ticket for that.

#30 Updated by intrigeri 2019-07-15 13:25:38

> I would say - create a ticket for that.

Bug #16881 :)

#31 Updated by intrigeri 2019-07-15 13:37:04

  • related to Bug #16881: Puppetize critical Weblate configuration added

#32 Updated by intrigeri 2019-07-15 14:25:58

  • Status changed from In Progress to Needs Validation
  • Assignee changed from intrigeri to drebs

Dear @drebs, see the message I’ve just sent you in the “Translation platform meeting notes July 5th 2019” thread.

#33 Updated by intrigeri 2019-07-15 14:26:23

  • Feature Branch deleted (https://salsa.debian.org/hefee/puppet-tails/commits/hefee/16855-dedicated-reviewers)
  • Blueprint set to https://tails.boum.org/blueprint/translation_platform/#access-control

#34 Updated by Anonymous 2019-07-19 16:43:52

#35 Updated by emmapeel 2019-07-29 07:45:16

When are the translations going to go to the main Tails repo? Once reviewed?

If so, my proposal is:

  • Create a group of translators in weblate and allow it to make changes, instead of suggestions, directly. Add all ‘tails translators’ to this group. The changes they do will only go to staging, and will not go to the main repository until they get reviewed by the reviewers subset (another group, smaller).
  • Leave suggestions enabled for anonymous users
  • Allow the translators group to accept suggestions (not mark as reviewed).

#36 Updated by intrigeri 2019-08-01 13:37:49

Hi @emmapeel!

> When are the translations going to go to the main Tails repo? Once reviewed?

In our current setup: once Approved, which in our current setup means “reviewed”.

In general in Weblate: once “stored in the file”.

> The changes they do will only go to staging, and will not go to the main repository until they get reviewed by the reviewers subset (another group, smaller).

drebs and I have looked into this today. Your proposal indeed would improve the UX a lot, especially when bootstrapping a new language.

Unfortunately, our understanding is that this is not possible: according to https://docs.weblate.org/en/weblate-2.19/workflows.html#translation-states, the only way we can store a translation without saving it to a file is as a suggestion. A translation in any other status will be “stored in the file”, and thus pushed to our production website. Or did we misunderstand something?

#37 Updated by drebs 2019-08-01 14:04:41

  • Status changed from Needs Validation to Resolved

I have reviewed the Translation Platform blueprint.

I talked to intrigeri and the rest of this issue has already been taken care of, so I’m closing it now.

#38 Updated by hefee 2019-08-22 14:02:37

  • related to Bug #16989: Enable voting for Weblate users added

#39 Updated by emmapeel 2020-03-10 11:47:37

regarding upload of translations, i have opened Feature #17515