Manage organization "People" #11342

divbzero · 2022-05-05T21:53:33Z

Closes Add Members to an Organization account #11076.
Closes Remove Members from an Organization account #11077.
Closes Cancel User invitation from an Organization account #11078.
Closes Assign role to an Organization account Member #11081.
Closes Accept/reject Organization invite #11083.

Borrowing styles from list of projects for now.

https://github.com/FortAwesome/Font-Awesome/blob/4e6402443679e0a9d12c7401ac8783ef4646657f/js-packages/%40fortawesome/free-solid-svg-icons/faUsers.js

Use the same gradient of grays as white-cube.svg.

- {.package-snippet__project-badge => .package-snippet__badge} - {.package-snippet__released => .package-snippet__created}

Cards with similar styling but different icons: - .organization-snippet - .package-snippet

"Manage" link for each organization listed on /manage/organizations/ navigates to a "Collaborators" management page for the organization.

@s-mm

Discussed with @s-mm and renaming to differentiate "people" in organizations from "collaborators" in projects.

I already added "Your organizations" to the sidebar for desktop but forgot to add it for mobile.

Status badges: - "Request Submitted" if still awaiting approve/decline - "Inactive" if declined or inactive for other reasons Role badges: - "Manager" for organization manager - "Owner" for organization owner - "Billing Manager" for billing manager

@sterbo

This is my third attempt at stabilizing these tests but I finally see the light of what I was doing wrong. I should not rely on probabilistic tests at all even if the probability is "very very very low". @sterbo actually suggested this earlier but it didn't fully click in my head until now. Co-authored-by: sterbo <[email protected]>

- Implement pypi#11076.

@s-mm

- Remove "Billing Manager" choice for non-companies - Add "Billing Manager" role description from @s-mm

- Copy-paste `revoke_project_role_invitation(...)`.

- Implement pypi#11078.

- Copy-paste `TestVerifyProjectRole`.

- Implement approve/decline tests.

Notification emails for the organization owner: - organization-member-invited - organization-member-invite-canceled - organization-member-invite-declined - organization-member-added - organization-member-removed - organization-member-role-changed Notification emails for the invited user: - verify-organization-role - canceled-as-invited-organization-member - declined-as-invited-organization-member - added-as-organization-member - removed-as-organization-member - role-changed-as-organization-member

divbzero

@ewdurbin This is ready for code review but @sterbo and I wanted to confirm a few details about permissions with you, please see comments on those below. Thank you!

divbzero · 2022-05-05T22:01:06Z

warehouse/organizations/models.py

+        # acls = [
+        #     (Allow, "group:admins", "manage:organization"),
+        #     (Allow, "group:moderators", "manage:team"),
+        # ]


@ewdurbin Do we want to grant PyPI admins or moderators sudo privileges to manage organizations or teams?

Yes. But as is we currently perform all admin actions via the admin app/interface.

Granting permissions this way would allow admins to use the user-facing interface to make these changes, I assume? That'd be a departure.... @di thoughts here? My preference is probably to maintain the current separation of concerns via the utility sets a la warehouse/utils/project.py

I've been thinking about doing this vs. maintaining a very similar but separate set of views. I think it's probably better to do the latter, because we can add additional context, and also easily do things like not send emails/notifications/events when admins perform actions

Agreed. In a quick review it appears that we do the ACL dance on Projects to set the level of access that PyPI Admins/Moderators have. So we should do similar here, but the use-case won't be a "sudo" mode, just setting the ACLs so we can act on them in warehouse/admin views.

@sterbo added "admin" and "moderator" in a95f708.

divbzero · 2022-05-05T22:06:15Z

warehouse/organizations/models.py

+            elif role.role_name == OrganizationRoleType.Manager:
+                acls.append((Allow, f"user:{role.user.id}", ["manage:team"]))
+            else:  # TODO Member: Do we need this? May be covered by Project acl?
+                acls.append((Allow, f"user:{role.user.id}", ["organization:member"]))


@ewdurbin There are two things we want to confirm here:

We think that project-related permissions will be governed on the project level with "manage:project" and we don’t need anything for that here. Would you agree?

Will users need to be logged in and an organization member to view the organization’s projects and people? If yes, we think we’ll want to define a "view:organization" permission.

@ewdurbin We’ve added "view:organization" with commit 5e453dc so that:

All organization members will have view access to organization settings

Non-owners can remove themselves from the organization

This feels like a sensible approach, but let us know your thoughts and we can adjust as needed.

(In the future, probably when package namespacing is implemented, it could make sense to add a public view of each organization’s packages at https://pypi.org/~pypa/ or https://pypi.org/@pypa/.)

- Allow read access to organization for all members - Allow non-owners to remove self from organization

ewdurbin

No blockers. Could another of @pypa/warehouse-committers review? One place I didn't have a lot of time to look over was the email templates, as I hope these will be re-considered during user testing.

warehouse/accounts/views.py

ewdurbin · 2022-05-12T09:14:26Z

warehouse/accounts/views.py

+        submitter_user = user_service.get_user(data.get("submitter_id"))
+        organization.record_event(
+            tag="organization:organization_role:declined",
+            ip_address=request.remote_addr,


In this instance we may want to either anonymize the ip_address on the event for the OrganizationEvent by hard coding to 0.0.0.0/127.0.0.1 or somehow be sure we don't leak it on display to Organization admins.

This is a potential vector for abuse/leak. I can just invite a bunch of people to an org and get their IPs if they decline (or accept I suppose!)

@di @dstufft thoughts?

We would probably want to anonymize/redact IP addresses on project collaboration invitations as well.

A pattern we use elsewhere is adding "redact_ip": True to "account:email:sent" events:
https://github.com/pypa/warehouse/blob/64fc7f1e4922017b476643ba7eda0859d69ae137/warehouse/admin/views/emails.py#L102-L111

And then using that flag to mask the IP address when displaying the event to users:
https://github.com/pypa/warehouse/blob/64fc7f1e4922017b476643ba7eda0859d69ae137/warehouse/templates/manage/account.html#L669-L672

A somewhat related issue regarding project deletion/re-use:

PII is leaked across project deletion #4457

ewdurbin · 2022-05-12T09:29:40Z

warehouse/organizations/models.py

+        # acls = [
+        #     (Allow, "group:admins", "manage:organization"),
+        #     (Allow, "group:moderators", "manage:team"),
+        # ]


Agreed. In a quick review it appears that we do the ACL dance on Projects to set the level of access that PyPI Admins/Moderators have. So we should do similar here, but the use-case won't be a "sudo" mode, just setting the ACLs so we can act on them in warehouse/admin views.

ewdurbin · 2022-05-12T09:46:20Z

warehouse/manage/views.py

@@ -1028,6 +1039,20 @@ def user_organizations(request):
    }


+def organization_owners(request, organization):


For some reason I want this to exist on the Organization service... But we do a similar thing for Project roles. I can't recall if there's a reason for that.

If we want to do this, it could be a bigger refactor where we move the helper functions for Project roles to warehouse.packaging.services.

ewdurbin · 2022-05-12T09:48:35Z

warehouse/templates/email/added-as-collaborator/subject.txt

@@ -14,4 +14,4 @@

 {% extends "email/_base/subject.txt" %}

-{% block subject %}{{ initiator_username }} has invited you to join the {{ project_name }} project{% endblock %}
+{% block subject %}You have joined the {{ project_name }} project{% endblock %}


Good catch!

ewdurbin · 2022-05-12T09:51:49Z

warehouse/templates/manage/projects.html

@@ -19,7 +19,7 @@

 {% block main %}
  {% if project_invites %}
-  <h1 class="page-title">{% trans project_count=project_invites|length %}Pending invitations ({{ project_count }}){% endtrans %}</h1>
+  <h1 class="page-title">{% trans invitation_count=project_invites|length %}Pending invitations ({{ invitation_count }}){% endtrans %}</h1>


another good catch 🐟

@ewdurbin

Update error text based on @ewdurbin's suggestion in pypi#11342.

brainwane

Minor wording stuff.

brainwane · 2022-05-12T16:41:45Z

warehouse/accounts/views.py

+        token = request.params.get("token")
+        data = token_service.loads(token)
+    except TokenExpired:
+        return _error(request._("Expired token: request a new organization invite"))


Double-checking: should this (and the "invalid token" message) say "invite" or "invitation"?

Updated error text in 2480614. We seem to use ‘invitation’ everywhere else and it is unambiguously a noun.

brainwane · 2022-05-12T16:43:01Z

warehouse/locale/messages.pot

 msgid "Email address ${email_address} verified. ${confirm_message}."
 msgstr ""

-#: warehouse/accounts/views.py:839
+#: warehouse/accounts/views.py:846
+msgid "Expired token: request a new organization invite"


(Same question - whether "invite" or "invitation" is right here, and in the Invalid token message below as well.)

(Updated error text in 2480614.)

brainwane · 2022-05-12T16:44:55Z

warehouse/locale/messages.pot

+#: warehouse/templates/manage/organizations.html:22
+#: warehouse/templates/manage/projects.html:22
+#, python-format
+msgid "Pending invitations (%(invitation_count)s)"


I think this will read better as

"(%(invitation_count)s) pending invitations"

or as

"Pending invitations: (%(invitation_count)s)" [note the colon]

brainwane · 2022-05-12T16:49:06Z

warehouse/templates/manage/organizations.html

@@ -18,8 +18,35 @@
 {% block title %}{% trans %}Your organizations{% endtrans %}{% endblock %}

 {% block main %}
-  <h1 class="page-title">{% trans organization_count=organizations|length %}Your organizations ({{ organization_count }}){% endtrans %}</h1>
+  {% if organization_invites %}
+  <h1 class="page-title">{% trans invitation_count=organization_invites|length %}Pending invitations ({{ invitation_count }}){% endtrans %}</h1>


Suggested change

<h1 class="page-title">{% trans invitation_count=organization_invites|length %}Pending invitations ({{ invitation_count }}){% endtrans %}</h1>

<h1 class="page-title">{% trans invitation_count=organization_invites|length %}Pending invitations: ({{ invitation_count }}){% endtrans %}</h1>

This could affect other heading text as well such as Your projects (5) or Your organizations (2) which appear in the side pane and the main pane on different pages.

@brainwane

Update error text based on @brainwane's suggestion in pypi#11342.

sterbo and others added 30 commits May 5, 2022 14:17

Added three get organizations variations

641bda1

List organizations on /manage/organizations/ page

af9e24e

Borrowing styles from list of projects for now.

Add SVG of fa-users from Font Awesome 5.13.0

48bc586

https://github.com/FortAwesome/Font-Awesome/blob/4e6402443679e0a9d12c7401ac8783ef4646657f/js-packages/%40fortawesome/free-solid-svg-icons/faUsers.js

Convert users.svg to light gray

7fd7eb4

3 shades of gray for users.svg

c9b78a1

Use the same gradient of grays as white-cube.svg.

Darker shades of gray for users.svg

084a380

Add Font Awesome 5.13.0 attribution to users.svg

44dfd93

Resize users.svg to match size of white-cube.svg

15597d7

Convert {users.svg => users.png}

d0a3d61

NFC: Rename {_package-snippet => _snippet}.scss

0f2da8e

NFC: Rename .package-snippet__* classes

5d40f2c

- {.package-snippet__project-badge => .package-snippet__badge} - {.package-snippet__released => .package-snippet__created}

snippet mixin for organizations and packages

c5ff0f2

Cards with similar styling but different icons: - .organization-snippet - .package-snippet

Add "Your organizations" to "Your account" sidebar

a27b0b3

Display callout block if user has no organizations

50ed814

NFC: Use for..else Jinja2 syntax for projects.html

81997b3

"Manage" collaborators for each organization

81eb8fc

"Manage" link for each organization listed on /manage/organizations/ navigates to a "Collaborators" management page for the organization.

Add sidebar for managing "Your organizations"

3fdb89b

Clean up SCSS imports to fix bin/static_lint

5e2fbf5

Rename {Collaborators => People} for organizations

9e43639

Discussed with @s-mm and renaming to differentiate "people" in organizations from "collaborators" in projects.

Update "Your account" sidebar for mobile

2ed0c34

I already added "Your organizations" to the sidebar for desktop but forgot to add it for mobile.

admin: Fix table spacing for cells with long text

18d6d34

Add services for org roles and invites

ad41487

Role descriptions for organization "People" settings

0f2d1b6

"Invite" form for organization "People" settings

e50902e

- Implement pypi#11076.

"Billing Manager" role for companies only

03fed9e

- Remove "Billing Manager" choice for non-companies - Add "Billing Manager" role description from @s-mm

List roles/invites for organization "People"

f8a8269

Revoke organization invite (1/2)

e9fcceb

- Copy-paste `revoke_project_role_invitation(...)`.

Revoke organization invite (2/2)

c20c4d9

- Implement pypi#11078.

divbzero and others added 15 commits May 5, 2022 14:39

Test approve/decline organization invites (1/2)

5071073

- Copy-paste `TestVerifyProjectRole`.

Test approve/decline organization invites (2/2)

bded23a

- Implement approve/decline tests.

Fix subject for added-as-collaborator email

45d6306

send_*_email for organization roles/invites

f00e95b

Test emails for organization roles/invites

f77333a

Send emails for organization roles/invites

8b081ba

Record events for organization roles/invites

93d5e18

Add task organization role invite expiry

58a46ea

Add task organization role invite expiry tests

597d065

Add acl function to organization model

693215c

Tests for Organization acl functionality

c77bf71

Fix to OrganizationNameCatalogFactory name field

6bd69e6

Require "manage:organization" to manage org

c6c8c0e

Add "Your organizations" to user dropdown menu

a332623

divbzero force-pushed the feature/manage-organization-people branch from b90fb95 to a332623 Compare May 5, 2022 21:57

divbzero commented May 5, 2022

View reviewed changes

divbzero requested a review from ewdurbin May 5, 2022 22:08

"view:organization" permission for all org members

5e453dc

- Allow read access to organization for all members - Allow non-owners to remove self from organization

divbzero force-pushed the feature/manage-organization-people branch from ba4bfcf to 5e453dc Compare May 6, 2022 18:58

ewdurbin added the organizations label May 10, 2022

Merge branch 'main' into feature/manage-organization-people

62efa71

ewdurbin approved these changes May 12, 2022

View reviewed changes

"{collaboration => organization} invitation"

3d7eca1

Update error text based on @ewdurbin's suggestion in pypi#11342.

brainwane reviewed May 12, 2022

View reviewed changes

sterbo and others added 2 commits May 12, 2022 10:09

Update the acl on organizations for admins

a95f708

"{invite => invitation}" for consistency

2480614

Update error text based on @brainwane's suggestion in pypi#11342.

ewdurbin merged commit addad4d into pypi:main May 13, 2022

domdfcoding pushed a commit to domdfcoding/warehouse that referenced this pull request Jun 7, 2022

Manage organization "People" (pypi#11342)

0a73201

stephenfin mentioned this pull request Jul 11, 2022

Support Groups/Organizations #201

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Manage organization "People" #11342

Manage organization "People" #11342

divbzero commented May 5, 2022

divbzero left a comment

divbzero May 5, 2022

ewdurbin May 10, 2022

di May 10, 2022

ewdurbin May 12, 2022

divbzero May 12, 2022

divbzero May 5, 2022

divbzero May 6, 2022

ewdurbin left a comment

ewdurbin May 12, 2022

ewdurbin May 12, 2022

divbzero May 12, 2022

divbzero May 13, 2022

ewdurbin May 12, 2022

ewdurbin May 12, 2022

divbzero May 12, 2022

ewdurbin May 12, 2022

ewdurbin May 12, 2022

brainwane left a comment

brainwane May 12, 2022

divbzero May 12, 2022

brainwane May 12, 2022

divbzero May 12, 2022

brainwane May 12, 2022

brainwane May 12, 2022

divbzero May 12, 2022

		@@ -1028,6 +1039,20 @@ def user_organizations(request):
		}


		def organization_owners(request, organization):

	<h1 class="page-title">{% trans invitation_count=organization_invites\|length %}Pending invitations ({{ invitation_count }}){% endtrans %}</h1>
	<h1 class="page-title">{% trans invitation_count=organization_invites\|length %}Pending invitations: ({{ invitation_count }}){% endtrans %}</h1>

Manage organization "People" #11342

Manage organization "People" #11342

Conversation

divbzero commented May 5, 2022

divbzero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewdurbin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brainwane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment