-
Notifications
You must be signed in to change notification settings - Fork 8.2k
fix: superuser credential handling and AUTO_LOGIN security #9542
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
Changes from all commits
456587f
42162f8
927d4c4
606fa70
c32b466
4cb3e21
40e1d38
f73286f
97b6d09
78aa8d1
7d4baa2
1d68fd6
febe15a
c0ac46c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,7 +48,8 @@ class AuthSettings(BaseSettings): | |
|
|
||
| NEW_USER_IS_ACTIVE: bool = False | ||
| SUPERUSER: str = DEFAULT_SUPERUSER | ||
| SUPERUSER_PASSWORD: str = DEFAULT_SUPERUSER_PASSWORD | ||
| # Store password as SecretStr to prevent accidental plaintext exposure | ||
| SUPERUSER_PASSWORD: SecretStr = Field(default=DEFAULT_SUPERUSER_PASSWORD) | ||
|
|
||
| REFRESH_SAME_SITE: Literal["lax", "strict", "none"] = "none" | ||
| """The SameSite attribute of the refresh token cookie.""" | ||
|
|
@@ -71,8 +72,8 @@ class AuthSettings(BaseSettings): | |
| model_config = SettingsConfigDict(validate_assignment=True, extra="ignore", env_prefix="LANGFLOW_") | ||
|
|
||
| def reset_credentials(self) -> None: | ||
| self.SUPERUSER = DEFAULT_SUPERUSER | ||
| self.SUPERUSER_PASSWORD = DEFAULT_SUPERUSER_PASSWORD | ||
| # Preserve the configured username but scrub the password from memory to avoid plaintext exposure. | ||
| self.SUPERUSER_PASSWORD = SecretStr("") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually think
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe we need a user at any point. @coderabbitai can you suggest ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainLet me analyze the authentication flow and how the @edwinjosechittilappilly, based on my analysis of the codebase, here's my suggestion regarding the Current Implementation AnalysisThe current
Recommendation: Keep Current Approach with Minor RefinementsI recommend keeping the current implementation rather than setting values to Why Not Set to
|
||
|
|
||
| # If autologin is true, then we need to set the credentials to | ||
| # the default values | ||
|
|
@@ -81,15 +82,17 @@ def reset_credentials(self) -> None: | |
| @field_validator("SUPERUSER", "SUPERUSER_PASSWORD", mode="before") | ||
| @classmethod | ||
| def validate_superuser(cls, value, info): | ||
| # When AUTO_LOGIN is enabled, force superuser to use default values. | ||
| if info.data.get("AUTO_LOGIN"): | ||
| if value != DEFAULT_SUPERUSER: | ||
| value = DEFAULT_SUPERUSER | ||
| logger.debug("Resetting superuser to default value") | ||
| if info.data.get("SUPERUSER_PASSWORD") != DEFAULT_SUPERUSER_PASSWORD: | ||
| info.data["SUPERUSER_PASSWORD"] = DEFAULT_SUPERUSER_PASSWORD | ||
| logger.debug("Resetting superuser password to default value") | ||
|
|
||
| return value | ||
| logger.debug("Auto login is enabled, forcing superuser to use default values") | ||
| if info.field_name == "SUPERUSER": | ||
| if value != DEFAULT_SUPERUSER: | ||
| logger.debug("Resetting superuser to default value") | ||
| return DEFAULT_SUPERUSER | ||
| if info.field_name == "SUPERUSER_PASSWORD": | ||
| if value != DEFAULT_SUPERUSER_PASSWORD.get_secret_value(): | ||
| logger.debug("Resetting superuser password to default value") | ||
| return DEFAULT_SUPERUSER_PASSWORD | ||
|
|
||
| return value | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,18 +72,20 @@ async def setup_superuser(settings_service: SettingsService, session: AsyncSessi | |
| if settings_service.auth_settings.AUTO_LOGIN: | ||
| await logger.adebug("AUTO_LOGIN is set to True. Creating default superuser.") | ||
| username = DEFAULT_SUPERUSER | ||
| password = DEFAULT_SUPERUSER_PASSWORD | ||
| password = DEFAULT_SUPERUSER_PASSWORD.get_secret_value() | ||
| else: | ||
| # Remove the default superuser if it exists | ||
| await teardown_superuser(settings_service, session) | ||
| username = settings_service.auth_settings.SUPERUSER | ||
| password = settings_service.auth_settings.SUPERUSER_PASSWORD | ||
| # If AUTO_LOGIN is disabled, attempt to use configured credentials | ||
| # or fall back to default credentials if none are provided. | ||
| username = settings_service.auth_settings.SUPERUSER or DEFAULT_SUPERUSER | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ideal case yes but if the user defined an empty user name in settings it might override the default to empty I believe right?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the user defined an empty user name in the settings we should catch that as a user-error and raise that, not assume that we should use the default_superuser |
||
| password = (settings_service.auth_settings.SUPERUSER_PASSWORD or DEFAULT_SUPERUSER_PASSWORD).get_secret_value() | ||
|
|
||
| if not username or not password: | ||
| msg = "Username and password must be set" | ||
| raise ValueError(msg) | ||
|
|
||
| is_default = (username == DEFAULT_SUPERUSER) and (password == DEFAULT_SUPERUSER_PASSWORD) | ||
| is_default = (username == DEFAULT_SUPERUSER) and (password == DEFAULT_SUPERUSER_PASSWORD.get_secret_value()) | ||
|
|
||
| try: | ||
| user = await get_or_create_super_user( | ||
|
|
@@ -96,6 +98,7 @@ async def setup_superuser(settings_service: SettingsService, session: AsyncSessi | |
| msg = "Could not create superuser. Please create a superuser manually." | ||
| raise RuntimeError(msg) from exc | ||
| finally: | ||
| # Scrub credentials from in-memory settings after setup | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Elaborate on this comment -- add a note saying something like: |
||
| settings_service.auth_settings.reset_credentials() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See other comment -- suggest clearing them completely |
||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| import pytest | ||
| from langflow.services.auth.utils import create_user_longterm_token | ||
| from langflow.services.deps import get_db_service, get_settings_service | ||
| from langflow.services.utils import initialize_services | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_mcp_longterm_token_headless_superuser_integration(): | ||
| """Integration-style check that without explicit credentials, AUTO_LOGIN=false path. | ||
|
|
||
| Creates a headless superuser via initialize_services and allows minting a long-term token. | ||
| """ | ||
| settings = get_settings_service() | ||
| settings.auth_settings.AUTO_LOGIN = False | ||
| settings.auth_settings.SUPERUSER = "" | ||
| settings.auth_settings.SUPERUSER_PASSWORD = "" | ||
|
|
||
| await initialize_services() | ||
|
|
||
| async with get_db_service().with_session() as session: | ||
| user_id, tokens = await create_user_longterm_token(session) | ||
| assert user_id is not None | ||
| assert tokens.get("access_token") |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| from pathlib import Path | ||
|
|
||
| import pytest | ||
| from langflow.services.settings.auth import AuthSettings | ||
| from langflow.services.settings.constants import DEFAULT_SUPERUSER, DEFAULT_SUPERUSER_PASSWORD | ||
| from pydantic import SecretStr | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("auto_login", [True, False]) | ||
| def test_superuser_password_is_secretstr(auto_login, tmp_path: Path): | ||
| cfg_dir = tmp_path.as_posix() | ||
| settings = AuthSettings(CONFIG_DIR=cfg_dir, AUTO_LOGIN=auto_login) | ||
| assert isinstance(settings.SUPERUSER_PASSWORD, SecretStr) | ||
|
|
||
|
|
||
| def test_auto_login_true_forces_default_and_scrubs_password(tmp_path: Path): | ||
| cfg_dir = tmp_path.as_posix() | ||
| settings = AuthSettings( | ||
| CONFIG_DIR=cfg_dir, | ||
| AUTO_LOGIN=True, | ||
| SUPERUSER="custom", | ||
| SUPERUSER_PASSWORD=DEFAULT_SUPERUSER_PASSWORD + "_changed", | ||
| ) | ||
| # Validator forces default username and scrubs password | ||
| assert settings.SUPERUSER == DEFAULT_SUPERUSER | ||
| assert isinstance(settings.SUPERUSER_PASSWORD, SecretStr) | ||
| assert settings.SUPERUSER_PASSWORD.get_secret_value() == "" | ||
|
|
||
| # reset_credentials keeps default username (AUTO_LOGIN on) and keeps password scrubbed | ||
| settings.reset_credentials() | ||
| assert settings.SUPERUSER == DEFAULT_SUPERUSER | ||
| assert settings.SUPERUSER_PASSWORD.get_secret_value() == "" | ||
|
|
||
|
|
||
| def test_auto_login_false_preserves_username_and_scrubs_password_on_reset(tmp_path: Path): | ||
| cfg_dir = tmp_path.as_posix() | ||
| settings = AuthSettings( | ||
| CONFIG_DIR=cfg_dir, | ||
| AUTO_LOGIN=False, | ||
| SUPERUSER="admin", | ||
| SUPERUSER_PASSWORD="strongpass", # noqa: S106 | ||
| ) | ||
| # Values preserved at init | ||
| assert settings.SUPERUSER == "admin" | ||
| assert settings.SUPERUSER_PASSWORD.get_secret_value() == "strongpass" | ||
|
|
||
| # After reset, username preserved, password scrubbed | ||
| settings.reset_credentials() | ||
| assert settings.SUPERUSER == "admin" | ||
| assert settings.SUPERUSER_PASSWORD.get_secret_value() == "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should get a non-default superuser here. As you said, this method should only be used with AUTO_LOGIN is true, meaning the default superuser must exist and be used.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function "create_user_longterm_token" is called only when auto login is true
@coderabbitai please check this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
@edwinjosechittilappilly I'll check where the
create_user_longterm_tokenfunction is called to verify if it's only used when auto login is true.🏁 Script executed:
Length of output: 4943
@edwinjosechittilappilly You are correct! After checking the codebase, I can confirm that
create_user_longterm_tokenis only called whenAUTO_LOGINis true.The main production usage is in
src/backend/base/langflow/api/v1/login.pyat line 85, inside theauto_loginendpoint, which has an explicit check:However, @jordanrfrazier raises a valid design point. Since this function is only used with
AUTO_LOGIN=true, the question becomes: should auto-login fall back to any existing superuser, or should it strictly require the configured superuser to exist?The current change allows fallback to any superuser when the configured one doesn't exist. This could be helpful for backward compatibility, but it might also mask configuration issues where the intended superuser wasn't created properly.
What's the intended behavior here? Should auto-login:
This design decision will help determine if the fallback logic should remain.