rusackas commented on code in PR #40695:
URL: https://github.com/apache/superset/pull/40695#discussion_r3364270481
##########
superset/security/manager.py:
##########
@@ -633,6 +633,12 @@ def create_login_manager(self, app: Flask) -> LoginManager:
return lm
def on_user_login(self, user: Any) -> None:
+ # pylint: disable=import-outside-toplevel
+ from superset.security.session_invalidation import stamp_login_time
+
+ # Record the authentication time so outstanding sessions can be
+ # invalidated when the account is later disabled.
+ stamp_login_time()
Review Comment:
Good call — added a patch of `stamp_login_time` to
`test_on_user_login_logs_event` and assert it's called once, alongside the
existing audit-log assertion. Done in 29e0ded865.
##########
tests/integration_tests/security/session_invalidation_tests.py:
##########
@@ -0,0 +1,78 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from superset import db
+from superset.models.user_attributes import UserAttribute
+from tests.integration_tests.base_tests import SupersetTestCase
+
+USERNAME = "session_invalidation_user"
+PASSWORD = "general" # noqa: S105
+
+
+class TestSessionInvalidation(SupersetTestCase):
+ def setUp(self) -> None:
+ self.create_user(
+ USERNAME,
+ PASSWORD,
+ "Admin",
+ email="[email protected]",
+ )
+ db.session.commit()
+
+ def tearDown(self) -> None:
+ if user := self.get_user(USERNAME):
+ db.session.query(UserAttribute).filter_by(user_id=user.id).delete()
+ db.session.delete(user)
+ db.session.commit()
+
+ def _set_active(self, active: bool) -> None:
+ # Update via the ORM so the after_update event fires (mirrors the
+ # FAB admin UI / REST API path that flips ``active``).
+ user = self.get_user(USERNAME)
+ user.active = active
+ db.session.commit()
+
+ def test_disabling_user_forces_logout_of_active_session(self) -> None:
+ self.login(USERNAME, PASSWORD)
+
+ # Authenticated request works before the account is disabled.
+ assert self.client.get("/api/v1/me/").status_code == 200
+
+ # Disabling the account stamps the per-user invalidation epoch...
+ self._set_active(False)
+ user = self.get_user(USERNAME)
+ attribute = (
+
db.session.query(UserAttribute).filter_by(user_id=user.id).one_or_none()
+ )
+ assert attribute is not None
+ assert attribute.sessions_invalidated_at is not None
+
+ # ...so the previously-authenticated session is now forced out.
+ assert self.client.get("/api/v1/me/").status_code != 200
Review Comment:
Agreed — tightened it to `== 401`. The `enforce_session_validity` hook
clears the session and continues anonymous, so the protected REST route
(`/api/v1/me/`) answers 401 specifically; asserting the exact code catches a
regression that turned a forced-logout into an unrelated 500/403. Fixed in
29e0ded865.
--
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]
