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

Bond userEmail with userId on Dust managed apps in createConversation #7478

Merged
merged 11 commits into from
Sep 20, 2024

Conversation

albandum
Copy link
Contributor

Description

  • We are preparing to test some dust-managed apps, in which users can create conversations from the public API
  • To be able to track MAU and bill accordingly, we need to bond the userId of these messages like we do with slack
  • So we use the email in the message context to find the corresponding user

Risk

Not much

Deploy Plan

@albandum albandum requested a review from spolu September 17, 2024 15:14
Copy link

github-actions bot commented Sep 17, 2024

Fails
🚫

Files in **/api/v1/ have been modified. Please add the documentation-ack label to acknowlegde that if anything changes
in a public endpoint, you need to edit the JSDoc comment
above the handler definition and/or the swagger_schemas.ts file and regenerate the documentation using npm run docs

Generated by 🚫 dangerJS against c00415e


// If we have a context email but no user, we try to find a user with that email in the workspace.
if (!userId && context.email) {
const matchingUser = await UserResource.fetchByEmail(context.email);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this to the API endpoint as this is specific to the public API right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. Sounds good indeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved - not sure about the location of getUserAuthenticatorFromEmail(), does it sound right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Put back here after authenticator discussion. Not sure moving it to API endpoint is worth the extra attribute on postUserMessage()

Copy link
Contributor

Choose a reason for hiding this comment

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

fair.

You likely want to dry it and use it in editUserMessage as well?

@albandum albandum requested a review from spolu September 18, 2024 09:28
Copy link
Contributor

@spolu spolu left a comment

Choose a reason for hiding this comment

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

Thinking more about it this means that anyone with an API key can impersonate any user on Dust.

They can as an example access vaults they don't have access to.

So that's deifnitely a no go.

What are the problems to solve exactly?

@albandum
Copy link
Contributor Author

That's just being used to save userId in the message when creating the conversation to track MAU from external apps managed by Dust (like we currently do with Slack).
Previous version in with the code the private conversation API did not use any authenticators but only added the userId when creating the message, so I'd say that was better.

If vaults accesses based on conversation participants (userId in messages from that conversation) then yes, that's an issue indeed. We then have 3 solutions:

  • Add a salt to dust-managed external apps (zendesk marketplace, jira marketplace, etc) and only allow this to happen when the salt is checked
  • Abort all this and rework a bit MAU calculation in stripe to invoice active users also based on userEmail in messages and not just userId
  • Let it go for now, check the usage of external apps using metabase and act accordingly if usage rises and billing becomes an issue

@albandum
Copy link
Contributor Author

Adding @thib-martin to the discussion, might have an insight

@spolu
Copy link
Contributor

spolu commented Sep 18, 2024

Access to vaults is based on the Authenticator object. Here you crearte a new One based on a user controled string which is not acceptable.

…sation creation + add it to messages endpoint as well"

This reverts commit 7902319.
@albandum
Copy link
Contributor Author

Access to vaults is based on the Authenticator object. Here you crearte a new One based on a user controled string which is not acceptable.

That's what I figured - reverted the last 2 commits.
Imho the best way is to keep that check in the conversations private endpoint, because if I want to move it to the public endpoint, I need to modify postUserMessage() to add a userId argument and that seems quite overkill.
WDYT?

@albandum albandum requested a review from spolu September 19, 2024 08:42
front/lib/api/assistant/conversation.ts Outdated Show resolved Hide resolved

// If we have a context email but no user, we try to find a user with that email in the workspace.
if (!userId && context.email) {
const matchingUser = await UserResource.fetchByEmail(context.email);
Copy link
Contributor

Choose a reason for hiding this comment

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

fair.

You likely want to dry it and use it in editUserMessage as well?

@albandum albandum requested a review from spolu September 20, 2024 15:41
@albandum
Copy link
Contributor Author

You likely want to dry it and use it in editUserMessage as well?
Done

Copy link
Contributor

@spolu spolu left a comment

Choose a reason for hiding this comment

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

LGTM modulo the last comment 🙏

@albandum albandum requested a review from spolu September 20, 2024 15:49
@albandum
Copy link
Contributor Author

LGTM modulo the last comment 🙏

Done and merging

@albandum albandum merged commit 2bf607d into main Sep 20, 2024
3 checks passed
@albandum albandum deleted the alban/userIdValidation branch September 20, 2024 15:54
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