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 all commits
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
18 changes: 17 additions & 1 deletion readthedocs/organizations/querysets.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,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
16 changes: 6 additions & 10 deletions readthedocs/organizations/tests/test_access.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,7 @@
from django.contrib.auth.models import User
from django.test import TestCase, override_settings

from readthedocs.organizations.models import (
Organization,
OrganizationOwner,
Team,
)
from readthedocs.organizations.models import Organization, OrganizationOwner, Team
from readthedocs.projects.models import Project
from readthedocs.rtd_tests.utils import create_user

Expand Down Expand Up @@ -34,7 +30,7 @@ def assertResponse(self, path, method=None, data=None, **kwargs):
response_attrs.update(kwargs)
response_attrs.update(self.url_responses.get(path, {}))
for (key, val) in list(response_attrs.items()):
self.assertEqual(getattr(response, key), val)
self.assertEqual(getattr(response, key), val, path)
return response

def setUp(self):
Expand Down Expand Up @@ -188,7 +184,7 @@ def test_organization_teams(self):
self.assertEqual(self.organization.teams.count(), 1)


@override_settings(RTD_ALLOW_ORGANIZATIONS=True)
@override_settings(RTD_ALLOW_ORGANIZATIONS=True, ALLOW_PRIVATE_REPOS=True)
class OrganizationOwnerAccess(OrganizationAccessMixin, TestCase):

"""Test organization paths with authed org owner."""
Expand All @@ -200,7 +196,7 @@ def is_admin(self):
return True


@override_settings(RTD_ALLOW_ORGANIZATIONS=True)
@override_settings(RTD_ALLOW_ORGANIZATIONS=True, ALLOW_PRIVATE_REPOS=True)
class OrganizationMemberAccess(OrganizationAccessMixin, TestCase):

"""Test organization paths with authed org member."""
Expand All @@ -226,13 +222,13 @@ def is_admin(self):
return False


@override_settings(RTD_ALLOW_ORGANIZATIONS=True)
@override_settings(RTD_ALLOW_ORGANIZATIONS=True, ALLOW_PRIVATE_REPOS=True)
class OrganizationNonmemberAccess(OrganizationAccessMixin, TestCase):

"""Test organization paths with authed but non-org user."""

url_responses = {
'/organizations/': {'status_code': 200},
"/organizations/": {"status_code": 200},
}

def assertResponse(self, path, method=None, data=None, **kwargs):
Expand Down
196 changes: 195 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,195 @@ 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... :(

7 changes: 7 additions & 0 deletions readthedocs/organizations/urls/private.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,13 @@
views.CreateOrganizationSignup.as_view(),
name='organization_create',
),
re_path(
r"^verify-email/$",
views.OrganizationTemplateView.as_view(
template_name="organizations/verify_email.html"
),
name="organization_verify_email",
),
re_path(
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 = [
re_path(
r'^verify-email/$',
views.OrganizationTemplateView.as_view(template_name='organizations/verify_email.html'),
name='organization_verify_email',
),
re_path(
r'^(?P<slug>[\w.-]+)/$',
views.DetailOrganization.as_view(),
Expand Down
Loading