Prevent group admins from changing their own role

[bzar]

Fixes #4560

This is one way to deal with the issue. I tried to make the changes as small as possible while still avoiding offering error-producing interaction paths to the user. I'm not entirely confident all the changes here have been implemented in the most elegant and style-preserving fashion, especially the ignore_self frontend part, but at the very least it works in our environment to counter the issue.

Proposed fixes:

• Prevent group admins from changing their own role
• Skip current user when autocompleting in new member view
• Disable role input when user is an admin editing their own membership

Features:

• includes tests covering changes
• includes updated documentation
• includes user-visible changes
• includes API changes
• includes bugfix for possible backport

Please [X] all the boxes above that apply

[smotornyuk]

I think it's not quite right. Originally, the problem came from the cases when someone was adding organization creator(himself, probably) as an organization member with access level different from admin. You are forbidding organization admin to reassign from his role.
I believe, that any admin has right do leave organization/decide to stop administering. Even if all admins were reassigned, there is always an organization creator, who can grant admin permissions to new members. What needs to be done is either:
a) Do not allow the addition of ORG_CREATOR as an organization member.
b) If the first option is not acceptable - do not allow to revoke admin role if there only one admin left in the organization.

I'm strongly against restricting operations with my own account. If I decided that I don't want to be an admin anymore - I should be able to change my membership level at any time.

[bzar]

The reason I implemented it like this is that both creating and updating members is done through member_new, unlike other APIs where these are separated. I couldn't see a real way of differentiating between the cases within the member_new action where an admin is being "recreated" as a non-admin, and where their privileges are just being edited away. Everything else was just removing error-producing options from the UI.

That said, I do agree with the general sentiment that this is not an optimal solution. One additional option that comes to mind would be to add a hidden parameter to the edit dialog that would skip the check, which would allow for self-edits but not self-creates.

[smotornyuk]

Whenever it's possible, all changes should be API-driven. Addition of hidden field will resolve the problem in UI, but direct API calls still vulnerable to this problem. Do you really need to separate calls inside member_new? If you choose to do it inside member_create action, you'll have everything you need by the end of the action and will be able to interrupt call with auth exception

[bzar]

Sorry, mixed the action name with the template name there. In the PR I'm doing the actual checking in the member_create action. At that point it's AFAICT impossible to tell if the caller meant to update an existing membership or create a new one. If they meant to create a new user membership but are in fact overriding an administrator the error should trigger. During the function call we know the desired end result "the user U is a member of group G with role R" and the user's possible previous membership and role, but not what the intent behind the call was. I don't see how this is sufficient to figure out if the error should trigger on its own, if it's only meant to stop accidentally recreating administrator memberships as something else, since recreating and updating calls are identical by that time.

The root of the issue, as I see it, is having both create and update in a single action, where they should be two. If they were separated, this couldn't be an issue.

[smotornyuk]

Let's focus on the desired behavior first. I/e, now we need to decide WHAT we are expecting from this functionality, not HOW we are going to implement this. And, when the workflow is agreed, only then we need to work out a solution

[bzar]

No argument there. The expected behavior in the bug is worded as "Admin of organization should not be able to add himself to organization members", is this correct?

Currently updating and creating memberships are identical action calls, so implementing the restriction as written also stops admins from updating themselves (current PR). If we want to restrict one and not the other, there needs to be a way to tell them apart, whatever it may be.

Lastly, there need to be some changes to the UI so it doesn't display error-producing actions (like autocomplete the admin himself in the new member dialog if it would produce an error.

[smotornyuk]

I see-e-e, sorry - i've misread original issue and all the time was incorrectly interpreting the problem. Now it has a lot of sense. Could you please fix those issues from CircleCI:

---------------------------
ckan/logic/action/create.py
---------------------------
E126 ln:614 continuation line over-indented for hanging indent

-----------------------
ckan/plugins/toolkit.py
-----------------------
E123 ln:396 closing bracket does not match indentation of opening bracket's line
E123 ln:397 closing bracket does not match indentation of opening bracket's line

and i'll merge this PR

[bzar]

Sure, though the second one isn't even from this PR :)

[smotornyuk]

🎆 Thanks