Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[batch/auth] Set accounts to "inactive" after extended inactivity #14789

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

[batch/auth] Set accounts to "inactive" after extended inactivity #14789

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
8089cf8
Initial additional SQL for tracking user activity.
grohli Jan 9, 2025
49f6456
Changed 'deleted' to new 'inactive' state; made placeholder for 'inac…
grohli Jan 10, 2025
74faa27
rename placeholder table
grohli Jan 10, 2025
f1bfbbf
Added update for active users; added HTTP errror for inactive users.
grohli Jan 14, 2025
29ca3c3
Fixed SQL even to actually check for active users in WHERE
grohli Jan 14, 2025
7d26f4d
Changed inactivity timeout period to be 60 days
grohli Jan 14, 2025
5688e27
Removed redundant if statement check
grohli Jan 14, 2025
5d6f3e4
Added SQL, build.yaml modification for database migration
grohli Jan 15, 2025
7278280
Change last_active data type to DATETIME
grohli Jan 16, 2025
14711cb
Changed loop to update inactive users to be Python-side rather than i…
grohli Jan 22, 2025
f353d77
Refactor in auth/auth.py
grohli Jan 22, 2025
33378a6
changed NOW() to UTC_TIMESTAMP(3) for precision and region agnosticism.
grohli Jan 22, 2025
23b9b23
Type change for default last_activated field
grohli Jan 28, 2025
af186e4
Added descriptive error message for inactive user case in auth.py
grohli Jan 29, 2025
b7ba072
Merge branch 'main' into disable-inactive-accounts
grohli Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion auth/auth/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -769,7 +769,7 @@ async def get_userinfo_from_hail_session_id(request: web.Request, session_id: st
SELECT users.*
FROM users
INNER JOIN sessions ON users.id = sessions.user_id
WHERE users.state = 'active' AND sessions.session_id = %s AND (ISNULL(sessions.max_age_secs) OR (NOW() < TIMESTAMPADD(SECOND, sessions.max_age_secs, sessions.created)));
WHERE (users.state = 'active' OR users.state = 'inactive') AND sessions.session_id = %s AND (ISNULL(sessions.max_age_secs) OR (NOW() < TIMESTAMPADD(SECOND, sessions.max_age_secs, sessions.created)));
""",
session_id,
'get_userinfo',
Expand All @@ -778,6 +778,18 @@ async def get_userinfo_from_hail_session_id(request: web.Request, session_id: st

if len(users) != 1:
return None

if users[0]['state'] == 'active' and len(users) == 1:
grohli marked this conversation as resolved.
Show resolved Hide resolved
current_uid = users[0]['id']
await db.execute_update(
"""
UPDATE users
SET last_active = NOW()
WHERE id = %s;
""",
current_uid,
)

return typing.cast(UserData, users[0])


Expand Down
12 changes: 11 additions & 1 deletion auth/sql/estimated-current.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
CREATE TABLE `users` (
`id` INT(11) NOT NULL AUTO_INCREMENT,
`state` VARCHAR(100) NOT NULL,
-- creating, active, deleting, deleted
-- creating, active, deleting, deleted, inactive
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As Patrick mentioned, estimated-current is documentation, not something that actively runs. You'll want to add a migration, something like this (and referenced by build.yaml like this)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this in line with the migration you linked?

`username` varchar(255) NOT NULL COLLATE utf8mb4_0900_as_cs,
`login_id` varchar(255) DEFAULT NULL COLLATE utf8mb4_0900_as_cs,
`display_name` varchar(255) DEFAULT NULL,
Expand All @@ -16,11 +16,21 @@
-- namespace, for developers
`namespace_name` varchar(255) DEFAULT NULL,
`trial_bp_name` varchar(300) DEFAULT NULL,
-- "last active" tracking
`last_active` TIMESTAMP NOT NULL DEFAULT NOW(),
PRIMARY KEY (`id`),
UNIQUE KEY `email` (`email`),
UNIQUE KEY `username` (`username`)
) ENGINE=InnoDB;

CREATE EVENT `disable_inactive`

Check failure on line 26 in auth/sql/estimated-current.sql

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

auth/sql/estimated-current.sql#L26 

syntax error at or near "`"
grohli marked this conversation as resolved.
Show resolved Hide resolved
ON SCHEDULE EVERY 1 DAY
ON COMPLETION PRESERVE
DO
UPDATE users
SET users.state = 'inactive'
WHERE (users.state = 'active') AND (users.last_active IS NOT NULL) AND (DATEDIFF(CURRENT_DATE(), users.last_active) > 60);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably make this invalidation timer be configurable, and default to off


CREATE TABLE `sessions` (
`session_id` VARCHAR(255) NOT NULL,
`user_id` INT(11) NOT NULL,
Expand Down
2 changes: 2 additions & 0 deletions gear/gear/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ async def wrapped(request: web.Request) -> web.StreamResponse:
if redirect or (redirect is None and '/api/' not in request.path):
raise login_redirect(request)
raise web.HTTPUnauthorized()
elif userdata['state'] == 'inactive':
raise web.HTTPUnauthorized()
return await fun(request, userdata)

return wrapped
Expand Down