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.

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 12 commits into
base: main
Choose a base branch
from

Conversation

grohli
Copy link
Contributor

@grohli grohli commented Jan 14, 2025

Change Description

Addressing compliance requirement wherein Hail accounts must be disabled/marked as inactive after a certain number of days of inactivity. This is accomplished as follows (pending PR revisions):

  • A last_active column has been added to the users table, used for storing a given user's last active timestamp;
  • Anytime a user does anything requiring authorization, their last_active timestamp is updated to the current date/time;
  • An error is raised to the user if they attempt to do anything requiring authorization while their account has a status of 'inactive'; and
  • A SQL event has been added that checks the users table once a day, and any 'active' users are marked as 'inactive' if their last_active timestamp is more than 60 days old.

Security Assessment

  • This change has a medium security impact

Impact Description

This change entails a change to the users table schema and additional authorization-related checks thereof, but none of this is exposed to users and all happens internally.

(Reviewers: please confirm the security impact before approving)

@grohli grohli requested a review from sarahgibs January 14, 2025 20:11
Copy link
Collaborator

I don't have the batch context to review this thoroughly, but I do have some high level questions:

  • What's the longest a user can use any part of the system without triggering a new authorization request? Is that on the order of a day?
  • Do we have a process in mind for how to reactivate an inactive account? I'm sure we'll have occasional users who were working on something else for a few months, or were on leave, and want to resume using hail after being deactivated. I don't think the answer should be "a developer needs to manually modify the database". Maybe we should expose something in the developer web UI?
  • Is modifying estimated-current.sql enough? My understanding is that that's more for documentation, and isn't used in production. I think we need to add a database migration to add the new column? (This is out of my depth!) I also remember past team members having a healthy fear of changes to the database, does anybody feel confident they know how this will work?

auth/auth/auth.py Outdated Show resolved Hide resolved
auth/sql/estimated-current.sql Outdated Show resolved Hide resolved
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

@@ -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?

@cjllanwarne
Copy link
Collaborator

Responding to @patrick-schultz -

  • What's the longest a user can use any part of the system without triggering a new authorization request? Is that on the order of a day?
    • Ignoring background tasks (like jobs running in VMs), any user activity or API call would re-trigger an authorization request
  • Do we have a process in mind for how to reactivate an inactive account? I'm sure we'll have occasional users who were working on something else for a few months, or were on leave, and want to resume using hail after being deactivated. I don't think the answer should be "a developer needs to manually modify the database". Maybe we should expose something in the developer web UI?
    • I agree. This should probably be added to the users page for developer/admin users
  • Is modifying estimated-current.sql enough? My understanding is that that's more for documentation, and isn't used in production. I think we need to add a database migration to add the new column? (This is out of my depth!) I also remember past team members having a healthy fear of changes to the database, does anybody feel confident they know how this will work?
    • I referenced this inline but no, it's not enough just to change estimated-current.sql

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants