Add flask routes to auth validators by BenWilson2 · Pull Request #18486 · mlflow/mlflow

BenWilson2 · 2025-10-23T18:25:04Z

🛠 DevTools 🛠

Install mlflow from this PR

# mlflow
pip install git+https://github.com/mlflow/mlflow.git@refs/pull/18486/merge
# mlflow-skinny
pip install git+https://github.com/mlflow/mlflow.git@refs/pull/18486/merge#subdirectory=libs/skinny

For Databricks, use the following command:

%sh curl -LsSf https://raw.githubusercontent.com/mlflow/mlflow/HEAD/dev/install-skinny.sh | sh -s pull/18486/merge

Related Issues/PRs

#xxx

What changes are proposed in this pull request?

Adds auth validation to flask routes that do not have corresponding proto definitions (server local routes)

How is this PR tested?

Existing unit/integration tests
New unit/integration tests
Manual tests

Due to the nature of this fix, ask me if you want to see the repro and fix validation.

Does this PR require documentation update?

Release Notes

Is this a user-facing change?

No. You can skip the rest of this section.
Yes. Give a description of this change to be included in the release notes for MLflow users.

What component(s), interfaces, languages, and integrations does this PR affect?

Components

How should the PR be classified in the release notes? Choose one:

rn/none - No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section
rn/breaking-change - The PR will be mentioned in the "Breaking Changes" section
rn/feature - A new user-facing feature worth mentioning in the release notes
rn/bug-fix - A user-facing bug fix worth mentioning in the release notes
rn/documentation - A user-facing documentation change worth mentioning in the release notes

Should this PR be included in the next patch release?

Yes should be selected for bug fixes, documentation updates, and other small changes. No should be selected for new features and larger changes. If you're unsure about the release classification of this PR, leave this unchecked to let the maintainers decide.

What is a minor/patch release?

Minor release: a release that increments the second part of the version number (e.g., 1.2.0 -> 1.3.0).
Bug fixes, doc updates and new features usually go into minor releases.
Patch release: a release that increments the third part of the version number (e.g., 1.2.0 -> 1.2.1).
Bug fixes and doc updates usually go into patch releases.

Yes (this PR will be cherry-picked and included in the next patch release)
No (this PR will be included in the next minor release)

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

github-actions · 2025-10-23T18:37:07Z

Documentation preview for c0af4e1 is available at:

https://pr-18486--mlflow-docs-preview.netlify.app/docs/latest/

More info

Ignore this comment if this PR does not change the documentation.
The preview is updated when a new commit is pushed to this PR.
This comment was created by this workflow run.
The documentation was built by this workflow run.

WeichenXu123 · 2025-10-24T00:50:27Z

mlflow/server/auth/__init__.py

+        (SEARCH_DATASETS, "POST"): validate_can_search_datasets,
+        (CREATE_PROMPTLAB_RUN, "POST"): validate_can_create_promptlab_run,
+        (GATEWAY_PROXY, "GET"): validate_gateway_proxy,
+        (GATEWAY_PROXY, "POST"): validate_gateway_proxy,


what's this for ? the validator always return True

Purely for consistency so that we have all server routes listed.

Could we remove them? It's confusing since it does nothing, will it cause CVE?

I think Ben's purpose is to force every path has a validator, an empty validator can be a placeholder (so that we won't missing permission validation for any endpoint path)

Q: can we make the request failing if no validator is found for a request path ? ( in

mlflow/mlflow/server/auth/__init__.py

Line 583 in 27c95c3

def _find_validator(req: Request) -> Callable[[], bool] | None:

)

WeichenXu123 · 2025-10-24T01:15:10Z

One idea (does not block this PR):

Current approach is to add validator for each endpoint path, which is easy to cause omissions,
The following way might be better.

when a request comes, put the request username in a thread-local variable,
then in SqlAlchemyStore.get_run / SqlAlchemyStore.get_experiment method, we can validate the related experiment permission setting with the username set in the thread-local variable, if validation fails, raise permission error.

serena-ruan · 2025-10-29T09:22:26Z

/review

🚀 Review workflow started

✅ Review completed.

Review Output

Perfect! I've successfully reviewed PR #18486 and added 12 review comments. The review found:

Style Guide Issues:

Multiple redundant docstrings that merely repeat function names (violations of MLflow's Python style guide)
Several functions with docstrings that could be more concise or converted to regular comments

Potential Bug:

The validate_gateway_proxy() function has a docstring that mentions "Returns empty list" but the function returns a boolean True, suggesting a documentation error

All comments have been posted to the PR with clickable suggestion blocks where applicable, making it easy for maintainers to apply the fixes with one click.

github-actions · 2025-10-29T09:24:09Z

mlflow/server/auth/__init__.py

+    """
+    Get permission for Flask routes that use either run_id or run_uuid parameter.
+    """


Per the Python style guide, this docstring is redundant - the function name already clearly indicates it gets permission from run_id or run_uuid parameter. Consider removing it.

Suggested change

"""

Get permission for Flask routes that use either run_id or run_uuid parameter.

"""

def _get_permission_from_run_id_or_uuid() -> Permission:

🤖 Generated with Claude Code

Yeah could we remove these useless docstrings?

github-actions · 2025-10-29T09:24:11Z

mlflow/server/auth/__init__.py

+def _get_permission_from_model_version() -> Permission:
+    """
+    Get permission for model version artifacts.
+    Model versions inherit permissions from their registered model.
+    """


Per the Python style guide, this docstring is redundant. The function name already conveys what it does, and the inheritance comment can be moved to a regular comment if needed.

Suggested change

def _get_permission_from_model_version() -> Permission:

"""

Get permission for model version artifacts.

Model versions inherit permissions from their registered model.

"""

def _get_permission_from_model_version() -> Permission:

# Model versions inherit permissions from their registered model.

🤖 Generated with Claude Code

github-actions · 2025-10-29T09:24:13Z

mlflow/server/auth/__init__.py

+def _get_permission_from_trace_request_id() -> Permission:
+    """
+    Get permission for trace artifacts.
+    Traces inherit permissions from their parent run/experiment.
+    """


Per the Python style guide, this docstring is redundant. The function name is self-documenting, and the inheritance comment can be a regular comment if needed.

Suggested change

def _get_permission_from_trace_request_id() -> Permission:

"""

Get permission for trace artifacts.

Traces inherit permissions from their parent run/experiment.

"""

def _get_permission_from_trace_request_id() -> Permission:

# Traces inherit permissions from their parent run/experiment.

🤖 Generated with Claude Code

github-actions · 2025-10-29T09:24:14Z

mlflow/server/auth/__init__.py

+def validate_can_read_run_artifact():
+    """Validator for /get-artifact Flask route"""
+    return _get_permission_from_run_id_or_uuid().can_read


Per the Python style guide, this docstring merely repeats what the function name already indicates. Consider removing it.

Suggested change

def validate_can_read_run_artifact():

"""Validator for /get-artifact Flask route"""

return _get_permission_from_run_id_or_uuid().can_read

def validate_can_read_run_artifact():

return _get_permission_from_run_id_or_uuid().can_read

🤖 Generated with Claude Code

github-actions · 2025-10-29T09:24:16Z

mlflow/server/auth/__init__.py

+def validate_can_update_run_artifact():
+    """Validator for /upload-artifact Flask route"""


Per the Python style guide, this docstring merely repeats what the function name already indicates. Consider removing it.

Suggested change

def validate_can_update_run_artifact():

"""Validator for /upload-artifact Flask route"""

def validate_can_update_run_artifact():

return _get_permission_from_run_id_or_uuid().can_update

🤖 Generated with Claude Code

github-actions · 2025-10-29T09:24:18Z

mlflow/server/auth/__init__.py

+def validate_can_read_model_version_artifact():
+    """Validator for /model-versions/get-artifact Flask route"""
+    return _get_permission_from_model_version().can_read


Per the Python style guide, this docstring merely repeats what the function name already indicates. Consider removing it.

Suggested change

def validate_can_read_model_version_artifact():

"""Validator for /model-versions/get-artifact Flask route"""

return _get_permission_from_model_version().can_read

def validate_can_read_model_version_artifact():

return _get_permission_from_model_version().can_read

🤖 Generated with Claude Code

github-actions · 2025-10-29T09:24:19Z

mlflow/server/auth/__init__.py

+def validate_can_read_trace_artifact():
+    """Validator for /ajax-api/2.0/mlflow/get-trace-artifact Flask route"""
+    return _get_permission_from_trace_request_id().can_read


Per the Python style guide, this docstring merely repeats what the function name already indicates. Consider removing it.

Suggested change

def validate_can_read_trace_artifact():

"""Validator for /ajax-api/2.0/mlflow/get-trace-artifact Flask route"""

return _get_permission_from_trace_request_id().can_read

def validate_can_read_trace_artifact():

return _get_permission_from_trace_request_id().can_read

🤖 Generated with Claude Code

github-actions · 2025-10-29T09:24:21Z

mlflow/server/auth/__init__.py

+def validate_can_read_metric_history_bulk():
+    """
+    Validator for /ajax-api/2.0/mlflow/metrics/get-history-bulk Flask route.
+    Checks READ permission on all requested runs.
+    """


Per the Python style guide, consider making this docstring more concise or removing it if the function name is self-explanatory. The "Checks READ permission on all requested runs" part is valuable context though.

Suggested change

def validate_can_read_metric_history_bulk():

"""

Validator for /ajax-api/2.0/mlflow/metrics/get-history-bulk Flask route.

Checks READ permission on all requested runs.

"""

def validate_can_read_metric_history_bulk():

"""Checks READ permission on all requested runs."""

🤖 Generated with Claude Code

github-actions · 2025-10-29T09:24:22Z

mlflow/server/auth/__init__.py

+def validate_can_read_metric_history_bulk_interval():
+    """
+    Validator for /ajax-api/2.0/mlflow/metrics/get-history-bulk-interval Flask route.
+    Same logic as bulk metrics endpoint.
+    """
+    return validate_can_read_metric_history_bulk()


Per the Python style guide, this docstring is redundant. The function call itself shows it has the same logic.

Suggested change

def validate_can_read_metric_history_bulk_interval():

"""

Validator for /ajax-api/2.0/mlflow/metrics/get-history-bulk-interval Flask route.

Same logic as bulk metrics endpoint.

"""

return validate_can_read_metric_history_bulk()

def validate_can_read_metric_history_bulk_interval():

# Same logic as bulk metrics endpoint

return validate_can_read_metric_history_bulk()

🤖 Generated with Claude Code

github-actions · 2025-10-29T09:24:24Z

mlflow/server/auth/__init__.py

+def validate_can_search_datasets():
+    """
+    Validator for /ajax-api/2.0/mlflow/experiments/search-datasets Flask route.
+    Checks READ permission on the experiment.
+    """


Per the Python style guide, consider making this docstring more concise. The "Checks READ permission on the experiment" part is the valuable context.

Suggested change

def validate_can_search_datasets():

"""

Validator for /ajax-api/2.0/mlflow/experiments/search-datasets Flask route.

Checks READ permission on the experiment.

"""

def validate_can_search_datasets():

"""Checks READ permission on the experiment."""

🤖 Generated with Claude Code

github-actions · 2025-10-29T09:24:25Z

mlflow/server/auth/__init__.py

+def validate_can_create_promptlab_run():
+    """
+    Validator for /ajax-api/2.0/mlflow/runs/create-promptlab-run Flask route.
+    Checks UPDATE permission on the experiment (same as CreateRun).


Per the Python style guide, consider making this docstring more concise. The key information is the UPDATE permission requirement.

Suggested change

def validate_can_create_promptlab_run():

"""

Validator for /ajax-api/2.0/mlflow/runs/create-promptlab-run Flask route.

Checks UPDATE permission on the experiment (same as CreateRun).

def validate_can_create_promptlab_run():

"""Checks UPDATE permission on the experiment (same as CreateRun)."""

🤖 Generated with Claude Code

github-actions · 2025-10-29T09:24:27Z

mlflow/server/auth/__init__.py

+def validate_gateway_proxy():
+    """
+    Validator for /ajax-api/2.0/mlflow/gateway-proxy Flask route.
+
+    This endpoint proxies requests to MLflow Deployments (AI Gateway) external services.
+    Returns empty list if MLFLOW_DEPLOYMENTS_TARGET is not configured.
+    No MLflow resource permissions are checked since this proxies to external services
+    that handle their own authorization.
+    """
+    return True


This docstring provides valuable context explaining why the function returns True and the security model. This is appropriate to keep as it explains non-obvious behavior.

However, there's a potential issue: the docstring mentions "Returns empty list if MLFLOW_DEPLOYMENTS_TARGET is not configured" but the function returns True (a boolean), not a list. This seems like a copy-paste error in the docstring. Consider clarifying what this function's behavior actually is.

🤖 Generated with Claude Code

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

serena-ruan · 2025-11-13T02:27:54Z

mlflow/server/auth/__init__.py

+        (GET_METRIC_HISTORY_BULK, "GET"): validate_can_read_metric_history_bulk,
+        (GET_METRIC_HISTORY_BULK_INTERVAL, "GET"): validate_can_read_metric_history_bulk_interval,
+        (SEARCH_DATASETS, "POST"): validate_can_search_datasets,
+        (CREATE_PROMPTLAB_RUN, "POST"): validate_can_create_promptlab_run,


Insteresting, is this still valid?

should be valid, these endpoints all exist

good point! I'll verify if this has any issues before merging.

confirmed the route still exists and is viable.

serena-ruan

LGTM once https://github.com/mlflow/mlflow/pull/18486/files#r2520933037 is addressed/answered! If it's for consistency only then I think we don't need it :)

WeichenXu123

LGTM

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com> Signed-off-by: Tian Lan <sky.blue266000@gmail.com>

Add flask routes to validators

7fce5c6

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

BenWilson2 added the team-review Trigger a team review request label Oct 23, 2025

github-actions bot added v3.5.2 area/tracking Tracking service, tracking client APIs, autologging rn/bug-fix Mention under Bug Fixes in Changelogs. labels Oct 23, 2025

github-actions bot requested review from B-Step62, TomeHirata, WeichenXu123, daniellok-db, harupy, kevin-lyn, serena-ruan and xq-yin October 23, 2025 18:25

WeichenXu123 reviewed Oct 24, 2025

View reviewed changes

github-actions bot reviewed Oct 29, 2025

View reviewed changes

style

2009ffd

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

BenWilson2 requested a review from WeichenXu123 October 31, 2025 14:03

Merge branch 'master' into artifact-security

5cd781a

serena-ruan reviewed Nov 13, 2025

View reviewed changes

serena-ruan approved these changes Nov 13, 2025

View reviewed changes

WeichenXu123 approved these changes Nov 13, 2025

View reviewed changes

Merge branch 'master' into artifact-security

c0af4e1

BenWilson2 added v3.6.1 and removed v3.5.2 labels Nov 13, 2025

BenWilson2 enabled auto-merge November 13, 2025 05:01

BenWilson2 added this pull request to the merge queue Nov 13, 2025

Merged via the queue into mlflow:master with commit ca9ab39 Nov 13, 2025
50 of 51 checks passed

BenWilson2 deleted the artifact-security branch November 13, 2025 05:43

Tian-Sky-Lan pushed a commit to Tian-Sky-Lan/mlflow that referenced this pull request Nov 24, 2025

Add flask routes to auth validators (mlflow#18486)

c188a24

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com> Signed-off-by: Tian Lan <sky.blue266000@gmail.com>

		def validate_can_update_run_artifact():
		"""Validator for /upload-artifact Flask route"""

Conversation

BenWilson2 commented Oct 23, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Install mlflow from this PR

Related Issues/PRs

What changes are proposed in this pull request?

How is this PR tested?

Does this PR require documentation update?

Release Notes

Is this a user-facing change?

What component(s), interfaces, languages, and integrations does this PR affect?

How should the PR be classified in the release notes? Choose one:

Should this PR be included in the next patch release?

Uh oh!

github-actions bot commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WeichenXu123 Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

BenWilson2 Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

serena-ruan Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

WeichenXu123 Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WeichenXu123 Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

WeichenXu123 commented Oct 24, 2025

Uh oh!

serena-ruan commented Oct 29, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

serena-ruan Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

serena-ruan Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

BenWilson2 commented Oct 23, 2025 •

edited by github-actions bot

Loading

github-actions bot commented Oct 23, 2025 •

edited

Loading

WeichenXu123 Nov 13, 2025 •

edited

Loading

serena-ruan commented Oct 29, 2025 •

edited by github-actions bot

Loading

WeichenXu123 Nov 13, 2025 •

edited

Loading

BenWilson2 Nov 13, 2025 •

edited

Loading