alitheg commented on code in PR #1150:
URL:
https://github.com/apache/tooling-trusted-releases/pull/1150#discussion_r3063060427
##########
atr/post/user.py:
##########
@@ -119,17 +119,16 @@ async def _cache_session(session: web.Committer) -> None:
session_data = {
"uid": session.uid,
- "dn": getattr(session, "dn", None),
- "fullname": getattr(session, "fullname", None),
- "email": getattr(session, "email", f"{session.uid}@apache.org"),
- "isMember": getattr(session, "isMember", False),
- "isChair": getattr(session, "isChair", False),
- "isRoot": getattr(session, "isRoot", False),
- "pmcs": getattr(session, "committees", []),
- "projects": getattr(session, "projects", []),
- "mfa": getattr(session, "mfa", False),
- "roleaccount": getattr(session, "isRole", False),
- "metadata": getattr(session, "metadata", {}),
+ "dn": session.dn,
+ "fullname": session.fullname,
+ "email": session.email,
+ "isMember": session.is_member,
+ "isChair": session.is_chair,
+ "isRoot": session.is_root,
Review Comment:
Did we say we didn't want to keep this on the session now? Or is that a
possible change for another time?
##########
atr/server.py:
##########
@@ -474,35 +475,45 @@ async def bind_request_context_vars() -> None:
@app.before_request
async def validate_session() -> None:
"""
- Check account is still active and augment cookie with additional
information
- Note - absolute session max lifetime (MAX_SESSION_AGE) is handled by
asfquart
+ Check account is still active via periodic LDAP liveness checks.
+ Absolute session max lifetime (MAX_SESSION_AGE) and idle timeout are
+ enforced by the session store during validate().
"""
- session = await asfquart.session.read()
- if session is None or session.uid is None:
+ session = await sessions.read()
+ if not isinstance(session, sql.UserSession):
return
+ quart.g.is_session_downgraded = session.downgrade_admin_to_user
+
conf = config.get()
account_check_interval = conf.ACCOUNT_CHECK_INTERVAL
- # Check if session has a check timestamp in metadata
- last_check = session.metadata.get("last_account_check")
+ # Check if session has a check timestamp
+ last_check = session.last_account_check
current_time = time.time()
- uid = str(session.uid)
+ uid = session.uid
if last_check is None or (current_time - last_check >
account_check_interval):
- # First time checking this session, record time
- session.metadata["last_account_check"] = current_time
if not await ldap.is_active(uid):
log.auth_failure("oauth", "account_deleted_or_banned", uid)
- asfquart.session.clear()
- raise base.ASFQuartException("Session expired", errorcode=401)
+ await asfquart.APP.sessions.revoke_by_uid(uid)
+ await asfquart.session.aclear()
+ sessions.invalidate_cache()
+ raise base.ASFQuartException("Account is disabled",
errorcode=401)
+
+ admin_uid = session.admin_uid
+ if isinstance(admin_uid, str) and admin_uid and (not await
ldap.is_active(admin_uid)):
+ log.auth_failure("oauth", "account_deleted_or_banned",
admin_uid)
+ await asfquart.APP.sessions.revoke_by_uid(admin_uid)
+ await asfquart.session.aclear()
+ sessions.invalidate_cache()
Review Comment:
These 3 (or possibly 4 including the exception) lines could be extracted as
I can see 4 times they're used identically
##########
atr/blueprints/common.py:
##########
@@ -54,24 +56,31 @@
async def authenticate() -> web.Committer:
- web_session = await asfquart.session.read()
- if web_session is None:
+ web_session = await sessions.read()
+ if not isinstance(web_session, sql.UserSession):
raise base.ASFQuartException("Not authenticated", errorcode=401)
- if (web_session.uid is None) or (not await
ldap.is_active(web_session.uid)):
- asfquart.session.clear()
+
+ if not await ldap.is_active(web_session.uid):
Review Comment:
Do we want to go to LDAP every time we validate a session in addition to the
periodic check in `before_request`? I assume this will be fine with the pubsub
implementation (so we have a cache rather than a live check?)
--
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]
