Skip to content
Open
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
"""add enduser authentication provider
Revision ID: a7b4e8f2c9d1
Revises: 132392a2635f
Create Date: 2025-11-18 14:00:00.000000
"""
import models as models
import sqlalchemy as sa
from alembic import op

# revision identifiers, used by Alembic.
revision = "a7b4e8f2c9d1"
down_revision = "132392a2635f"
branch_labels = None
depends_on = None


def upgrade():
# ### commands auto generated by Alembic - please adjust! ###
op.create_table(
"tool_enduser_authentication_providers",
sa.Column(
"id",
models.types.StringUUID(),
server_default=sa.text("uuid_generate_v4()"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would break MySQL support.

Copy link
Author

Choose a reason for hiding this comment

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

Will edit the script at once after all comments got resolved at the new provider

nullable=False,
),
sa.Column(
"name",
sa.String(length=256),
server_default=sa.text("'API KEY 1'::character varying"),
nullable=False,
),
sa.Column("tenant_id", models.types.StringUUID(), nullable=False),
sa.Column("end_user_id", models.types.StringUUID(), nullable=False),
sa.Column("provider", sa.String(length=256), nullable=False),
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The provider column is defined as sa.String(length=256), but the corresponding model EndUserAuthenticationProvider defines it as LongText. This can cause data truncation if a provider name exceeds 256 characters. Please use models.types.LongText() for consistency with the model and to avoid potential data loss.

Suggested change
sa.Column("provider", sa.String(length=256), nullable=False),
sa.Column("provider", models.types.LongText(), nullable=False),

sa.Column("encrypted_credentials", sa.Text(), nullable=True),
sa.Column(
"created_at",
sa.DateTime(),
server_default=sa.text("CURRENT_TIMESTAMP(0)"),
nullable=False,
),
sa.Column(
"updated_at",
sa.DateTime(),
server_default=sa.text("CURRENT_TIMESTAMP(0)"),
nullable=False,
),
sa.Column(
"credential_type",
sa.String(length=32),
server_default=sa.text("'api-key'::character varying"),
nullable=False,
),
sa.Column("expires_at", sa.BigInteger(), server_default=sa.text("-1"), nullable=False),
sa.PrimaryKeyConstraint("id", name="tool_enduser_authentication_provider_pkey"),
sa.UniqueConstraint(
"tenant_id",
"provider",
"end_user_id",
"name",
name="unique_enduser_authentication_provider",
),
)
op.create_index(
"tool_enduser_authentication_provider_tenant_id_idx",
"tool_enduser_authentication_providers",
["tenant_id"],
unique=False,
)
op.create_index(
"tool_enduser_authentication_provider_end_user_id_idx",
"tool_enduser_authentication_providers",
["end_user_id"],
unique=False,
)
# ### end Alembic commands ###


def downgrade():
# ### commands auto generated by Alembic - please adjust! ###
op.drop_index(
"tool_enduser_authentication_provider_end_user_id_idx",
table_name="tool_enduser_authentication_providers",
)
op.drop_index(
"tool_enduser_authentication_provider_tenant_id_idx",
table_name="tool_enduser_authentication_providers",
)
op.drop_table("tool_enduser_authentication_providers")
# ### end Alembic commands ###


2 changes: 2 additions & 0 deletions api/models/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@
from .tools import (
ApiToolProvider,
BuiltinToolProvider,
EndUserAuthenticationProvider,
ToolConversationVariables,
ToolFile,
ToolLabelBinding,
Expand Down Expand Up @@ -149,6 +150,7 @@
"DocumentSegment",
"Embedding",
"EndUser",
"EndUserAuthenticationProvider",
"ExternalKnowledgeApis",
"ExternalKnowledgeBindings",
"IconType",
Expand Down
52 changes: 52 additions & 0 deletions api/models/tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,11 @@
from sqlalchemy import ForeignKey, String, func
from sqlalchemy.orm import Mapped, mapped_column

from core.plugin.entities.plugin_daemon import CredentialType
from core.tools.entities.common_entities import I18nObject
from core.tools.entities.tool_bundle import ApiToolBundle
from core.tools.entities.tool_entities import ApiProviderSchemaType, WorkflowToolParameterConfiguration
from libs.uuid_utils import uuidv7

from .base import TypeBase
from .engine import db
Expand Down Expand Up @@ -109,6 +111,56 @@ def credentials(self) -> dict[str, Any]:
return cast(dict[str, Any], json.loads(self.encrypted_credentials))


class EndUserAuthenticationProvider(TypeBase):
"""
This table stores the authentication credentials for end users in tools.
Mimics the BuiltinToolProvider structure but for end users instead of tenants.
"""

__tablename__ = "tool_enduser_authentication_providers"
__table_args__ = (
sa.UniqueConstraint("end_user_id", "provider", "name"),
)
Comment on lines +121 to +123
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

The unique constraint in the model definition is ("end_user_id", "provider", "name"), but the corresponding Alembic migration defines it as ("tenant_id", "provider", "end_user_id", "name"). This inconsistency can lead to unexpected database behavior and ORM errors. The constraint in the migration seems more correct as it includes tenant_id, ensuring uniqueness across the whole system. Please update the model to match the migration.

Suggested change
__table_args__ = (
sa.UniqueConstraint("end_user_id", "provider", "name"),
)
__table_args__ = (
sa.UniqueConstraint("tenant_id", "provider", "end_user_id", "name"),
)


# id of the authentication provider
id: Mapped[str] = mapped_column(StringUUID, primary_key=True, default=uuidv7, init=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The id column uses default=uuidv7, while the migration uses uuid_generate_v4(). Furthermore, other models in this file, like BuiltinToolProvider, use uuid4. For consistency within the codebase and with the database schema, please use a consistent UUID generation method. Using uuid4 would align with other models in this file.

Suggested change
id: Mapped[str] = mapped_column(StringUUID, primary_key=True, default=uuidv7, init=False)
id: Mapped[str] = mapped_column(StringUUID, primary_key=True, default=lambda: str(uuid4()), init=False)

name: Mapped[str] = mapped_column(
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO, enforcing name uniqueness for a (provider, end_user_id) pair creates more overhead than value.

Ideally, we should rely on IDs as the primary identifier and treat names merely as user-facing hints. Therefore, strict uniqueness shouldn't be required. If a user creates duplicate names, they should bear the responsibility for any ambiguity.

Enforcing name uniqueness has caused implementation issues in the past, such as with credential management in the EE version. Furthermore, it complicates name generation.

That said, I suggest discussing this with the PM. I’ve skimmed the PRD and found no explicit requirement for name uniqueness.

Copy link
Author

@CourTeous33 CourTeous33 Nov 21, 2025

Choose a reason for hiding this comment

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

Agreed, should we remove the unique constraint at all? Since ID is enough for uniqueness

String(256),
nullable=False,
default="API KEY 1",
index=True
)
# id of the tenant
tenant_id: Mapped[str] = mapped_column(StringUUID, nullable=False)
# id of the end user
end_user_id: Mapped[str] = mapped_column(StringUUID, nullable=False, index=True)
# name of the tool provider
provider: Mapped[str] = mapped_column(LongText, nullable=False, index=True)
Comment on lines +127 to +138
Copy link
Contributor

Choose a reason for hiding this comment

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

high

There are several inconsistencies between the model definition and the Alembic migration regarding indexes:

  1. name: The model specifies index=True, but the migration does not create a separate index for this column.
  2. provider: The model specifies index=True, but the migration does not create a separate index.
  3. tenant_id: The migration creates an index on this column, but the model does not specify index=True.

These discrepancies can lead to performance issues and schema drift. Please ensure the model definition and the migration are synchronized. It's recommended to make the model the source of truth and adjust the migration accordingly.

# encrypted credentials for the end user
encrypted_credentials: Mapped[str] = mapped_column(LongText, nullable=False, default="")
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The encrypted_credentials column is defined as non-nullable (nullable=False) with a Mapped[str] type hint, but the migration file defines it as nullable. This is inconsistent. To align with the database schema and avoid potential runtime errors, it should be defined as nullable. The BuiltinToolProvider model uses Mapped[str | None] and nullable=True, which is a good pattern to follow.

Suggested change
encrypted_credentials: Mapped[str] = mapped_column(LongText, nullable=False, default="")
encrypted_credentials: Mapped[str | None] = mapped_column(LongText, nullable=True, default=None)

created_at: Mapped[datetime] = mapped_column(
sa.DateTime, nullable=False, default=datetime.now, init=False
)
Comment on lines +141 to +143
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The created_at column uses default=datetime.now. This can lead to issues with timezones as it may not be UTC-aware. It's better to use a consistent, timezone-aware UTC datetime. The BuiltinToolProvider model uses server_default=func.current_timestamp(), which is a good practice for ensuring database-level timestamping in UTC. Please update this to be consistent and avoid potential timezone bugs.

Suggested change
created_at: Mapped[datetime] = mapped_column(
sa.DateTime, nullable=False, default=datetime.now, init=False
)
created_at: Mapped[datetime] = mapped_column(
sa.DateTime, nullable=False, server_default=func.current_timestamp(), init=False
)

updated_at: Mapped[datetime] = mapped_column(
sa.DateTime,
nullable=False,
default=datetime.now,
onupdate=func.current_timestamp(),
init=False,
)
# credential type, e.g., "api-key", "oauth2"
credential_type: Mapped[CredentialType] = mapped_column(
String(32), nullable=False, default=CredentialType.API_KEY
)
expires_at: Mapped[int] = mapped_column(sa.BigInteger, nullable=False, default=-1)

@property
def credentials(self) -> dict[str, Any]:
if not self.encrypted_credentials:
return {}
return cast(dict[str, Any], json.loads(self.encrypted_credentials))


class ApiToolProvider(TypeBase):
"""
The table stores the api providers.
Expand Down