refactor: streamline required scopes handling in GoogleDriveOAuth and add test for optional group scopes#1716
Closed
ricofurtado wants to merge 4 commits into
Closed
Conversation
… add test for optional group scopes
Contributor
WalkthroughThis PR adds a REQUIRED_SCOPES constant and narrows Google Drive credential scope checks, enables asymmetric JWT signing for Langflow ingest tokens with updated creation/validation, refactors OpenSearch onboarding/startup to select credentials by run mode with per-call getters, updates Langflow flow JSON (nodes, model options, Calculator code), and bumps a Docker base image. ChangesGoogle Drive OAuth Scope Refactor
Langflow Ingest Token Service
OpenSearch Run-Mode and Bootstrap
Langflow Flow JSON Updates
Miscellaneous
Sequence Diagram(s)sequenceDiagram
participant OnboardingEndpoint
participant PlatformServiceToken
participant OpenSearchClient
participant AdminIdentity
participant SetupOpenSearchSecurity
OnboardingEndpoint->>PlatformServiceToken: request service token (saas)
PlatformServiceToken-->>OnboardingEndpoint: JWT (claims with admin username)
OnboardingEndpoint->>AdminIdentity: derive admin username from JWT
OnboardingEndpoint->>OpenSearchClient: create client (JWT-based or unauthenticated or basic-auth)
OpenSearchClient->>SetupOpenSearchSecurity: wait_for_opensearch() / setup_opensearch_security(admin)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
backend
…rding flow, and support asymmetric JWT signing (#1713) * Support OpenSearch basic-auth for on-prem Add support for using OpenSearch basic-auth credentials in on-prem IBM-auth deployments. Introduces get_opensearch_username(), get_opensearch_password(), and use_opensearch_basic_auth() to read credentials at runtime (allowing overrides without restart). Update onboarding and lifespan startup logic to create a basic-auth OpenSearch client and use the OpenSearch username as the admin when basic-auth is enabled; otherwise preserve JWT-based client creation and bootstrap behavior. Adjust AppClients.initialize to set opensearch auth appropriately based on IBM_AUTH_ENABLED and the new basic-auth mode. * Prefer service token for OpenSearch onboarding Import get_openrag_service_token and prefer the platform service JWT when creating the OpenSearch client during onboarding, so the onboarding process pins the same admin identity as the startup security bootstrap. If a service token is present, extract admin username via admin_username_from_service_jwt and create the client with create_opensearch_client_from_jwt; raise a RuntimeError if the token lacks a username/sub claim. Maintain a backward-compatibility fallback: log a warning and fall back to using the onboarding user's jwt_token (and set admin_username to the user) when no service token is provided. Add a TODO to remove the fallback once OPENRAG_SERVICE_TOKEN is always supplied. * Support JWT_SIGNING_KEY and key type detection Prefer JWT_SIGNING_KEY over SESSION_SECRET and add PEM private key support for asymmetric JWTs. Introduces _resolve_default_signing_config to load PEM keys via cryptography, detect key types (RSA, EC with curve->ES*, EdDSA) and return signing/verification keys plus algorithm. LangflowIngestTokenService now stores separate signing and verification keys, uses them for jwt.encode/decode, and still accepts an explicit symmetric secret. Adds a unit test to ensure JWT_SIGNING_KEY is preferred and that a session secret cannot validate a JWT signed with JWT_SIGNING_KEY. * style: ruff autofix (auto) * Update langflow_ingest_token_service.py * Use run-mode utils for OpenSearch auth selection Unify OpenSearch client and admin selection across onboarding, startup, and client initialization by using run-mode helpers (is_run_mode_saas/on_prem/oss) instead of the old use_opensearch_basic_auth helper. Prefer the platform service token in SaaS mode (with a backward-compat fallback to the onboarding user when the token is missing), and use OpenSearch basic-auth credentials in on-prem/OSS modes. Added informative logging for each path and adjusted AppClients.initialize to choose the global writer client based on run mode. Changes in src/api/settings/endpoints.py, src/app/lifespan.py, and src/config/settings.py. * style: ruff autofix (auto) * Update lifespan.py --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
* Upgraded Langflow to 1.9.3 * update docling remote to throw errors * upgrade docling version on docling manager * style: ruff autofix (auto) * Update docling code and fix docling manager lint * updated langflow to 1.9.4 * change image to future langflow image * upgraded to langflow 1.9.6rc0 * fix langflow image --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/services/langflow_ingest_token_service.py (1)
29-50: 💤 Low valueWrap PEM parsing in error handling for clearer startup failures.
If
JWT_SIGNING_KEYis set to a malformed or password-protected PEM,load_pem_private_keyraises an opaqueValueErrororTypeError. Consider catching and re-raising with a descriptive message to aid debugging configuration issues at startup.🛡️ Suggested improvement
if signing_key.lstrip().startswith("-----BEGIN"): - key = serialization.load_pem_private_key(signing_key.encode(), password=None) + try: + key = serialization.load_pem_private_key(signing_key.encode(), password=None) + except (ValueError, TypeError) as exc: + raise ValueError( + "JWT_SIGNING_KEY appears to be a PEM key but could not be loaded. " + "Ensure it is a valid unencrypted private key." + ) from exc public_key = key.public_key()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/langflow_ingest_token_service.py` around lines 29 - 50, Wrap the PEM parsing call around serialization.load_pem_private_key in a try/except that catches ValueError, TypeError and cryptography.exceptions.UnsupportedAlgorithm (or a broad Exception if UnsupportedAlgorithm isn't imported) and re-raise a new ValueError with a clear, descriptive message including the original exception text and indicating JWT_SIGNING_KEY is malformed or password-protected; keep the subsequent logic that derives public_key and sets algorithm (rsa.RSAPrivateKey, ec.EllipticCurvePrivateKey, ed25519.Ed25519PrivateKey/ed448.Ed448PrivateKey checks) unchanged so only the PEM loading step is given clearer error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@flows/openrag_agent.json`:
- Line 1741: The evaluator in CalculatorComponent (method _eval_expr) currently
rejects ast.UnaryOp, causing expressions like "-3+2" to fail; update _eval_expr
to handle ast.UnaryOp by mapping ast.UAdd/ast.USub to +/- and applying them to
the recursively evaluated operand (use operator.pos/operator.neg or simple unary
logic), ensure the handler coexists with the existing OPERATORS lookup for
ast.BinOp, and keep error handling in evaluate_expression unchanged so
unary-related errors still surface as invalid expressions.
In `@src/config/settings.py`:
- Around line 618-619: When constructing basic auth for on-prem/oss, validate
that get_opensearch_password() returns a non-None, non-empty string before
building os_auth; if it is None/empty, raise or log an explicit error (mirroring
the SaaS branch service token check) instead of passing
(get_opensearch_username(), None) to http_auth. Update the block that calls
is_run_mode_on_prem() or is_run_mode_oss() to fetch pw =
get_opensearch_password(), check pw, and only set os_auth =
(get_opensearch_username(), pw) when valid; apply the same validation pattern
for other call sites that pass get_opensearch_password() into
create_basic_opensearch_client().
---
Nitpick comments:
In `@src/services/langflow_ingest_token_service.py`:
- Around line 29-50: Wrap the PEM parsing call around
serialization.load_pem_private_key in a try/except that catches ValueError,
TypeError and cryptography.exceptions.UnsupportedAlgorithm (or a broad Exception
if UnsupportedAlgorithm isn't imported) and re-raise a new ValueError with a
clear, descriptive message including the original exception text and indicating
JWT_SIGNING_KEY is malformed or password-protected; keep the subsequent logic
that derives public_key and sets algorithm (rsa.RSAPrivateKey,
ec.EllipticCurvePrivateKey, ed25519.Ed25519PrivateKey/ed448.Ed448PrivateKey
checks) unchanged so only the PEM loading step is given clearer error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 167b9508-2b5c-4558-980a-d2c9ca6558bf
📒 Files selected for processing (8)
Dockerfile.langflowflows/ingestion_flow.jsonflows/openrag_agent.jsonsrc/api/settings/endpoints.pysrc/app/lifespan.pysrc/config/settings.pysrc/services/langflow_ingest_token_service.pytests/unit/test_langflow_ingest_callback.py
✅ Files skipped from review due to trivial changes (1)
- Dockerfile.langflow
| "title_case": false, | ||
| "type": "code", | ||
| "value": "import ast\nimport operator\nfrom collections.abc import Callable\n\nfrom lfx.custom.custom_component.component import Component\nfrom lfx.inputs.inputs import MessageTextInput\nfrom lfx.io import Output\nfrom lfx.schema.data import Data\n\n\nclass CalculatorComponent(Component):\n display_name = \"Calculator\"\n description = \"Perform basic arithmetic operations on a given expression.\"\n documentation: str = \"https://docs.langflow.org/calculator\"\n icon = \"calculator\"\n\n # Cache operators dictionary as a class variable\n OPERATORS: dict[type[ast.operator], Callable] = {\n ast.Add: operator.add,\n ast.Sub: operator.sub,\n ast.Mult: operator.mul,\n ast.Div: operator.truediv,\n ast.Pow: operator.pow,\n }\n\n inputs = [\n MessageTextInput(\n name=\"expression\",\n display_name=\"Expression\",\n info=\"The arithmetic expression to evaluate (e.g., '4*4*(33/22)+12-20').\",\n tool_mode=True,\n ),\n ]\n\n outputs = [\n Output(display_name=\"JSON\", name=\"result\", type_=Data, method=\"evaluate_expression\"),\n ]\n\n def _eval_expr(self, node: ast.AST) -> float:\n \"\"\"Evaluate an AST node recursively.\"\"\"\n if isinstance(node, ast.Constant):\n if isinstance(node.value, int | float):\n return float(node.value)\n error_msg = f\"Unsupported constant type: {type(node.value).__name__}\"\n raise TypeError(error_msg)\n if isinstance(node, ast.Num): # For backwards compatibility\n if isinstance(node.n, int | float):\n return float(node.n)\n error_msg = f\"Unsupported number type: {type(node.n).__name__}\"\n raise TypeError(error_msg)\n\n if isinstance(node, ast.BinOp):\n op_type = type(node.op)\n if op_type not in self.OPERATORS:\n error_msg = f\"Unsupported binary operator: {op_type.__name__}\"\n raise TypeError(error_msg)\n\n left = self._eval_expr(node.left)\n right = self._eval_expr(node.right)\n return self.OPERATORS[op_type](left, right)\n\n error_msg = f\"Unsupported operation or expression type: {type(node).__name__}\"\n raise TypeError(error_msg)\n\n def evaluate_expression(self) -> Data:\n \"\"\"Evaluate the mathematical expression and return the result.\"\"\"\n try:\n tree = ast.parse(self.expression, mode=\"eval\")\n result = self._eval_expr(tree.body)\n\n formatted_result = f\"{float(result):.6f}\".rstrip(\"0\").rstrip(\".\")\n self.log(f\"Calculation result: {formatted_result}\")\n\n self.status = formatted_result\n return Data(data={\"result\": formatted_result})\n\n except ZeroDivisionError:\n error_message = \"Error: Division by zero\"\n self.status = error_message\n return Data(data={\"error\": error_message, \"input\": self.expression})\n\n except (SyntaxError, TypeError, KeyError, ValueError, AttributeError, OverflowError) as e:\n error_message = f\"Invalid expression: {e!s}\"\n self.status = error_message\n return Data(data={\"error\": error_message, \"input\": self.expression})\n\n def build(self):\n \"\"\"Return the main evaluation function.\"\"\"\n return self.evaluate_expression\n" | ||
| "value": "import ast\nimport operator\nfrom collections.abc import Callable\n\nfrom lfx.custom.custom_component.component import Component\nfrom lfx.inputs.inputs import MessageTextInput\nfrom lfx.io import Output\nfrom lfx.schema.data import Data\n\n\nclass CalculatorComponent(Component):\n display_name = \"Calculator\"\n description = \"Perform basic arithmetic operations on a given expression.\"\n documentation: str = \"https://docs.langflow.org/calculator\"\n icon = \"calculator\"\n\n # Cache operators dictionary as a class variable\n OPERATORS: dict[type[ast.operator], Callable] = {\n ast.Add: operator.add,\n ast.Sub: operator.sub,\n ast.Mult: operator.mul,\n ast.Div: operator.truediv,\n ast.Pow: operator.pow,\n }\n\n inputs = [\n MessageTextInput(\n name=\"expression\",\n display_name=\"Expression\",\n info=\"The arithmetic expression to evaluate (e.g., '4*4*(33/22)+12-20').\",\n tool_mode=True,\n ),\n ]\n\n outputs = [\n Output(display_name=\"JSON\", name=\"result\", type_=Data, method=\"evaluate_expression\"),\n ]\n\n def _eval_expr(self, node: ast.AST) -> float:\n \"\"\"Evaluate an AST node recursively.\"\"\"\n if isinstance(node, ast.Constant):\n if isinstance(node.value, int | float):\n return float(node.value)\n error_msg = f\"Unsupported constant type: {type(node.value).__name__}\"\n raise TypeError(error_msg)\n\n if isinstance(node, ast.BinOp):\n op_type = type(node.op)\n if op_type not in self.OPERATORS:\n error_msg = f\"Unsupported binary operator: {op_type.__name__}\"\n raise TypeError(error_msg)\n\n left = self._eval_expr(node.left)\n right = self._eval_expr(node.right)\n return self.OPERATORS[op_type](left, right)\n\n error_msg = f\"Unsupported operation or expression type: {type(node).__name__}\"\n raise TypeError(error_msg)\n\n def evaluate_expression(self) -> Data:\n \"\"\"Evaluate the mathematical expression and return the result.\"\"\"\n try:\n tree = ast.parse(self.expression, mode=\"eval\")\n result = self._eval_expr(tree.body)\n\n formatted_result = f\"{float(result):.6f}\".rstrip(\"0\").rstrip(\".\")\n self.log(f\"Calculation result: {formatted_result}\")\n\n self.status = formatted_result\n return Data(data={\"result\": formatted_result})\n\n except ZeroDivisionError:\n error_message = \"Error: Division by zero\"\n self.status = error_message\n return Data(data={\"error\": error_message, \"input\": self.expression})\n\n except (SyntaxError, TypeError, KeyError, ValueError, AttributeError, OverflowError) as e:\n error_message = f\"Invalid expression: {e!s}\"\n self.status = error_message\n return Data(data={\"error\": error_message, \"input\": self.expression})\n\n def build(self):\n \"\"\"Return the main evaluation function.\"\"\"\n return self.evaluate_expression\n" |
Contributor
There was a problem hiding this comment.
Support unary operators in the AST calculator evaluator.
At Line 1741, the evaluator rejects ast.UnaryOp, so valid expressions like -3+2 fail as invalid. This breaks basic arithmetic support.
Suggested fix
- def _eval_expr(self, node: ast.AST) -> float:
+ def _eval_expr(self, node: ast.AST) -> float:
"""Evaluate an AST node recursively."""
if isinstance(node, ast.Constant):
if isinstance(node.value, int | float):
return float(node.value)
error_msg = f"Unsupported constant type: {type(node.value).__name__}"
raise TypeError(error_msg)
+ if isinstance(node, ast.UnaryOp):
+ operand = self._eval_expr(node.operand)
+ if isinstance(node.op, ast.UAdd):
+ return +operand
+ if isinstance(node.op, ast.USub):
+ return -operand
+ error_msg = f"Unsupported unary operator: {type(node.op).__name__}"
+ raise TypeError(error_msg)
+
if isinstance(node, ast.BinOp):
op_type = type(node.op)
if op_type not in self.OPERATORS:
error_msg = f"Unsupported binary operator: {op_type.__name__}"
raise TypeError(error_msg)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@flows/openrag_agent.json` at line 1741, The evaluator in CalculatorComponent
(method _eval_expr) currently rejects ast.UnaryOp, causing expressions like
"-3+2" to fail; update _eval_expr to handle ast.UnaryOp by mapping
ast.UAdd/ast.USub to +/- and applying them to the recursively evaluated operand
(use operator.pos/operator.neg or simple unary logic), ensure the handler
coexists with the existing OPERATORS lookup for ast.BinOp, and keep error
handling in evaluate_expression unchanged so unary-related errors still surface
as invalid expressions.
Comment on lines
+618
to
+619
| if is_run_mode_on_prem() or is_run_mode_oss(): | ||
| os_auth = (get_opensearch_username(), get_opensearch_password()) |
Contributor
There was a problem hiding this comment.
Validate password before constructing basic auth credentials.
get_opensearch_password() can return None, but passing (username, None) to http_auth may cause OpenSearch to receive a malformed auth header or fail unexpectedly. In on_prem/oss modes, the password is required for basic auth to succeed.
Consider adding validation similar to the SaaS branch's service token check:
🛡️ Proposed fix
if is_run_mode_on_prem() or is_run_mode_oss():
+ os_password = get_opensearch_password()
+ if not os_password:
+ raise RuntimeError(
+ "OPENSEARCH_PASSWORD is required for on_prem/oss modes "
+ "but is not set"
+ )
- os_auth = (get_opensearch_username(), get_opensearch_password())
+ os_auth = (get_opensearch_username(), os_password)Note: The same pattern appears in src/api/settings/endpoints.py (line 1110-1112) and src/app/lifespan.py (line 225-227) where get_opensearch_password() is passed to create_basic_opensearch_client() which expects password: str.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config/settings.py` around lines 618 - 619, When constructing basic auth
for on-prem/oss, validate that get_opensearch_password() returns a non-None,
non-empty string before building os_auth; if it is None/empty, raise or log an
explicit error (mirroring the SaaS branch service token check) instead of
passing (get_opensearch_username(), None) to http_auth. Update the block that
calls is_run_mode_on_prem() or is_run_mode_oss() to fetch pw =
get_opensearch_password(), check pw, and only set os_auth =
(get_opensearch_username(), pw) when valid; apply the same validation pattern
for other call sites that pass get_opensearch_password() into
create_basic_opensearch_client().
Collaborator
Author
|
Too many merging issues. replacing it for #1744 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Google Drive OAuth was requesting Drive scopes plus optional Google Workspace group/admin scopes, then treating all of them as required. If Google withheld either optional scope, the OAuth callback could succeed, but the next connector status check would reject/delete the token and show Google Drive as not connected. That matches your symptom: consent flow completes, but ingestion/file selection never becomes enabled.
Summary by CodeRabbit
Bug Fixes
New Features
Chores
Tests