rusackas commented on code in PR #40671:
URL: https://github.com/apache/superset/pull/40671#discussion_r3385607640
##########
superset/security/guest_token.py:
##########
@@ -14,13 +14,70 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import logging
from typing import Optional, TypedDict, Union
from flask_appbuilder.security.sqla.models import Group, Role
from flask_login import AnonymousUserMixin
from superset.utils.backports import StrEnum
+logger = logging.getLogger(__name__)
+
+# JWT claim that carries the revocation version a token was minted with.
+GUEST_TOKEN_REVOCATION_CLAIM = "rev" # noqa: S105
+
+# Tokens minted before the revocation feature existed carry no version claim.
+# They are treated as version 0, which is only rejected once an admin has
+# explicitly bumped the expected version above 0.
+DEFAULT_GUEST_TOKEN_REVOCATION_VERSION = 0
+
+
+def get_current_guest_token_revocation_version() -> int:
+ """
+ Return the minimum guest-token revocation version accepted at validation
time.
+
+ The value is stored in the metadata database via the ``key_value`` store
so it
+ can be bumped at runtime (e.g. via the ``revoke-guest-tokens`` CLI command)
+ without a code deploy. A missing entry means revocation has never been
+ triggered, so the default version is returned.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset.key_value.shared_entries import get_shared_value
+ from superset.key_value.types import SharedKey
+
+ try:
+ value = get_shared_value(SharedKey.GUEST_TOKEN_REVOCATION_VERSION)
+ except Exception: # pylint: disable=broad-except
+ # Never let a metadata-store hiccup hard-fail token validation; fall
back
+ # to the default version (fail-open to the pre-feature behaviour).
+ logger.warning("Could not read guest token revocation version",
exc_info=True)
Review Comment:
Thanks, but narrowing this catch would defeat the purpose of the block. The
intent here is explicitly fail-open on a metadata-store outage so a transient
DB hiccup never hard-fails guest-token validation (the inline comment and the
`# pylint: disable=broad-except` call this out). The failures we are guarding
against are database/connection errors raised by `get_shared_value` (e.g.
`SQLAlchemyError`/`OperationalError`), not `KeyError`/`ValueError`/`TypeError`
— those latter cases are actually handled separately by the `int(value)`
conversion just below. Restricting the catch to `(KeyError, ValueError,
TypeError)` would let a real store outage propagate and take down all embedded
access, which is exactly the outcome this is designed to avoid. Keeping the
broad catch intentionally.
--
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]
