bito-code-review[bot] commented on code in PR #40671:
URL: https://github.com/apache/superset/pull/40671#discussion_r3385379929
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Catching generic Exception is risky</b></div>
<div id="fix">
The bare `Exception` is caught which may hide unexpected errors. Consider
catching more specific exceptions like `KeyError` and `ValueError` instead, or
document why a broad catch is intentional.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
value = get_shared_value(SharedKey.GUEST_TOKEN_REVOCATION_VERSION)
except (KeyError, ValueError, TypeError):
# 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)
````
</div>
</details>
</div>
<small><i>Code Review Run #6a6dcb</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
