Skip to content

Commit

Permalink
fix(org-tokens): Ensure tokens cannot be generated without url (#57123)
Browse files Browse the repository at this point in the history
This PR adjusts the org auth token creation to fail if no `url` can be
found.

According to the org auth token spec, `url` is required and should
always be there.
However, on self hosted instances the underlying `system.url-prefix`
config _may_ not be set. This could result in users having org tokens
without this config.

This adjusts the org token creation to fail if no url can be found.
While at it, I actually also adjusted the error response format to the
actual format `detail: str` vs `detail: [str]`, and updated the UI to
show a reasonable error message if that happens:

---------

Co-authored-by: Mark Story <mark@mark-story.com>
Co-authored-by: Riccardo Busetti <riccardo.busetti@sentry.io>
  • Loading branch information
3 people authored Oct 2, 2023
1 parent 6ce3206 commit d7509e2
Show file tree
Hide file tree
Showing 9 changed files with 140 additions and 52 deletions.
4 changes: 2 additions & 2 deletions src/sentry/api/endpoints/org_auth_token_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,11 @@ def put(
name = request.data.get("name")

if not name:
return Response({"detail": ["The name cannot be blank."]}, status=400)
return Response({"detail": "The name cannot be blank."}, status=400)

if len(name) > MAX_NAME_LENGTH:
return Response(
{"detail": ["The name cannot be longer than 255 characters."]}, status=400
{"detail": "The name cannot be longer than 255 characters."}, status=400
)

instance.update(name=name)
Expand Down
24 changes: 20 additions & 4 deletions src/sentry/api/endpoints/org_auth_tokens.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,11 @@
RpcOrganization,
RpcUserOrganizationContext,
)
from sentry.utils.security.orgauthtoken_token import generate_token, hash_token
from sentry.utils.security.orgauthtoken_token import (
SystemUrlPrefixMissingException,
generate_token,
hash_token,
)


@control_silo_endpoint
Expand Down Expand Up @@ -62,18 +66,30 @@ def post(
organization_context: RpcUserOrganizationContext,
organization: RpcOrganization,
) -> Response:
token_str = generate_token(organization.slug, generate_region_url())
try:
token_str = generate_token(organization.slug, generate_region_url())
except SystemUrlPrefixMissingException:
return Response(
{
"detail": {
"message": "system.url-prefix is not set. You need to set this to generate a token.",
"code": "missing_system_url_prefix",
}
},
status=400,
)

token_hashed = hash_token(token_str)

name = request.data.get("name")

# Main validation cases with specific error messages
if not name:
return Response({"detail": ["The name cannot be blank."]}, status=400)
return Response({"detail": "The name cannot be blank."}, status=400)

if len(name) > MAX_NAME_LENGTH:
return Response(
{"detail": ["The name cannot be longer than 255 characters."]}, status=400
{"detail": "The name cannot be longer than 255 characters."}, status=400
)

token = OrgAuthToken.objects.create(
Expand Down
12 changes: 9 additions & 3 deletions src/sentry/models/orgauthtoken.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,11 @@ def write_relocation_import(
# TODO(getsentry/team-ospo#190): Prevents a circular import; could probably split up the
# source module in such a way that this is no longer an issue.
from sentry.api.utils import generate_region_url
from sentry.utils.security.orgauthtoken_token import generate_token, hash_token
from sentry.utils.security.orgauthtoken_token import (
SystemUrlPrefixMissingException,
generate_token,
hash_token,
)

# If there is a token collision, or the token does not exist for some reason, generate a new
# one.
Expand All @@ -101,8 +105,10 @@ def write_relocation_import(
).first()
if org_mapping is None:
return None

token_str = generate_token(org_mapping.slug, generate_region_url())
try:
token_str = generate_token(org_mapping.slug, generate_region_url())
except SystemUrlPrefixMissingException:
return None
self.token_hashed = hash_token(token_str)
self.token_last_characters = token_str[-4:]

Expand Down
14 changes: 11 additions & 3 deletions src/sentry/utils/security/orgauthtoken_token.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,23 @@
from base64 import b64decode, b64encode
from datetime import datetime

from django.conf import settings

from sentry import options
from sentry.utils import hashlib, json

SENTRY_ORG_AUTH_TOKEN_PREFIX = "sntrys_"


class SystemUrlPrefixMissingException(Exception):
# system.url-prefix is not set. You need to set this to generate a token.
pass


def generate_token(org_slug: str, region_url: str):
sentry_url = settings.SENTRY_OPTIONS.get("system.url-prefix")
sentry_url = options.get("system.url-prefix")

if sentry_url is None:
raise SystemUrlPrefixMissingException

payload = {
"iat": datetime.utcnow().timestamp(),
"url": sentry_url,
Expand Down
81 changes: 49 additions & 32 deletions src/sentry/web/frontend/setup_wizard.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,13 @@
ProjectKeyStatus,
)
from sentry.models.orgauthtoken import OrgAuthToken
from sentry.models.user import User
from sentry.utils.http import absolute_uri
from sentry.utils.security.orgauthtoken_token import generate_token, hash_token
from sentry.utils.security.orgauthtoken_token import (
SystemUrlPrefixMissingException,
generate_token,
hash_token,
)
from sentry.utils.urls import add_params_to_url
from sentry.web.frontend.base import BaseView
from sentry.web.helpers import render_to_response
Expand Down Expand Up @@ -93,37 +98,7 @@ def get(self, request: HttpRequest, wizard_hash) -> HttpResponse:
filled_projects.append(enriched_project)

# Fetching or creating a token
token = None
serialized_token = None

can_use_org_tokens = len(orgs) == 1

if can_use_org_tokens:
org = orgs[0]
token_str = generate_token(org.slug, generate_region_url())
token_hashed = hash_token(token_str)
token = OrgAuthToken.objects.create(
name=f"Generated by Sentry Wizard on {date.today()}",
organization_id=org.id,
scope_list=["org:ci"],
created_by_id=request.user.id,
token_last_characters=token_str[-4:],
token_hashed=token_hashed,
)
serialized_token = serialize(token, request.user, token=token_str)
else:
tokens = ApiToken.objects.filter(user=request.user)
token = next(
(token for token in tokens if "project:releases" in token.get_scopes()), None
)
if token is None:
token = ApiToken.objects.create(
user=request.user,
scope_list=["project:releases"],
refresh_token=None,
expires_at=None,
)
serialized_token = serialize(token)
serialized_token = get_token(orgs, request.user)

result = {"apiKeys": serialized_token, "projects": filled_projects}

Expand All @@ -132,3 +107,45 @@ def get(self, request: HttpRequest, wizard_hash) -> HttpResponse:

context["organizations"] = serialize(list(orgs))
return render_to_response("sentry/setup-wizard.html", context, request)


def get_token(orgs: list[Organization], user: User):
can_use_org_tokens = len(orgs) == 1

# If only one org, try to generate an org auth token
if can_use_org_tokens:
org = orgs[0]
token = get_org_token(org, user)

if token is not None:
return token

# Otherwise, generate a user token
tokens = ApiToken.objects.filter(user=user)
token = next((token for token in tokens if "project:releases" in token.get_scopes()), None)
if token is None:
token = ApiToken.objects.create(
user=user,
scope_list=["project:releases"],
refresh_token=None,
expires_at=None,
)
return serialize(token)


def get_org_token(org: Organization, user: User):
try:
token_str = generate_token(org.slug, generate_region_url())
except SystemUrlPrefixMissingException:
return None

token_hashed = hash_token(token_str)
token = OrgAuthToken.objects.create(
name=f"Generated by Sentry Wizard on {date.today()}",
organization_id=org.id,
scope_list=["org:ci"],
created_by_id=user.id,
token_last_characters=token_str[-4:],
token_hashed=token_hashed,
)
return serialize(token, user, token=token_str)
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ describe('OrganizationAuthTokensNewAuthToken', function () {
url: ENDPOINT,
method: 'POST',
body: {
details: ['Test API error occurred.'],
detail: 'Test API error occurred.',
},
statusCode: 400,
});
Expand All @@ -94,4 +94,37 @@ describe('OrganizationAuthTokensNewAuthToken', function () {
})
);
});

it('handles missing_system_url_prefix API error when creating token', async function () {
jest.spyOn(indicators, 'addErrorMessage');

render(<OrganizationAuthTokensNewAuthToken {...defaultProps} />);

const mock = MockApiClient.addMockResponse({
url: ENDPOINT,
method: 'POST',
body: {
detail: {message: 'test message', code: 'missing_system_url_prefix'},
},
statusCode: 400,
});

expect(screen.queryByLabelText('Generated token')).not.toBeInTheDocument();

await userEvent.type(screen.getByLabelText('Name'), 'My Token');
await userEvent.click(screen.getByRole('button', {name: 'Create Auth Token'}));

expect(screen.queryByLabelText('Generated token')).not.toBeInTheDocument();

expect(indicators.addErrorMessage).toHaveBeenCalledWith(
'You have to configure `system.url-prefix` in your Sentry instance in order to generate tokens.'
);

expect(mock).toHaveBeenCalledWith(
ENDPOINT,
expect.objectContaining({
data: {name: 'My Token'},
})
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,15 @@ function AuthTokenCreateForm({
onCreatedToken(token);
},
onError: error => {
const message = t('Failed to create a new auth token.');
const detail = error.responseJSON?.detail;
const code = detail && typeof detail === 'object' ? detail.code : undefined;

const message =
code === 'missing_system_url_prefix'
? t(
'You have to configure `system.url-prefix` in your Sentry instance in order to generate tokens.'
)
: t('Failed to create a new auth token.');
handleXhrErrorResponse(message, error);
addErrorMessage(message);
},
Expand Down
6 changes: 3 additions & 3 deletions tests/sentry/api/endpoints/test_org_auth_token_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ def test_no_name(self):
self.organization.slug, token.id, status_code=status.HTTP_400_BAD_REQUEST, **payload
)
assert response.content
assert response.data == {"detail": ["The name cannot be blank."]}
assert response.data == {"detail": "The name cannot be blank."}

tokenNew = OrgAuthToken.objects.get(id=token.id)
assert tokenNew.name == "token 1"
Expand All @@ -189,7 +189,7 @@ def test_name_too_long(self):
self.organization.slug, token.id, status_code=status.HTTP_400_BAD_REQUEST, **payload
)
assert response.content
assert response.data == {"detail": ["The name cannot be longer than 255 characters."]}
assert response.data == {"detail": "The name cannot be longer than 255 characters."}

tokenNew = OrgAuthToken.objects.get(id=token.id)
assert tokenNew.name == "token 1"
Expand All @@ -210,7 +210,7 @@ def test_blank_name(self):
self.organization.slug, token.id, status_code=status.HTTP_400_BAD_REQUEST, **payload
)
assert response.content
assert response.data == {"detail": ["The name cannot be blank."]}
assert response.data == {"detail": "The name cannot be blank."}

tokenNew = OrgAuthToken.objects.get(id=token.id)
assert tokenNew.name == "token 1"
Expand Down
6 changes: 3 additions & 3 deletions tests/sentry/api/endpoints/test_org_auth_tokens.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ def test_no_name(self):
self.organization.slug, status_code=status.HTTP_400_BAD_REQUEST, **payload
)
assert response.content
assert response.data == {"detail": ["The name cannot be blank."]}
assert response.data == {"detail": "The name cannot be blank."}

def test_blank_name(self):
payload = {"name": ""}
Expand All @@ -154,7 +154,7 @@ def test_blank_name(self):
self.organization.slug, status_code=status.HTTP_400_BAD_REQUEST, **payload
)
assert response.content
assert response.data == {"detail": ["The name cannot be blank."]}
assert response.data == {"detail": "The name cannot be blank."}

def test_name_too_long(self):
payload = {"name": "a" * 300}
Expand All @@ -164,7 +164,7 @@ def test_name_too_long(self):
self.organization.slug, status_code=status.HTTP_400_BAD_REQUEST, **payload
)
assert response.content
assert response.data == {"detail": ["The name cannot be longer than 255 characters."]}
assert response.data == {"detail": "The name cannot be longer than 255 characters."}

def test_no_auth(self):
response = self.get_error_response(self.organization.slug)
Expand Down

0 comments on commit d7509e2

Please sign in to comment.