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

Re-add UserTokenContext, with instance checks #15590

Merged
merged 14 commits into from
Mar 20, 2024
4 changes: 2 additions & 2 deletions tests/unit/accounts/test_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
from warehouse.accounts.tasks import compute_user_metrics
from warehouse.oidc.interfaces import SignedClaims
from warehouse.oidc.models import OIDCPublisher
from warehouse.oidc.utils import OIDCContext
from warehouse.oidc.utils import PublisherTokenContext
from warehouse.rate_limiting import IRateLimiter, RateLimit

from ...common.db.accounts import UserFactory
Expand Down Expand Up @@ -62,7 +62,7 @@ def test_with_oidc_publisher(self, db_request):
assert isinstance(publisher, OIDCPublisher)
claims = SignedClaims({"foo": "bar"})

request = pretend.stub(identity=OIDCContext(publisher, claims))
request = pretend.stub(identity=PublisherTokenContext(publisher, claims))

assert accounts._oidc_publisher(request) is publisher
assert accounts._oidc_claims(request) is claims
Expand Down
4 changes: 2 additions & 2 deletions tests/unit/forklift/test_legacy.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
from warehouse.forklift import legacy
from warehouse.metrics import IMetricsService
from warehouse.oidc.interfaces import SignedClaims
from warehouse.oidc.utils import OIDCContext
from warehouse.oidc.utils import PublisherTokenContext
from warehouse.packaging.interfaces import IFileStorage, IProjectService
from warehouse.packaging.models import (
Dependency,
Expand Down Expand Up @@ -3354,7 +3354,7 @@ def test_upload_succeeds_creates_release(
else:
publisher = GitHubPublisherFactory.create(projects=[project])
claims = {"sha": "somesha"}
identity = OIDCContext(publisher, SignedClaims(claims))
identity = PublisherTokenContext(publisher, SignedClaims(claims))
db_request.oidc_publisher = identity.publisher
db_request.oidc_claims = identity.claims

Expand Down
14 changes: 9 additions & 5 deletions tests/unit/macaroons/test_caveats.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
verify,
)
from warehouse.macaroons.caveats._core import _CaveatRegistry
from warehouse.oidc.utils import OIDCContext
from warehouse.oidc.utils import PublisherTokenContext

from ...common.db.accounts import UserFactory
from ...common.db.oidc import GitHubPublisherFactory
Expand Down Expand Up @@ -295,7 +295,7 @@ def test_verify_no_identity(self):
)

def test_verify_invalid_publisher_id(self, db_request):
identity = OIDCContext(GitHubPublisherFactory.create(), None)
identity = PublisherTokenContext(GitHubPublisherFactory.create(), None)
request = pretend.stub(identity=identity)
request.oidc_publisher = _oidc_publisher(request)

Expand All @@ -307,7 +307,7 @@ def test_verify_invalid_publisher_id(self, db_request):
)

def test_verify_invalid_context(self, db_request):
identity = OIDCContext(GitHubPublisherFactory.create(), None)
identity = PublisherTokenContext(GitHubPublisherFactory.create(), None)
request = pretend.stub(identity=identity)
request.oidc_publisher = _oidc_publisher(request)

Expand All @@ -322,7 +322,9 @@ def test_verify_invalid_project(self, db_request):

# This OIDC publisher is only registered to "foobar", so it should
# not verify a caveat presented for "foobaz".
identity = OIDCContext(GitHubPublisherFactory.create(projects=[foobar]), None)
identity = PublisherTokenContext(
GitHubPublisherFactory.create(projects=[foobar]), None
)
request = pretend.stub(identity=identity)
request.oidc_publisher = _oidc_publisher(request)
caveat = OIDCPublisher(oidc_publisher_id=str(request.oidc_publisher.id))
Expand All @@ -336,7 +338,9 @@ def test_verify_ok(self, db_request):

# This OIDC publisher is only registered to "foobar", so it should
# not verify a caveat presented for "foobaz".
identity = OIDCContext(GitHubPublisherFactory.create(projects=[foobar]), None)
identity = PublisherTokenContext(
GitHubPublisherFactory.create(projects=[foobar]), None
)
request = pretend.stub(identity=identity)
request.oidc_publisher = _oidc_publisher(request)
caveat = OIDCPublisher(oidc_publisher_id=str(request.oidc_publisher.id))
Expand Down
7 changes: 4 additions & 3 deletions tests/unit/macaroons/test_security_policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,13 @@
from zope.interface.verify import verifyClass

from warehouse.accounts.interfaces import IUserService
from warehouse.accounts.utils import UserTokenContext
from warehouse.authnz import Permissions
from warehouse.macaroons import security_policy
from warehouse.macaroons.interfaces import IMacaroonService
from warehouse.macaroons.services import InvalidMacaroonError
from warehouse.oidc.interfaces import SignedClaims
from warehouse.oidc.utils import OIDCContext
from warehouse.oidc.utils import PublisherTokenContext


@pytest.mark.parametrize(
Expand Down Expand Up @@ -214,7 +215,7 @@ def test_identity_user(self, monkeypatch):
),
)

assert policy.identity(request) is user
assert policy.identity(request) == UserTokenContext(user, macaroon)
assert extract_http_macaroon.calls == [pretend.call(request)]
assert request.find_service.calls == [
pretend.call(IMacaroonService, context=None),
Expand Down Expand Up @@ -258,7 +259,7 @@ def test_identity_oidc_publisher(self, monkeypatch):
identity = policy.identity(request)
assert identity
assert identity.publisher is oidc_publisher
assert identity == OIDCContext(
assert identity == PublisherTokenContext(
oidc_publisher, SignedClaims(oidc_additional["oidc"])
)

Expand Down
24 changes: 24 additions & 0 deletions tests/unit/macaroons/test_utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

import pretend

from tests.common.db.accounts import UserFactory
from warehouse.accounts.utils import UserTokenContext
from warehouse.utils.security_policy import principals_for


def test_usertoken_context_principals(db_request):
user = UserFactory.create()
assert principals_for(
UserTokenContext(user=user, macaroon=pretend.stub())
) == principals_for(user)
2 changes: 1 addition & 1 deletion tests/unit/oidc/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ def test_find_publisher_by_issuer_activestate(

def test_oidc_context_principals():
assert principals_for(
utils.OIDCContext(publisher=pretend.stub(id=17), claims=None)
utils.PublisherTokenContext(publisher=pretend.stub(id=17), claims=None)
) == [
Authenticated,
"oidc:17",
Expand Down
19 changes: 13 additions & 6 deletions warehouse/accounts/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,10 @@
database_login_factory,
)
from warehouse.accounts.tasks import compute_user_metrics
from warehouse.accounts.utils import UserTokenContext
from warehouse.admin.flags import AdminFlagValue
from warehouse.macaroons.security_policy import MacaroonSecurityPolicy
from warehouse.oidc.utils import OIDCContext
from warehouse.oidc.utils import PublisherTokenContext
from warehouse.organizations.services import IOrganizationService
from warehouse.rate_limiting import IRateLimiter, RateLimit
from warehouse.utils.security_policy import MultiSecurityPolicy
Expand All @@ -54,23 +55,29 @@ def _user(request):
if request.identity is None:
return None

if not isinstance(request.identity, User):
if isinstance(request.identity, UserTokenContext):
# A UserTokenContext signals a user-created API token;
# take the underlying user.
return request.identity.user
elif isinstance(request.identity, User):
return request.identity
else:
di marked this conversation as resolved.
Show resolved Hide resolved
return None

return request.identity


def _oidc_publisher(request):
return (
request.identity.publisher
if isinstance(request.identity, OIDCContext)
if isinstance(request.identity, PublisherTokenContext)
else None
)


def _oidc_claims(request):
return (
request.identity.claims if isinstance(request.identity, OIDCContext) else None
request.identity.claims
if isinstance(request.identity, PublisherTokenContext)
else None
)


Expand Down
4 changes: 3 additions & 1 deletion warehouse/accounts/security_policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,9 @@ def permits(self, request, context, permission):

def _permits_for_user_policy(acl, request, context, permission):
# It should only be possible for request.identity to be a User object
# at this point, and we only a User in these policies.
# at this point, and we only allow a User in these policies.
# Note that UserTokenContext is not allowed here, since a UserTokenContext
# can only appear in an API-token-authenticated request, not a session.
assert isinstance(request.identity, User)

# Dispatch to our ACL
Expand Down
44 changes: 44 additions & 0 deletions warehouse/accounts/utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

from __future__ import annotations

from dataclasses import dataclass

from warehouse.accounts.models import User
from warehouse.macaroons.models import Macaroon


@dataclass
class UserTokenContext:
"""
This class supports `MacaroonSecurityPolicy` in
`warehouse.macaroons.security_policy`.

It is a wrapper containing both the user associated with an API token
authenticated request and its corresponding Macaroon. We use it to smuggle
the Macaroon associated to the API token through to a session. `request.identity`
in an API token authenticated request should return this type.
"""

user: User
"""
The associated user.
"""

macaroon: Macaroon
"""
The Macaroon associated to the API token used to authenticate.
"""

def __principals__(self) -> list[str]:
return self.user.__principals__()
4 changes: 2 additions & 2 deletions warehouse/macaroons/caveats/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
# limitations under the License.

import time

from typing import Any

from pydantic import StrictInt, StrictStr
Expand All @@ -22,6 +21,7 @@
from pyramid.security import Allowed

from warehouse.accounts.models import User
from warehouse.accounts.utils import UserTokenContext
from warehouse.errors import WarehouseDenied
from warehouse.macaroons.caveats._core import (
Caveat,
Expand Down Expand Up @@ -104,7 +104,7 @@ class RequestUser(Caveat):
user_id: StrictStr

def verify(self, request: Request, context: Any, permission: str) -> Result:
if not isinstance(request.identity, User):
if not isinstance(request.identity, UserTokenContext):
di marked this conversation as resolved.
Show resolved Hide resolved
return Failure("token with user restriction without a user")

if str(request.identity.id) != self.user_id:
woodruffw marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
7 changes: 4 additions & 3 deletions warehouse/macaroons/security_policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,14 @@
from zope.interface import implementer

from warehouse.accounts.interfaces import IUserService
from warehouse.accounts.utils import UserTokenContext
from warehouse.authnz import Permissions
from warehouse.cache.http import add_vary_callback
from warehouse.errors import WarehouseDenied
from warehouse.macaroons import InvalidMacaroonError
from warehouse.macaroons.interfaces import IMacaroonService
from warehouse.metrics.interfaces import IMetricsService
from warehouse.oidc.utils import OIDCContext
from warehouse.oidc.utils import PublisherTokenContext
from warehouse.utils.security_policy import AuthenticationMethod, principals_for


Expand Down Expand Up @@ -119,9 +120,9 @@ def identity(self, request):
is_disabled, _ = login_service.is_disabled(dm.user.id)
if is_disabled:
return None
return dm.user
return UserTokenContext(dm.user, dm)

return OIDCContext(dm.oidc_publisher, oidc_claims)
return PublisherTokenContext(dm.oidc_publisher, oidc_claims)

def remember(self, request, userid, **kw):
# This is a NO-OP because our Macaroon header policy doesn't allow
Expand Down
2 changes: 1 addition & 1 deletion warehouse/oidc/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ def find_publisher_by_issuer(


@dataclass
class OIDCContext:
class PublisherTokenContext:
"""
This class supports `MacaroonSecurityPolicy` in
`warehouse.macaroons.security_policy`.
Expand Down
3 changes: 3 additions & 0 deletions warehouse/utils/security_policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from zope.interface import implementer

from warehouse.accounts.models import User
from warehouse.accounts.utils import UserTokenContext


# NOTE: Is there a better place for this to live? It may not even need to exist
Expand Down Expand Up @@ -84,6 +85,8 @@ def authenticated_userid(self, request):
# more correct pattern before fixing this.
if isinstance(ident, User):
return str(ident.id)
elif isinstance(ident, UserTokenContext):
return str(ident.user.id)
di marked this conversation as resolved.
Show resolved Hide resolved
return None

def forget(self, request, **kw):
Expand Down