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

Secure Admin Accounts: Remove password for code studio admins #32234

Merged
merged 9 commits into from Dec 4, 2019

Conversation

nkiruka
Copy link
Contributor

@nkiruka nkiruka commented Dec 3, 2019

Description

Remove password for code studio admins since admins can only sign in via google authentication.

Links

Testing story

Dry run was performed and script was tested manually.

No. of users that have admin privileges that will be impacted below:

mysql> SELECT COUNT(*) FROM `users` WHERE `users`.`deleted_at` IS NULL AND `users`.`admin` = 1 AND (`users`.`encrypted_password` IS NOT NULL);
+----------+
| COUNT(*) |
+----------+
|       48 |
+----------+
1 row in set

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@nkiruka nkiruka changed the title remove password for code studio admins Remove password for code studio admins Dec 3, 2019
# Remove passwords for code studio admins

batch_number = 0
User.where(admin: true).where.not(encrypted_password: nil).find_in_batches(batch_size: 10) do |batch|
Copy link
Contributor

Choose a reason for hiding this comment

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

will this query complete in time? Given that you had to use SQL to execute the count query, I would assume the same problem will present itself when you try to run this script.

It also seems unnecessary to batch a query that we expect to run on fewer than 50 rows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this and went the route of using a transaction because it was considered a best practice. However, I do agree that it is not necessary in this case and that catching and logging the error will be more effective.

Copy link
Contributor

Choose a reason for hiding this comment

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

note that transactions are used for a different concern; specifically, they ensure that you can group queries together such that if any one query fails, all others will be rolled back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Hamms You mention concern with how long the query will take. Any ideas on how to tackle this?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the query is too expensive to fit in the 30 second timeout, it's recommended to write the script in such a way that it can accept a file containing a list of ids to update; you can then run the query against the reporting database to generate that list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

puts "PROCESSING: #{batch_number}..."
ActiveRecord::Base.transaction do
batch.each do |user|
user.update(encrypted_password: nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

recommend we use

Suggested change
user.update(encrypted_password: nil)
user.update!(encrypted_password: nil)

and catch and log errors


# Remove passwords for code studio admins

batch_number = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

we can remove batch_number here and in its other usages now 🎉

@nkiruka nkiruka marked this pull request as ready for review December 4, 2019 00:48

ADMIN_IDS.each do |admin_id|
ActiveRecord::Base.transaction do
User.where(id: admin_id).update!(encrypted_password: nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that ActiveRecord where returns a Relation. I think we can simplify this logic with find

Suggested change
User.where(id: admin_id).update!(encrypted_password: nil)
User.find(admin_id).update!(encrypted_password: nil)

@nkiruka
Copy link
Contributor Author

nkiruka commented Dec 4, 2019

@sureshc I tested your update and it works great! Thanks

@sureshc sureshc merged commit 44f7dc4 into staging Dec 4, 2019
@sureshc sureshc deleted the script-to-remove-passwords-for-admins branch December 4, 2019 21:27
@nkiruka nkiruka changed the title Remove password for code studio admins Secure Admin Accounts: Remove password for code studio admins Dec 7, 2019
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.

None yet

4 participants