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

Organizations: Test access from anonymous users and users outside the organization #8603

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion readthedocs/organizations/managers.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
"""Organizations managers."""

from django.conf import settings
from django.db import models

from readthedocs.core.utils.extend import SettingsOverrideObject
Expand All @@ -12,7 +12,12 @@ class TeamManagerBase(models.Manager):
"""Manager to control team's access."""

def teams_for_user(self, user, organization, admin, member):
"""Get the teams where the user is an admin or member."""
teams = self.get_queryset().none()

if not user.is_authenticated:
return teams

if admin:
# Project Team Admin
teams |= user.teams.filter(access=ADMIN_ACCESS)
Expand All @@ -30,6 +35,17 @@ def teams_for_user(self, user, organization, admin, member):

return teams.distinct()

def public(self, user):
"""
Return all teams the user has access to.

If ``ALLOW_PRIVATE_REPOS`` is `False`, all teams are public by default.
Otherwise, we return only the teams where the user is a member.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh.. do we want teams to always be public? I guess the example here is Orgs & Teams on .org once we enable them. I guess I'm fine defaulting them public, since everything is default public now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same thought as Eric here. Where, or in which situations, are we going to show these teams to anonymous users?

How do other platforms work here? In GitHub for example, I don't think you have access to see all the teams inside an organization. For example, in ours https://github.com/orgs/readthedocs/people, I don't think we have a way to list backend, frontend and advocate teams there. However, on PRs I can see those teams publicly: #929. When logged in, I can access to its members as well https://github.com/orgs/readthedocs/teams/backend

Another thing that GitHub has related to permissions on teams/organizations is "I want to show publicly that I'm member of this organization". Which adds another layer of complexity here.

I'm just describing how another platform works, I'm not saying we need to implement any of this, but just to keep in mind where are we going with this and how do we want it to work.

"""
if not settings.ALLOW_PRIVATE_REPOS:
return self.get_queryset().all()
return self.member(user)

def admin(self, user, organization=None):
return self.teams_for_user(
user,
Expand Down
19 changes: 18 additions & 1 deletion readthedocs/organizations/querysets.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from datetime import timedelta

from django.conf import settings
from django.db import models
from django.db.models import Q
from django.utils import timezone
Expand All @@ -13,13 +14,29 @@ class BaseOrganizationQuerySet(models.QuerySet):

"""Organizations queryset."""

def public(self, user):
"""
Return all organizations the user has access to.

If ``ALLOW_PRIVATE_REPOS`` is `False`, all organizations are public by default.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should probably rename this to RTD_CORPORATE or something. We have been using this setting to differentiate if we are on .org or .com lately, but it's not what the name suggests.

Otherwise, we return only the organizations where the user is a member or owner.
"""
if not settings.ALLOW_PRIVATE_REPOS:
return self.all()
return self.for_user(user)

def for_user(self, user):
# Never list all for membership
"""List all organizations where the user is a member or owner."""
if not user.is_authenticated:
return self.none()
return self.filter(
Q(owners__in=[user]) | Q(teams__members__in=[user]),
).distinct()

def for_admin_user(self, user):
"""List all organizations where the user is an owner."""
if not user.is_authenticated:
return self.none()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume these are just optimizations?

return self.filter(owners__in=[user],).distinct()

def created_days_ago(self, days, field='pub_date'):
Expand Down
2 changes: 2 additions & 0 deletions readthedocs/organizations/templatetags/organizations.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ def org_owner(user, obj): # noqa
Any model instance with a relationship with an organization, or an
organization itself.
"""
if not user.is_authenticated:
return False
try:
cls = type(obj)
if cls is Organization:
Expand Down
184 changes: 183 additions & 1 deletion readthedocs/organizations/tests/test_privacy_urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ def setUp(self):
}
)

self.another_user = get(User)

def get_url_path_ctx(self):
return self.default_kwargs

Expand All @@ -35,7 +37,7 @@ def get_url_path_ctx(self):
class NoOrganizationsTest(OrganizationMixin, TestCase):

"""Organization views aren't available if organizations aren't allowed."""

default_status_code = 404

def login(self):
Expand Down Expand Up @@ -74,3 +76,183 @@ def test_public_urls(self):
def test_private_urls(self):
from readthedocs.organizations.urls.private import urlpatterns
self._test_url(urlpatterns)


@override_settings(
RTD_ALLOW_ORGANIZATIONS=True,
ALLOW_PRIVATE_REPOS=False,
)
class AnonymousUserWithPublicOrganizationsTest(OrganizationMixin, TestCase):

"""If organizations are public, an anonymous user can access the public views."""

response_data = {
# Places where we 302 on success.
'/organizations/invite/{hash}/redeem/': {'status_code': 302},
}

def login(self):
pass

def test_public_urls(self):
from readthedocs.organizations.urls.public import urlpatterns
self._test_url(urlpatterns)


@override_settings(
RTD_ALLOW_ORGANIZATIONS=True,
ALLOW_PRIVATE_REPOS=True,
)
class AnonymousUserWithPrivateOrganizationsTest(OrganizationMixin, TestCase):

"""If organizations are private, an anonymous user can't access the public views."""

default_status_code = 404
response_data = {
# Places where we 302 on success.
'/organizations/invite/{hash}/redeem/': {'status_code': 302},
}

def login(self):
pass

def test_public_urls(self):
from readthedocs.organizations.urls.public import urlpatterns
self._test_url(urlpatterns)


@override_settings(
RTD_ALLOW_ORGANIZATIONS=True,
ALLOW_PRIVATE_REPOS=False,
)
class AnonymousUserWithPublicOrganizationsPrivateViewsTest(OrganizationMixin, TestCase):

"""If organizations are public, an anonymous user can't access the private views."""

# We get redirected to the login page.
default_status_code = 302

def login(self):
pass

def test_private_urls(self):
from readthedocs.organizations.urls.private import urlpatterns
self._test_url(urlpatterns)


@override_settings(
RTD_ALLOW_ORGANIZATIONS=True,
ALLOW_PRIVATE_REPOS=True,
)
class AnonymousUserWithPrivateOrganizationsPrivateViewsTest(OrganizationMixin, TestCase):

"""If organizations are private, an anonymous user can't access the private views."""

# We get redirected to the login page.
default_status_code = 302

def login(self):
pass

def test_private_urls(self):
from readthedocs.organizations.urls.private import urlpatterns
self._test_url(urlpatterns)


@override_settings(
RTD_ALLOW_ORGANIZATIONS=True,
ALLOW_PRIVATE_REPOS=False,
)
class AnotherUserWithPublicOrganizationsTest(OrganizationMixin, TestCase):

"""If organizations are public, an user outside the organization can access the public views."""

response_data = {
# Places where we 302 on success.
'/organizations/invite/{hash}/redeem/': {'status_code': 302},
}

def login(self):
self.client.force_login(self.another_user)

def test_public_urls(self):
from readthedocs.organizations.urls.public import urlpatterns
self._test_url(urlpatterns)


@override_settings(
RTD_ALLOW_ORGANIZATIONS=True,
ALLOW_PRIVATE_REPOS=True,
)
class AnotherUserWithPrivateOrganizationsTest(OrganizationMixin, TestCase):

"""If organizations are private, an user outside the organization can't access the public views."""

default_status_code = 404
response_data = {
# Places where we 302 on success.
'/organizations/invite/{hash}/redeem/': {'status_code': 302},
}

def login(self):
self.client.force_login(self.another_user)

def test_public_urls(self):
from readthedocs.organizations.urls.public import urlpatterns
self._test_url(urlpatterns)


@override_settings(
RTD_ALLOW_ORGANIZATIONS=True,
ALLOW_PRIVATE_REPOS=False,
)
class AnotherUserWithPublicOrganizationsPrivateViewsTest(OrganizationMixin, TestCase):

"""If organizations are public, an user outside the organization can access the public views."""

default_status_code = 404
response_data = {
# All users have access to these views.
'/organizations/': {'status_code': 200},
'/organizations/create/': {'status_code': 200},
'/organizations/verify-email/': {'status_code': 200},

# 405's where we should be POST'ing
'/organizations/{slug}/owners/{owner}/delete/': {'status_code': 405},
'/organizations/{slug}/teams/{team}/members/{member}/revoke/': {'status_code': 405},
}

def login(self):
self.client.force_login(self.another_user)

def test_private_urls(self):
from readthedocs.organizations.urls.private import urlpatterns
self._test_url(urlpatterns)


@override_settings(
RTD_ALLOW_ORGANIZATIONS=True,
ALLOW_PRIVATE_REPOS=True,
)
class AnotherUserWithPrivateOrganizationsPrivateViewsTest(OrganizationMixin, TestCase):

"""If organizations are private, an user outside the organization can't access the private views."""

default_status_code = 404
response_data = {
# All users have access to these views.
'/organizations/': {'status_code': 200},
'/organizations/create/': {'status_code': 200},
'/organizations/verify-email/': {'status_code': 200},

# 405's where we should be POST'ing
'/organizations/{slug}/owners/{owner}/delete/': {'status_code': 405},
'/organizations/{slug}/teams/{team}/members/{member}/revoke/': {'status_code': 405},
}

def login(self):
self.client.force_login(self.another_user)

def test_private_urls(self):
from readthedocs.organizations.urls.private import urlpatterns
self._test_url(urlpatterns)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty hard to follow all these tests with such magic. None of them seems to do anything different one from the other (just a setting and URLs). I assume that all this magic is inside this method :/

I assume this is fine, tho, to avoid repetition, but... :(

5 changes: 5 additions & 0 deletions readthedocs/organizations/urls/private.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@
views.CreateOrganizationSignup.as_view(),
name='organization_create',
),
url(
r'^verify-email/$',
views.OrganizationTemplateView.as_view(template_name='organizations/verify_email.html'),
name='organization_verify_email',
),
url(
r'^(?P<slug>[\w.-]+)/edit/$',
views.EditOrganization.as_view(),
Expand Down
5 changes: 0 additions & 5 deletions readthedocs/organizations/urls/public.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,6 @@
from readthedocs.organizations.views import public as views

urlpatterns = [
url(
r'^verify-email/$',
views.OrganizationTemplateView.as_view(template_name='organizations/verify_email.html'),
name='organization_verify_email',
),
url(
r'^(?P<slug>[\w.-]+)/$',
views.DetailOrganization.as_view(),
Expand Down
10 changes: 7 additions & 3 deletions readthedocs/organizations/views/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ def get_organization_queryset(self):
"""
if self.admin_only:
return Organization.objects.for_admin_user(user=self.request.user)
return Organization.objects.for_user(user=self.request.user)
return Organization.objects.public(user=self.request.user)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have the concept of privacy level in Organizations, so I'm not sure these changes make a ton of sense.


@lru_cache(maxsize=1)
def get_organization(self):
Expand Down Expand Up @@ -109,7 +109,11 @@ def get_team_queryset(self):
This will either be team the user is a member of, or teams where the
user is an owner of the organization.
"""
return Team.objects.member(self.request.user).filter(
if self.admin_only:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are we setting this self.admin_only?

queryset = Team.objects.member(self.request.user)
else:
queryset = Team.objects.public(self.request.user)
return queryset.filter(
organization=self.get_organization(),
).order_by('name')

Expand Down Expand Up @@ -141,7 +145,7 @@ class OrganizationView(CheckOrganizationsEnabled):
def get_queryset(self):
if self.admin_only:
return Organization.objects.for_admin_user(user=self.request.user)
return Organization.objects.for_user(user=self.request.user)
return Organization.objects.public(user=self.request.user)

def get_form(self, data=None, files=None, **kwargs):
kwargs['user'] = self.request.user
Expand Down
9 changes: 8 additions & 1 deletion readthedocs/organizations/views/private.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from django.contrib import messages
from django.urls import reverse_lazy
from django.utils.translation import ugettext_lazy as _
from django.views.generic.base import TemplateView
from vanilla import CreateView, DeleteView, ListView, UpdateView

from readthedocs.core.history import UpdateChangeReasonPostView
Expand All @@ -13,13 +14,19 @@
)
from readthedocs.organizations.models import Organization
from readthedocs.organizations.views.base import (
CheckOrganizationsEnabled,
OrganizationOwnerView,
OrganizationTeamMemberView,
OrganizationTeamView,
OrganizationView,
)


class OrganizationTemplateView(PrivateViewMixin, CheckOrganizationsEnabled, TemplateView):

"""Wrapper around `TemplateView` to check if organizations are enabled."""


# Organization views
class CreateOrganizationSignup(PrivateViewMixin, OrganizationView, CreateView):

Expand Down Expand Up @@ -52,9 +59,9 @@ def get_success_url(self):

class ListOrganization(PrivateViewMixin, OrganizationView, ListView):
template_name = 'organizations/organization_list.html'
admin_only = False

def get_queryset(self):
"""Overriden so we always list the organizations of the current user."""
return Organization.objects.for_user(user=self.request.user)


Expand Down
Loading