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

Add user when they interact outside of UI (e.g. Slack bot) #2369

Merged
merged 11 commits into from
Sep 9, 2024

Conversation

hj-danswer
Copy link
Contributor

@hj-danswer hj-danswer commented Sep 8, 2024

Description

Add capability to add user when someone interacts with a Slack bot without having logged in via the UI. This invovles:

  • Adding a has_web_login column to user table. This allows us to have logic where we allow a user to set their password with basic auth. Otherwise, a user would not know the placeholder password we set for them.

How Has This Been Tested?

  • Create a new user with Slack only. Be able to then authenticate with web login (basic, oidc, google_oauth).
  • Authenticate with web first. Can continue to interact with Slack bot.

Accepted Risk

More database calls / Slack APIs in Slack bot + login flows.

Checklist:

  • All of the automated tests pass
  • All PR comments are addressed and marked resolved
  • If there are migrations, they have been rebased to latest main
  • If there are new dependencies, they are added to the requirements
  • If there are new environment variables, they are added to all of the deployment methods
  • If there are new APIs that don't require auth, they are added to PUBLIC_ENDPOINT_SPECS
  • Docker images build and basic functionalities work
  • Author has done a final read through of the PR right before merge

Copy link

vercel bot commented Sep 8, 2024

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 9, 2024 7:46pm

Copy link
Contributor

@yuhongsun96 yuhongsun96 left a comment

Choose a reason for hiding this comment

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

I think this is a breaking migration, rest looks good 👍

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

Choose a reason for hiding this comment

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

We should test with another user as well (turn off email verification and use basic auth). Do a login with test@danswer.ai off of main and then upgrade the version. I think it will fail due to the NOT NULL constraint. We have to support graceful upgrades

@@ -1,5 +1,6 @@
import uuid
from enum import Enum
from typing import Optional
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we can't use the built in typing that came with the more recent python versions?

@@ -47,6 +52,25 @@ def send_msg_ack_to_user(details: SlackMessageInfo, client: WebClient) -> None:
)


def add_user_if_not_exists(email: str, db_session: Session) -> User:
Copy link
Contributor

Choose a reason for hiding this comment

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

We like to throw these functions that interface with the DB models (and that may be reuseable) in the related file under backend/db. Makes organization/finding things easy and helps prevents circular imports.

@@ -208,7 +232,14 @@ def handle_message(
except SlackApiError as e:
logger.error(f"Was not able to react to user message due to: {e}")

slack_user_info = expert_info_from_slack_id(
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we've added this in a few places recently, is there a way we can do it at the top level and pass it through? That way we make less Slack calls. In this case the volume shouldn't be very high but I think we can probably make this small optimization. The case where it would matter is if for example they have a Slack connector that has been rate limited due to volume of requests and it's affecting this side as well (some folks don't create separate Slack apps for connector and SlackBot)

Copy link
Contributor

@yuhongsun96 yuhongsun96 left a comment

Choose a reason for hiding this comment

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

Whoohoo LGTM

@hj-danswer hj-danswer added this pull request to the merge queue Sep 9, 2024
Merged via the queue into main with commit e4e4765 Sep 9, 2024
7 checks passed
rajivml pushed a commit to UiPath/danswer that referenced this pull request Oct 2, 2024
…app#2369)

* Add user when they interact outside of UI (e.g. Slack bot)

* fix mypy errors

* don't use user manager to avoid async messiness

* fix email is none scenario

* fix mypy

* make code slightly clearer

* PR comments

* get slack email in generate button as well

* fix alembic migration

* update name to be more descriptive

---------

Co-authored-by: Hyeong Joon Suh <hyeongjoonsuh@Hyeongs-MacBook-Pro.local>
rajivml pushed a commit to UiPath/danswer that referenced this pull request Oct 2, 2024
…app#2369)

* Add user when they interact outside of UI (e.g. Slack bot)

* fix mypy errors

* don't use user manager to avoid async messiness

* fix email is none scenario

* fix mypy

* make code slightly clearer

* PR comments

* get slack email in generate button as well

* fix alembic migration

* update name to be more descriptive

---------

Co-authored-by: Hyeong Joon Suh <hyeongjoonsuh@Hyeongs-MacBook-Pro.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants