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

Made external permissioned users and slack users show diff #3147

Merged
merged 6 commits into from
Nov 17, 2024
Merged
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
42 changes: 42 additions & 0 deletions backend/alembic/versions/dfbe9e93d3c7_extended_role_for_non_web.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
"""extended_role_for_non_web

Revision ID: dfbe9e93d3c7
Revises: 9cf5c00f72fe
Create Date: 2024-11-16 07:54:18.727906

"""
from alembic import op
import sqlalchemy as sa

# revision identifiers, used by Alembic.
revision = "dfbe9e93d3c7"
down_revision = "9cf5c00f72fe"
branch_labels = None
depends_on = None


def upgrade() -> None:
op.execute(
"""
UPDATE "user"
SET role = 'EXT_PERM_USER'
WHERE has_web_login = false
"""
)
op.drop_column("user", "has_web_login")


def downgrade() -> None:
op.add_column(
"user",
sa.Column("has_web_login", sa.Boolean(), nullable=False, server_default="true"),
)

op.execute(
"""
UPDATE "user"
SET has_web_login = false,
role = 'BASIC'
WHERE role IN ('SLACK_USER', 'EXT_PERM_USER')
"""
)
13 changes: 11 additions & 2 deletions backend/danswer/auth/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,24 @@ class UserRole(str, Enum):
groups they are curators of
- Global Curator can perform admin actions
for all groups they are a member of
- Limited can access a limited set of basic api endpoints
- Slack are users that have used danswer via slack but dont have a web login
- External permissioned users that have been picked up during the external permissions sync process but don't have a web login
"""

LIMITED = "limited"
BASIC = "basic"
ADMIN = "admin"
CURATOR = "curator"
GLOBAL_CURATOR = "global_curator"
SLACK_USER = "slack_user"
EXT_PERM_USER = "ext_perm_user"

def is_web_login(self) -> bool:
return self not in [
UserRole.SLACK_USER,
UserRole.EXT_PERM_USER,
]


class UserStatus(str, Enum):
Expand All @@ -34,10 +45,8 @@ class UserRead(schemas.BaseUser[uuid.UUID]):

class UserCreate(schemas.BaseUserCreate):
role: UserRole = UserRole.BASIC
has_web_login: bool | None = True
tenant_id: str | None = None


class UserUpdate(schemas.BaseUserUpdate):
role: UserRole
has_web_login: bool | None = True
39 changes: 20 additions & 19 deletions backend/danswer/auth/users.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
from httpx_oauth.oauth2 import OAuth2Token
from pydantic import BaseModel
from sqlalchemy import text
from sqlalchemy.orm import attributes
from sqlalchemy.orm import Session

from danswer.auth.api_key import get_hashed_api_key_from_request
Expand Down Expand Up @@ -222,6 +221,8 @@ class UserManager(UUIDIDMixin, BaseUserManager[User, uuid.UUID]):
reset_password_token_secret = USER_AUTH_SECRET
verification_token_secret = USER_AUTH_SECRET

user_db: SQLAlchemyUserDatabase[User, uuid.UUID]

async def create(
self,
user_create: schemas.UC | UserCreate,
Expand All @@ -247,7 +248,9 @@ async def create(
verify_email_is_invited(user_create.email)
verify_email_domain(user_create.email)
if MULTI_TENANT:
tenant_user_db = SQLAlchemyUserAdminDB(db_session, User, OAuthAccount)
tenant_user_db = SQLAlchemyUserAdminDB[User, uuid.UUID](
db_session, User, OAuthAccount
)
self.user_db = tenant_user_db
self.database = tenant_user_db

Expand All @@ -266,14 +269,9 @@ async def create(
except exceptions.UserAlreadyExists:
user = await self.get_by_email(user_create.email)
# Handle case where user has used product outside of web and is now creating an account through web
if (
not user.has_web_login
and hasattr(user_create, "has_web_login")
and user_create.has_web_login
):
if not user.role.is_web_login() and user_create.role.is_web_login():
user_update = UserUpdate(
password=user_create.password,
has_web_login=True,
role=user_create.role,
is_verified=user_create.is_verified,
)
Expand All @@ -287,7 +285,7 @@ async def create(
return user

async def oauth_callback(
self: "BaseUserManager[models.UOAP, models.ID]",
self,
oauth_name: str,
access_token: str,
account_id: str,
Expand All @@ -298,7 +296,7 @@ async def oauth_callback(
*,
associate_by_email: bool = False,
is_verified_by_default: bool = False,
) -> models.UOAP:
) -> User:
referral_source = None
if request:
referral_source = getattr(request.state, "referral_source", None)
Expand All @@ -324,9 +322,11 @@ async def oauth_callback(
verify_email_domain(account_email)

if MULTI_TENANT:
tenant_user_db = SQLAlchemyUserAdminDB(db_session, User, OAuthAccount)
tenant_user_db = SQLAlchemyUserAdminDB[User, uuid.UUID](
db_session, User, OAuthAccount
)
self.user_db = tenant_user_db
self.database = tenant_user_db # type: ignore
self.database = tenant_user_db

oauth_account_dict = {
"oauth_name": oauth_name,
Expand Down Expand Up @@ -378,7 +378,11 @@ async def oauth_callback(
and existing_oauth_account.oauth_name == oauth_name
):
user = await self.user_db.update_oauth_account(
user, existing_oauth_account, oauth_account_dict
user,
# NOTE: OAuthAccount DOES implement the OAuthAccountProtocol
# but the type checker doesn't know that :(
existing_oauth_account, # type: ignore
oauth_account_dict,
)

# NOTE: Most IdPs have very short expiry times, and we don't want to force the user to
Expand All @@ -391,16 +395,15 @@ async def oauth_callback(
)

# Handle case where user has used product outside of web and is now creating an account through web
if not user.has_web_login: # type: ignore
if not user.role.is_web_login():
await self.user_db.update(
user,
{
"is_verified": is_verified_by_default,
"has_web_login": True,
"role": UserRole.BASIC,
},
)
user.is_verified = is_verified_by_default
user.has_web_login = True # type: ignore

# this is needed if an organization goes from `TRACK_EXTERNAL_IDP_EXPIRY=true` to `false`
# otherwise, the oidc expiry will always be old, and the user will never be able to login
Expand Down Expand Up @@ -475,9 +478,7 @@ async def authenticate(
self.password_helper.hash(credentials.password)
return None

has_web_login = attributes.get_attribute(user, "has_web_login")

if not has_web_login:
if not user.role.is_web_login():
raise BasicAuthenticationError(
detail="NO_WEB_LOGIN_AND_HAS_NO_PASSWORD",
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
from danswer.db.enums import AccessType
from danswer.db.enums import ConnectorCredentialPairStatus
from danswer.db.models import ConnectorCredentialPair
from danswer.db.users import batch_add_non_web_user_if_not_exists
from danswer.db.users import batch_add_ext_perm_user_if_not_exists
from danswer.redis.redis_connector import RedisConnector
from danswer.redis.redis_connector_doc_perm_sync import (
RedisConnectorPermissionSyncData,
Expand Down Expand Up @@ -301,7 +301,7 @@ def update_external_document_permissions_task(
try:
with get_session_with_tenant(tenant_id) as db_session:
# Then we build the update requests to update vespa
batch_add_non_web_user_if_not_exists(
batch_add_ext_perm_user_if_not_exists(
db_session=db_session,
emails=list(external_access.external_user_emails),
)
Expand Down
4 changes: 2 additions & 2 deletions backend/danswer/danswerbot/slack/handlers/handle_message.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
from danswer.danswerbot.slack.utils import update_emote_react
from danswer.db.engine import get_session_with_tenant
from danswer.db.models import SlackBotConfig
from danswer.db.users import add_non_web_user_if_not_exists
from danswer.db.users import add_slack_user_if_not_exists
from danswer.utils.logger import setup_logger
from shared_configs.configs import SLACK_CHANNEL_ID

Expand Down Expand Up @@ -213,7 +213,7 @@ def handle_message(

with get_session_with_tenant(tenant_id) as db_session:
if message_info.email:
add_non_web_user_if_not_exists(db_session, message_info.email)
add_slack_user_if_not_exists(db_session, message_info.email)

# first check if we need to respond with a standard answer
used_standard_answer = handle_standard_answers(
Expand Down
8 changes: 6 additions & 2 deletions backend/danswer/db/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from typing import Dict

from fastapi import Depends
from fastapi_users.models import ID
from fastapi_users.models import UP
from fastapi_users_db_sqlalchemy import SQLAlchemyUserDatabase
from fastapi_users_db_sqlalchemy.access_token import SQLAlchemyAccessTokenDatabase
Expand Down Expand Up @@ -43,7 +44,10 @@ def get_total_users_count(db_session: Session) -> int:
"""
user_count = (
db_session.query(User)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this filters out externally permissioned users

.filter(~User.email.endswith(get_api_key_email_pattern())) # type: ignore
.filter(
~User.email.endswith(get_api_key_email_pattern()), # type: ignore
User.role != UserRole.EXT_PERM_USER,
)
.count()
)
invited_users = len(get_invited_users())
Expand All @@ -61,7 +65,7 @@ async def get_user_count() -> int:


# Need to override this because FastAPI Users doesn't give flexibility for backend field creation logic in OAuth flow
class SQLAlchemyUserAdminDB(SQLAlchemyUserDatabase):
class SQLAlchemyUserAdminDB(SQLAlchemyUserDatabase[UP, ID]):
async def create(
self,
create_dict: Dict[str, Any],
Expand Down
2 changes: 0 additions & 2 deletions backend/danswer/db/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,6 @@ class User(SQLAlchemyBaseUserTableUUID, Base):
notifications: Mapped[list["Notification"]] = relationship(
"Notification", back_populates="user"
)
# Whether the user has logged in via web. False if user has only used Danswer through Slack bot
has_web_login: Mapped[bool] = mapped_column(Boolean, default=True)
cc_pairs: Mapped[list["ConnectorCredentialPair"]] = relationship(
"ConnectorCredentialPair",
back_populates="creator",
Expand Down
Loading
Loading