Skip to content

Commit

Permalink
Integration tests (#2256)
Browse files Browse the repository at this point in the history
* initial commit

* almost done

* finished 3 tests

* minor refactor

* built out initial permisison tests

* reworked test_deletion

* removed logging

* all original tests have been converted

* renamed user_groups to user_group

* mypy

* added test for doc set permissions

* unified naming for manager methods

* Refactored models and added new deletion test

* minor additions

* better logging+fixed input variables

* commented out failed tests

* Added readme

* readme update

* Added auth to IT

set auth_type to basic and require_email_verification to false

* Update run-it.yml

* used verify and added to readme

* added api key manager
  • Loading branch information
hagen-danswer authored Sep 1, 2024
1 parent 634de83 commit 8d443ad
Show file tree
Hide file tree
Showing 40 changed files with 2,881 additions and 603 deletions.
2 changes: 2 additions & 0 deletions .github/workflows/run-it.yml
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ jobs:
run: |
cd deployment/docker_compose
ENABLE_PAID_ENTERPRISE_EDITION_FEATURES=true \
AUTH_TYPE=basic \
REQUIRE_EMAIL_VERIFICATION=false \
IMAGE_TAG=it \
docker compose -f docker-compose.dev.yml -p danswer-stack up -d --build
id: start_docker
Expand Down
17 changes: 0 additions & 17 deletions backend/danswer/auth/users.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,23 +67,6 @@
logger = setup_logger()


def validate_curator_request(groups: list | None, is_public: bool) -> None:
if is_public:
detail = "Curators cannot create public objects"
logger.error(detail)
raise HTTPException(
status_code=401,
detail=detail,
)
if not groups:
detail = "Curators must specify 1+ groups"
logger.error(detail)
raise HTTPException(
status_code=401,
detail=detail,
)


def is_user_admin(user: User | None) -> bool:
if AUTH_TYPE == AuthType.DISABLED:
return True
Expand Down
12 changes: 8 additions & 4 deletions backend/danswer/db/connector_credential_pair.py
Original file line number Diff line number Diff line change
Expand Up @@ -334,9 +334,13 @@ def add_credential_to_connector(
raise HTTPException(status_code=404, detail="Connector does not exist")

if credential is None:
error_msg = (
f"Credential {credential_id} does not exist or does not belong to user"
)
logger.error(error_msg)
raise HTTPException(
status_code=401,
detail="Credential does not exist or does not belong to user",
detail=error_msg,
)

existing_association = (
Expand All @@ -350,7 +354,7 @@ def add_credential_to_connector(
if existing_association is not None:
return StatusResponse(
success=False,
message=f"Connector already has Credential {credential_id}",
message=f"Connector {connector_id} already has Credential {credential_id}",
data=connector_id,
)

Expand All @@ -374,8 +378,8 @@ def add_credential_to_connector(
db_session.commit()

return StatusResponse(
success=False,
message=f"Connector already has Credential {credential_id}",
success=True,
message=f"Creating new association between Connector {connector_id} and Credential {credential_id}",
data=association.id,
)

Expand Down
19 changes: 8 additions & 11 deletions backend/danswer/server/documents/cc_pair.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
from fastapi import APIRouter
from fastapi import Depends
from fastapi import HTTPException
from pydantic import BaseModel
from sqlalchemy.exc import IntegrityError
from sqlalchemy.orm import Session

Expand All @@ -21,12 +20,13 @@
from danswer.db.index_attempt import cancel_indexing_attempts_past_model
from danswer.db.index_attempt import get_index_attempts_for_connector
from danswer.db.models import User
from danswer.db.models import UserRole
from danswer.server.documents.models import CCPairFullInfo
from danswer.server.documents.models import CCStatusUpdateRequest
from danswer.server.documents.models import ConnectorCredentialPairIdentifier
from danswer.server.documents.models import ConnectorCredentialPairMetadata
from danswer.server.models import StatusResponse
from danswer.utils.logger import setup_logger
from ee.danswer.db.user_group import validate_user_creation_permissions

logger = setup_logger()

Expand Down Expand Up @@ -84,10 +84,6 @@ def get_cc_pair_full_info(
)


class CCStatusUpdateRequest(BaseModel):
status: ConnectorCredentialPairStatus


@router.put("/admin/cc-pair/{cc_pair_id}/status")
def update_cc_pair_status(
cc_pair_id: int,
Expand Down Expand Up @@ -157,11 +153,12 @@ def associate_credential_to_connector(
user: User | None = Depends(current_curator_or_admin_user),
db_session: Session = Depends(get_session),
) -> StatusResponse[int]:
if user and user.role != UserRole.ADMIN and metadata.is_public:
raise HTTPException(
status_code=400,
detail="Public connections cannot be created by non-admin users",
)
validate_user_creation_permissions(
db_session=db_session,
user=user,
target_group_ids=metadata.groups,
object_is_public=metadata.is_public,
)

try:
response = add_credential_to_connector(
Expand Down
53 changes: 18 additions & 35 deletions backend/danswer/server/documents/connector.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@
from danswer.file_store.file_store import get_default_file_store
from danswer.server.documents.models import AuthStatus
from danswer.server.documents.models import AuthUrl
from danswer.server.documents.models import ConnectorBase
from danswer.server.documents.models import ConnectorCredentialPairIdentifier
from danswer.server.documents.models import ConnectorIndexingStatus
from danswer.server.documents.models import ConnectorSnapshot
Expand All @@ -93,6 +92,7 @@
from danswer.server.documents.models import RunConnectorRequest
from danswer.server.models import StatusResponse
from danswer.utils.logger import setup_logger
from ee.danswer.db.user_group import validate_user_creation_permissions

logger = setup_logger()

Expand Down Expand Up @@ -514,35 +514,6 @@ def _validate_connector_allowed(source: DocumentSource) -> None:
)


def _check_connector_permissions(
connector_data: ConnectorUpdateRequest, user: User | None
) -> ConnectorBase:
"""
This is not a proper permission check, but this should prevent curators creating bad situations
until a long-term solution is implemented (Replacing CC pairs/Connectors with Connections)
"""
if user and user.role != UserRole.ADMIN:
if connector_data.is_public:
raise HTTPException(
status_code=400,
detail="Public connectors can only be created by admins",
)
if not connector_data.groups:
raise HTTPException(
status_code=400,
detail="Connectors created by curators must have groups",
)
return ConnectorBase(
name=connector_data.name,
source=connector_data.source,
input_type=connector_data.input_type,
connector_specific_config=connector_data.connector_specific_config,
refresh_freq=connector_data.refresh_freq,
prune_freq=connector_data.prune_freq,
indexing_start=connector_data.indexing_start,
)


@router.post("/admin/connector")
def create_connector_from_model(
connector_data: ConnectorUpdateRequest,
Expand All @@ -551,13 +522,19 @@ def create_connector_from_model(
) -> ObjectCreationIdResponse:
try:
_validate_connector_allowed(connector_data.source)
connector_base = _check_connector_permissions(connector_data, user)

validate_user_creation_permissions(
db_session=db_session,
user=user,
target_group_ids=connector_data.groups,
object_is_public=connector_data.is_public,
)
connector_base = connector_data.to_connector_base()
return create_connector(
db_session=db_session,
connector_data=connector_base,
)
except ValueError as e:
logger.error(f"Error creating connector: {e}")
raise HTTPException(status_code=400, detail=str(e))


Expand Down Expand Up @@ -608,12 +585,18 @@ def create_connector_with_mock_credential(
def update_connector_from_model(
connector_id: int,
connector_data: ConnectorUpdateRequest,
user: User = Depends(current_admin_user),
user: User = Depends(current_curator_or_admin_user),
db_session: Session = Depends(get_session),
) -> ConnectorSnapshot | StatusResponse[int]:
try:
_validate_connector_allowed(connector_data.source)
connector_base = _check_connector_permissions(connector_data, user)
validate_user_creation_permissions(
db_session=db_session,
user=user,
target_group_ids=connector_data.groups,
object_is_public=connector_data.is_public,
)
connector_base = connector_data.to_connector_base()
except ValueError as e:
raise HTTPException(status_code=400, detail=str(e))

Expand Down Expand Up @@ -643,7 +626,7 @@ def update_connector_from_model(
@router.delete("/admin/connector/{connector_id}", response_model=StatusResponse[int])
def delete_connector_by_id(
connector_id: int,
_: User = Depends(current_admin_user),
_: User = Depends(current_curator_or_admin_user),
db_session: Session = Depends(get_session),
) -> StatusResponse[int]:
try:
Expand Down
23 changes: 10 additions & 13 deletions backend/danswer/server/documents/credential.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
from danswer.auth.users import current_admin_user
from danswer.auth.users import current_curator_or_admin_user
from danswer.auth.users import current_user
from danswer.auth.users import validate_curator_request
from danswer.db.credentials import alter_credential
from danswer.db.credentials import create_credential
from danswer.db.credentials import CREDENTIAL_PERMISSIONS_TO_IGNORE
Expand All @@ -20,14 +19,14 @@
from danswer.db.engine import get_session
from danswer.db.models import DocumentSource
from danswer.db.models import User
from danswer.db.models import UserRole
from danswer.server.documents.models import CredentialBase
from danswer.server.documents.models import CredentialDataUpdateRequest
from danswer.server.documents.models import CredentialSnapshot
from danswer.server.documents.models import CredentialSwapRequest
from danswer.server.documents.models import ObjectCreationIdResponse
from danswer.server.models import StatusResponse
from danswer.utils.logger import setup_logger
from ee.danswer.db.user_group import validate_user_creation_permissions

logger = setup_logger()

Expand Down Expand Up @@ -80,7 +79,7 @@ def get_cc_source_full_info(
]


@router.get("/credentials/{id}")
@router.get("/credential/{id}")
def list_credentials_by_id(
user: User | None = Depends(current_user),
db_session: Session = Depends(get_session),
Expand All @@ -105,7 +104,7 @@ def delete_credential_by_id_admin(
)


@router.put("/admin/credentials/swap")
@router.put("/admin/credential/swap")
def swap_credentials_for_connector(
credential_swap_req: CredentialSwapRequest,
user: User | None = Depends(current_user),
Expand All @@ -131,14 +130,12 @@ def create_credential_from_model(
user: User | None = Depends(current_curator_or_admin_user),
db_session: Session = Depends(get_session),
) -> ObjectCreationIdResponse:
if (
user
and user.role != UserRole.ADMIN
and not _ignore_credential_permissions(credential_info.source)
):
validate_curator_request(
groups=credential_info.groups,
is_public=credential_info.curator_public,
if not _ignore_credential_permissions(credential_info.source):
validate_user_creation_permissions(
db_session=db_session,
user=user,
target_group_ids=credential_info.groups,
object_is_public=credential_info.curator_public,
)

credential = create_credential(credential_info, user, db_session)
Expand Down Expand Up @@ -179,7 +176,7 @@ def get_credential_by_id(
return CredentialSnapshot.from_credential_db_model(credential)


@router.put("/admin/credentials/{credential_id}")
@router.put("/admin/credential/{credential_id}")
def update_credential_data(
credential_id: int,
credential_update: CredentialDataUpdateRequest,
Expand Down
14 changes: 8 additions & 6 deletions backend/danswer/server/documents/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,12 @@ class ConnectorBase(BaseModel):


class ConnectorUpdateRequest(ConnectorBase):
is_public: bool | None = None
is_public: bool = True
groups: list[int] = Field(default_factory=list)

def to_connector_base(self) -> ConnectorBase:
return ConnectorBase(**self.model_dump(exclude={"is_public", "groups"}))


class ConnectorSnapshot(ConnectorBase):
id: int
Expand Down Expand Up @@ -103,11 +106,6 @@ class CredentialSnapshot(CredentialBase):
user_id: UUID | None
time_created: datetime
time_updated: datetime
name: str | None
source: DocumentSource
credential_json: dict[str, Any]
admin_public: bool
curator_public: bool

@classmethod
def from_credential_db_model(cls, credential: Credential) -> "CredentialSnapshot":
Expand Down Expand Up @@ -261,6 +259,10 @@ class ConnectorCredentialPairMetadata(BaseModel):
groups: list[int] = Field(default_factory=list)


class CCStatusUpdateRequest(BaseModel):
status: ConnectorCredentialPairStatus


class ConnectorCredentialPairDescriptor(BaseModel):
id: int
name: str | None = None
Expand Down
25 changes: 13 additions & 12 deletions backend/danswer/server/features/document_set/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,19 @@

from danswer.auth.users import current_curator_or_admin_user
from danswer.auth.users import current_user
from danswer.auth.users import validate_curator_request
from danswer.db.document_set import check_document_sets_are_public
from danswer.db.document_set import fetch_all_document_sets_for_user
from danswer.db.document_set import insert_document_set
from danswer.db.document_set import mark_document_set_as_to_be_deleted
from danswer.db.document_set import update_document_set
from danswer.db.engine import get_session
from danswer.db.models import User
from danswer.db.models import UserRole
from danswer.server.features.document_set.models import CheckDocSetPublicRequest
from danswer.server.features.document_set.models import CheckDocSetPublicResponse
from danswer.server.features.document_set.models import DocumentSet
from danswer.server.features.document_set.models import DocumentSetCreationRequest
from danswer.server.features.document_set.models import DocumentSetUpdateRequest
from ee.danswer.db.user_group import validate_user_creation_permissions


router = APIRouter(prefix="/manage")
Expand All @@ -31,11 +30,12 @@ def create_document_set(
user: User = Depends(current_curator_or_admin_user),
db_session: Session = Depends(get_session),
) -> int:
if user and user.role != UserRole.ADMIN:
validate_curator_request(
groups=document_set_creation_request.groups,
is_public=document_set_creation_request.is_public,
)
validate_user_creation_permissions(
db_session=db_session,
user=user,
target_group_ids=document_set_creation_request.groups,
object_is_public=document_set_creation_request.is_public,
)
try:
document_set_db_model, _ = insert_document_set(
document_set_creation_request=document_set_creation_request,
Expand All @@ -53,11 +53,12 @@ def patch_document_set(
user: User = Depends(current_curator_or_admin_user),
db_session: Session = Depends(get_session),
) -> None:
if user and user.role != UserRole.ADMIN:
validate_curator_request(
groups=document_set_update_request.groups,
is_public=document_set_update_request.is_public,
)
validate_user_creation_permissions(
db_session=db_session,
user=user,
target_group_ids=document_set_update_request.groups,
object_is_public=document_set_update_request.is_public,
)
try:
update_document_set(
document_set_update_request=document_set_update_request,
Expand Down
Loading

0 comments on commit 8d443ad

Please sign in to comment.