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.

Sign up for GitHub

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

Jump to bottom

Transition to using access_control_list Vespa field for permissioning #450

Merged
merged 1 commit into from
Sep 26, 2023
Merged

Transition to using access_control_list Vespa field for permissioning #450

merged 1 commit into from
Sep 26, 2023

Conversation

Weves
Copy link
Contributor

@Weves Weves commented Sep 16, 2023 •
edited
Loading

Lays the framework for adding document sets.

  • Removes allowed_users / allowed_groups from document indexes and uses access_control_list instead
  • Simplifies document deletion + makes all document updates safe for multi-processing
  • Adds access utility functions, this "module" be where all the logic related to computing a users access levels lives

@vercel
Copy link

vercel bot commented Sep 16, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
internal-search ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 26, 2023 7:16pm

@Weves Weves marked this pull request as ready for review September 16, 2023 02:45
yuhongsun96
yuhongsun96 requested changes Sep 16, 2023
View reviewed changes
backend/danswer/access/access.py Show resolved Hide resolved
backend/danswer/datastores/vespa/app_config/schemas/danswer_chunk.sd Show resolved Hide resolved
backend/danswer/db/document.py
DocumentByConnectorCredentialPair.connector_id == connector_id,
DocumentByConnectorCredentialPair.credential_id == credential_id,
document_ids: list[str],
) -> Sequence[tuple[str, int]]:
Copy link
Contributor

Choose a reason for hiding this comment

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

some comment/description please

backend/danswer/server/document_sets.py Outdated
router = APIRouter(prefix="/manage")


@router.patch("/promote-user-to-admin")
Copy link
Contributor

Choose a reason for hiding this comment

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

nice meme

backend/danswer/server/document_sets.py Outdated
from danswer.db.models import User


router = APIRouter(prefix="/manage")
Copy link
Contributor

Choose a reason for hiding this comment

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

is this still best?

backend/danswer/access/access.py Outdated Show resolved Hide resolved
backend/danswer/access/document_set_utils.py Outdated
if user_id is None:
return PUBLIC_DOCUMENT_SET

return f"__USER_{user_id}"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: prefer having this be PERSONAL_DOCUMENT_SET.format(user_id) or something for consistency

backend/danswer/access/access.py Outdated Show resolved Hide resolved
backend/danswer/background/connector_deletion.py Show resolved Hide resolved
backend/danswer/datastores/vespa/store.py Outdated
ALLOWED_GROUPS: cross_connector_document_metadata_map[
document.id
].allowed_user_groups,
# give each document set "equal weighting" in the weightedset
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this for upcoming feature of document-sets boosting?

If this is supported, how will we handle different personas or knowledge sets favoring different document sets?

Copy link
Contributor

Choose a reason for hiding this comment

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

for future clarity, let's call them personas which have document sets (knowledge sets as a term is banned!)

Weves
Weves commented Sep 16, 2023
View reviewed changes
backend/danswer/datastores/typesense/store.py Outdated
ALLOWED_GROUPS: cross_connector_document_metadata_map[
document.id
].allowed_user_groups,
DOCUMENT_SETS: json.dumps(list(chunk.access.document_sets)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add comment about performance issue

Weves
Weves commented Sep 16, 2023
View reviewed changes
backend/danswer/chunking/models.py
@@ -55,6 +58,34 @@ class IndexChunk(DocAwareChunk):
embeddings: ChunkEmbedding


@dataclass
class DocMetadataAwareIndexChunk(IndexChunk):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: could also attach this info to the document itself, but this doesn't play as well with our current setup

Copy link
Contributor

@yuhongsun96 yuhongsun96 Sep 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of the chunk or?

@Weves Weves changed the title Transition to using document_sets for current permissioning Transition to using access_control_list for permissioning Sep 24, 2023
@Weves Weves changed the title Transition to using access_control_list for permissioning Transition to using access_control_list Vespa field for permissioning Sep 24, 2023
@yuhongsun96
Copy link
Contributor

Is this still relevant? or is this covered by the other PR now?

@Weves Weves merged commit 8594bac into main Sep 26, 2023
@Weves Weves deleted the document-sets branch September 26, 2023 19:26
sidravi1 pushed a commit to IDinsight/danswer that referenced this pull request Oct 10, 2023
@Weves @sidravi1
Transition to using access_control_list to manage access in Vespa (o…
fdf809e 
…nyx-dot-app#450)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuhongsun96 yuhongsun96 yuhongsun96 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Weves @yuhongsun96