sadpandajoe commented on code in PR #44036:
URL: https://github.com/apache/superset/pull/44036#discussion_r3964903123
##########
superset/commands/semantic_layer/update.py:
##########
@@ -71,6 +92,26 @@ def _unmask_configuration(
except (TypeError, ValueError):
existing_configuration = {}
+ masked_keys = {
+ key
+ for key, value in new_configuration.items()
+ if value == PASSWORD_MASK and key in existing_configuration
+ }
+ # `.get(key)` alone can't tell "key absent from storage" apart from "key
+ # present and stored as None" -- both return None -- so a newly
+ # introduced key with an explicit None value would be misread as
+ # unchanged and let a masked secret slip through alongside it. A
+ # sentinel default makes that distinction explicit.
+ if masked_keys and any(
Review Comment:
Removing a stored configuration key is not compared here because the check
only iterates submitted items, yet this update replaces the stored dictionary.
An editor can retain a masked secret while dropping an optional
destination-affecting field and thereby change the runtime configuration
without re-entering the secret. Could this also reject key-set removals while a
masked value is reused?
##########
superset/security/manager.py:
##########
@@ -5349,11 +5349,24 @@ def validate_guest_token_resources(resources:
GuestTokenResources) -> None:
embedded =
EmbeddedDashboardDAO.find_by_id(str(resource["id"]))
if not embedded:
raise EmbeddedDashboardNotFoundError()
+ dashboard = embedded.dashboard
elif not dashboard.embedded:
# A raw dashboard id must still reference an embedded
dashboard;
# otherwise a guest token could be scoped to a
non-embedded one.
raise EmbeddedDashboardNotFoundError()
+ # The caller minting the token must themselves be entitled to
+ # the dashboard being scoped. `grant_guest_token` is a
+ # coarse, instance-wide permission -- without this check, an
+ # operator who narrows it to a non-Admin role (a realistic
+ # "embedding backend service" grant) would let that
+ # principal mint a fully valid guest token for *any*
+ # embedded dashboard, not just ones they have access to.
+ try:
+ self.raise_for_access(dashboard=dashboard)
+ except SupersetSecurityException as ex:
+ raise EmbeddedDashboardAccessDeniedError() from ex
Review Comment:
This turns an expected authorization denial into an unhandled exception: the
guest-token endpoint only maps `EmbeddedDashboardNotFoundError` and validation
errors, so a scoped service account receives a 500 instead of a 403 when it
lacks dashboard access. Could the endpoint translate this access-denied
exception to a 403?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
