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

Remove project alumni from all environments. #2260

Open
wants to merge 1 commit into
base: master
from

Conversation

3 participants
@jim
Copy link
Contributor

commented Jun 11, 2019

Description

This PR removes users for folks no longer on the project.

Reviewer Notes

Is there anything you would like reviewers to give additional scrutiny?

Setup

$ make db_prod_migrations_run

and then check to see that the folks mentioned in the Pivotal story aren't in the database anywhere.

Code Review Verification Steps

  • Any new migrations/schema changes:
    • Secure migrations have been tested using scripts/run-prod-migrations

References

@jim jim requested a review from sarboc Jun 11, 2019

@chrisgilmerproj chrisgilmerproj self-requested a review Jun 11, 2019

@chrisgilmerproj
Copy link
Contributor

left a comment

These should set users.disabled=True instead of using DELETE. We have had bad experiences with DELETE which is why we added the disabled column.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

Here's what you'd want to use instead (pulled from 20190405161919_remove-patrick-s.sql):

UPDATE users SET disabled=true, is_superuser=false WHERE login_gov_email='patrick@truss.works';
@jim

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

@chrisgilmerproj I think there might be some confusion: the tsp_users table doesn't have a disabled column and so deletion is the only option there. The rows in the users table are bring updated by having disabled set to true.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

@chrisgilmerproj I think there might be some confusion: the tsp_users table doesn't have a disabled column and so deletion is the only option there. The rows in the users table are bring updated by having disabled set to true.

My understanding is that for service_members, office_users, and tsp_users that we're required to keep data for auditing reasons. Also the related models that rely on those tables have problems with CASCADE during deletion operations. So the users.disabled was added so we could simply disable them from being able to get a session on login and also so they wouldn't show up in any queries.

With Patrick we initially tried to use DELETE for all of his users in the various tables and that caused the migrations to fail in staging. So we reverted to just using the new table column which is good enough. It doesn't harm us to have extra users in the tables if they can't get a session. So we're pretty much good there.

@jim

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

@chrisgilmerproj I can add a disabled column to the other user tables in that case, but that will also require code changes in addition to just a migration. I guess it's possible this is the first time we've had to disable a TSP user, and this just hasn't come up before. Thanks for the feedback.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

@chrisgilmerproj I can add a disabled column to the other user tables in that case, but that will also require code changes in addition to just a migration. I guess it's possible this is the first time we've had to disable a TSP user, and this just hasn't come up before. Thanks for the feedback.

I don't think you need to add a new column. Just updating the users table should be enough to disable login by denying a session. All the apps use the same session middleware I think so that ought to suffice.

@sarboc

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

@chrisgilmerproj Per some digging TTV did for admin permissions, there might be multiple user records for any given email address, depending on environment. And, if a person hasn't logged in to a specific app yet, we won't have a user record at all. For example, if I never logged in to staging with one of my 3 TSP accounts, then I think after Jim's migration I would be able to do so.

I completely agree that we should keep users around for auditing purposes. But I'm not sure flipping the disabled flag on the users table is going to solve the problem the way we think it will.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

@chrisgilmerproj Per some digging TTV did for admin permissions, there might be multiple user records for any given email address, depending on environment. And, if a person hasn't logged in to a specific app yet, we won't have a user record at all. For example, if I never logged in to staging with one of my 3 TSP accounts, then I think after Jim's migration I would be able to do so.

I completely agree that we should keep users around for auditing purposes. But I'm not sure flipping the disabled flag on the users table is going to solve the problem the way we think it will.

You're right. In staging and experimental there are multiple users with the same email but different login.gov uuids. And I think you bring up a good point that some office and tsp users rows haven't been claimed int he users table. But if a user has been created and claimed an office or tsp user row then we shouldn't use DELETE.

So perhaps the migration has to be a bit more complicated. First check if an entry exists in users for that row and if so modify it to set disabled. If there isn't a corresponding row in users then DELETE is appropriate. I'd verify that it doesn't somehow delete the corresponding TSP or Office but it should be fine if that user never made any records.

@jim

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

For example, if I never logged in to staging with one of my 3 TSP accounts, then I think after Jim's migration I would be able to do so.

The rationale behind deleting rows from the tsp_users table was to handle this situation: if you hadn't logged in to the app yet, deleting those records is the only way to prevent you from logging in later and getting access as a TSP user. Without rows in the tsp_users table, though, you can only create a service member account.

I need to dig into the app a little bit to understand how we're querying for and creating records from the users table during initial signin. I am thinking that if we can put a row into the users table if it isn't already there, then we can deactivate users who haven't yet logged in.

I'd like to keep this kind of migration as simple as possible to minimize the chances of an oversight or simple mistake botching our attempts to control access in the future.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

I think we're all on the same page here. There are two problems to solve here:

  1. Disable any user accounts that have been "claimed" (ie they logged in via login.gov for a given office/tsp user)
  2. Delete only those accounts which have not been "claimed".

I think it's tricky figuring out #2 in the migration but we ought to be able to do it if we still want to proceed with deleting.

As for auditing, I think we ought to just have a "disabled" flag on office_users and tsp_users so we can prevent "claiming" and still keep those accounts around. It doesn't hurt us to keep records in our DB, it only hurts us if those can be used for nefarious purposes. If you'd like I'm happy to take on this part of the task today.

@jim

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

I think we ought to just have a "disabled" flag on office_users and tsp_users so we can prevent "claiming" and still keep those accounts around.

I like this solution as it means that all migrations are UPDATES and we can avoid adding conditionals to the migrations that revoke access.

If you'd like I'm happy to take on this part of the task today.

If you have bandwidth, I won't argue with that. 😄 I'm thinking we do a separate PR to get those additional columns in place and update any affected code, after which I will come back through and make changes to this one to bring it up to date.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

SGTM. I'll try to get that done this morning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.