Spring cleaning users #1006

emcoding · 2018-05-25T16:23:22Z

Related issue #991

On the way to separating authentication from authorisation:

Straighten out the authorisation in User: now we authorise all user actions, so that the abilities can take care what is shown to who (for example: we want a supervisor to be able to see email addresses of team members in the show view, even if the team member choose to hide their email from regular users).
Added a feature test for the sign in process for unconfirmed users.

Also:

UX: Added a client side validation for the city field, which already has a server side validation
Database: Added migration to remove users without a github_handle.
Why? For accounts imported from GitHUb, the github_id is not nil. So if the id is nil, it is a fake
account or an input error. Related issue Add rake task to strip down production database dump #979
@klappradla I never used a migration for removing data before. I copied one of your examples, can you double double check please?
FIX: only confirmed users can read email addresses in users index and show

ALERT

In Dev env, running the migration will remove the seeded users from your local database. If you want some of them to persist, give them a fake github_id < 100.
Run rails db:migrate and rails db:seed

Background info:

Cancancan's load_and_authorize_resource will load the resource, unless it is already set. That means: we don't need the separate before_action: set_resource for authorised actions. (Note that my previous description about this behaviour where not 100% accurate 😇 )

Follow up:

Update the hint in the form, and tell the user that a supervisor and admin will be able to see email, whether marked hidden or not. Check first if this statement is and should be true.
Add message to slack dev channel that all user-seeds will be removed with the latest migration.

We added a rule that all users `can :read User`; we can choose to not authorize those actions, or check the abilities anyway.

City field has a presence validation, but that was not visible in the view.

#979) For real github accounts, the github_id is not nil. So if the id is nil, it is a fake account or an input error. Updated seeds with github_id

…show views

Solved in cancancan Fixed in cancancan: ryanb/cancan#835 (comment)

emcoding · 2018-05-26T07:54:30Z

This is ready for review 🙇

hola-soy-milk

Looks great to me. Thank you so much!

hola-soy-milk · 2018-05-26T09:11:52Z

app/controllers/application_controller.rb

-    method = "#{resource}_params"
-    params[resource] &&= send(method) if respond_to?(method, true)
-  end
-


pgaspar

I think this is heading in the right direction 🙌 🚀

Left some comments on things I think we should improve before merging 👍

pgaspar · 2018-05-27T22:32:14Z

app/views/community/index.html.slim

@@ -31,8 +31,7 @@ table.users.table.table-striped.table-bordered.table-condensed.table-hover
        td.image = avatar_url(user, size: 40)
        td = link_to user.name_or_handle, user
        td = user.interested_in.map { |key| User::INTERESTS[key].presence }.compact.join(', ')
-        - if signed_in?
-          td = mail_to user.email unless user.hide_email?
+        td = mail_to user.email if can?(:read, user.email)


I think there's a bug here: this keeps the td even if the can? method returns false, which breaks the layout a bit. (Try it with a logged out or unconfirmed user.)

I think you need to change this back to be a 2-line conditional.

Also, as pointed out below, I think the check should be can?(:read_email, user).

Oiii, thanks for catching this. Turns out I was solving the wrong problem. Fixing the ability rules also fixed the layout and what I was trying to do with the new ability rule.

pgaspar · 2018-05-27T22:34:55Z

app/views/community/index.html.slim

@@ -16,7 +16,7 @@ table.users.table.table-striped.table-bordered.table-condensed.table-hover
      th
      th = sortable :name
      th = sortable :interested_in
-      - if signed_in?
+      - if current_user&.confirmed?


I think we should make this check be the same can? check as you have below. What do you think?

🤔To use the can? helper , I'd need access to the single user, as I do have below. I'd think this hack is better than alternatives, like adding an extra @user ivar? Or...?
Neat it is not. 😢

Hmm, good point. I wonder what people usually do in these circumstances - we are kinda duplicating authorization knowledge here.

Also this seems to be slightly different from what we have specified in the Ability class: from what I understand, confirmed users can: can :read_email, User, hide_email: false, can't they? (I may be missing something here! I'm not too familiar with CanCan).

I hope we'll have a nicer solution at the end of the Overhaul. And I'll keep looking for a better solution.

Saw your recent change to Ability 👍 One thing that seems to work is - if can?(:read_email, nil).

That didn't work for me, not as a twoliner, and not as a oneliner :-(
See next commits.

pgaspar · 2018-05-27T22:42:31Z

db/migrate/20180524190818_destroy_non_existing_git_hub_users.rb

@@ -0,0 +1,5 @@
+class DestroyNonExistingGitHubUsers < ActiveRecord::Migration[5.1]
+  def up
+    execute 'DELETE FROM users WHERE github_id IS NULL'


Some considerations 🤔:

First, I think we may want to prevent this from running on development to avoid the issue you pointed out on the PR description. I worry this may cause unexpected behavior for existing developers who update their local copy of the app.

Wrapping the execute statement on a unless Rails.env.development? should do it, I think 👍

Second point: do these users have any relationships with other models in the database? If they do, maybe we should consider doing this using Ruby so the destroy callbacks run. User.where(github_id: nil).destroy_all. I don't have access to production data to see if this is the case or not.

I like the ruby destroy better anyway.

Are you sure about not running the migration in dev env? I'd think that we want the user objects in the local database not to be different from the test and production databases?

Then maybe we could generate github_ids for users without one in development to make them be compatible without deleting them?

I intended to add a 'how-to' to the readme or troubleshooting guide. Or should I add it to the migration? Add a part to add gh_ids if env == dev?

Sorry, I missed this comment yesterday.

pgaspar · 2018-05-27T22:48:42Z

spec/controllers/community_controller_spec.rb

-        expect(response.body).not_to include user_opted_out.email
+        # expect(response.body).to include user.email
+        # This expectation ^ fails, but should this be tested in a controller test in
+        # the first place?


I'm confused 😕 What changed in this PR to make this expectation start failing?

Ah, I think this really is an issue with your changes:

# Shouldn't this: can?(:read, user.email) # Be this: can?(:read_email, user)

Yes, it should! See other comment. 💃

pgaspar · 2018-05-27T22:57:36Z

spec/models/ability/guest_spec.rb

@@ -15,6 +15,8 @@

    it_behaves_like 'has access to public features'

+    it { expect(subject).not_to be_able_to(:read, other_user.email) }


Heads up I believe this should be be_able_to(:read_email, other_user). Same below.

I thought this is testing the desired outcome of the read_email rule? No?

I'm not experienced with CanCan, but looking at how the rule is defined, using read_email instead of read, I would assume you need to use read_email here as well. Also, the rule definition does not receive an email, right? It receives a User.

But it should be testable. I made these changes and the spec fails:

Using this makes it pass:

it { expect(subject).to be_able_to(:read_email, other_user) }

You are right. I remember what I did here. The thing is, the read_email rule is not yet renewed yet. So it is hard to test it (and use it in the community view) the right way.
The one thing I wanted to guard is that a guest user can not read emails. I am 98% sure that the test is testing the right thing here. 😇
See my general comment for a proposal on how to handle this.

Still think this is a false positive @F3PiX 😟 I wasn't able to make be_able_to(:read, other_user.email) return true even for cases where it should return true, as I showed above.

This is the same error you did initially, where you were testing things in the view as can?(:read, user.email) and it didn't work and we changed it to can?(:read_email, user). It makes sense that the same logic would apply here in the expectation.

O yes. So sorry. You are right.

emcoding · 2018-05-28T11:25:54Z

@pgaspar Back to you!
Heads up: if you pulled the branch and ran the migration, you may want to rollback that migration first.

pgaspar

Thanks for your changes @F3PiX!

Added some more comments. Thanks for bearing with me 😅

pgaspar · 2018-05-28T11:39:28Z

app/views/community/index.html.slim

@@ -16,7 +16,7 @@ table.users.table.table-striped.table-bordered.table-condensed.table-hover
      th
      th = sortable :name
      th = sortable :interested_in
-      - if signed_in?
+      - if current_user&.confirmed?


Saw your recent change to Ability 👍 One thing that seems to work is - if can?(:read_email, nil).

pgaspar · 2018-05-28T11:41:09Z

app/views/community/index.html.slim

@@ -31,8 +31,7 @@ table.users.table.table-striped.table-bordered.table-condensed.table-hover
        td.image = avatar_url(user, size: 40)
        td = link_to user.name_or_handle, user
        td = user.interested_in.map { |key| User::INTERESTS[key].presence }.compact.join(', ')
-        - if signed_in?
-          td = mail_to user.email unless user.hide_email?
+        td = mail_to user.email if can?(:read_email, user)


This still needs to change to this form:

- if can?(:read_email, user) td = mail_to user.email

Otherwise, this happens when you're logged out (or when the can? check returns false):

(There's a ghost column under the Github table header).

🙈 I thought I had it right. Sorry.
I didn't miss your suggestion, I just found that it gave problems when a user as their email hidden. Look:

(first user hides email).

I will look again!!

Ah, that's a great point - I'm sorry I missed that!

I haven't tested this, but maybe something like this would work:

- if can?(:read_email, nil) td = mail_to user.email if can?(:read_email, user)

The idea being that the first check will be false for unconfirmed / logged out users, while being true for other users. It's not the prettiest thing every, but it may solve the issue without us needing to explicitly check if the user is confirmed (which duplicates knowledge that unconfirmed/logged out users should not see emails).

pgaspar · 2018-05-28T11:50:19Z

db/migrate/20180524190818_destroy_non_existing_git_hub_users.rb

+  def up
+    unless ENV['RAILS_ENV'] == 'development'
+      User.find_each do |user|
+        if user.github_id == nil


I'm curious why you're doing this like that instead of filtering users upfront.

User.where(github_id: nil).find_each do |user| puts "removing user #{user.id} from database" user.destroy end

It would avoid needing to go through all the users in the database.

pgaspar · 2018-05-28T12:07:34Z

db/migrate/20180524190818_destroy_non_existing_git_hub_users.rb

+      User.find_each do |user|
+        if user.github_id == nil
+          puts "removing user #{user.id} from database"
+          user.destroy


This may not matter much, depending on the number of records we have, but I think this version leads to better performance:

User.where(github_id: nil).destroy_all.each do |user| puts "removed user #{user.id} from database" end

destroy_all returns the records that were removed, so we can use those to print stuff.

PS: did a quick one-run test and for 5000 users this method ran in ~23 seconds while the inline user.destroy method ran in ~41 seconds. That being said, this probably doesn't matter at all since we don't have a lot of records being destroyed! 😅

pgaspar · 2018-05-28T12:12:48Z

spec/controllers/community_controller_spec.rb

@@ -36,7 +36,8 @@
        user_opted_out = create(:user, hide_email: true)
        get :index
        expect(response.body).to include user.email
-        expect(response.body).not_to include user_opted_out.email
+        # Should this be tested in a controller test?
+        expect(response.body).not_to include user_opted_out.email # false negative; also passes with hide_email: false


@F3PiX I may be missing something here, but I think this may not be a false negative. Changing user_opted_out to have hide_email: false will make the test fail.

pgaspar · 2018-05-28T12:15:25Z

spec/features/users/guest_user_spec.rb

-  let!(:activity)      { create(:status_update, :published, team: team1) }
-  let!(:other_user)    { create(:user) }
+  let!(:activity) { create(:status_update, :published, team: team1) }
+  let(:other_user) { create(:user, hide_email: false) }


Isn't this change unnecessary? Removing it does not make the spec fail (I assume hide_email defaults to false).

Are you doing it for clarity? Looks good if you are 👍

pgaspar · 2018-05-28T12:19:31Z

spec/features/users/unconfirmed_sign_in_user_spec.rb

+
+    it 'nothing happens when clicking on resend link' do
+      find_link('Click here to resend the email.').click
+      # todo: expect_to happen: something visible


Maybe check that an email was enqueued here or something?

I mean there is no visual feedback to the user.

Yep, I understood that part 😅 I was just saying that maybe we should also check that an email was enqueued, because that's what happens, right? Or maybe it isn't? I'm a bit confused by the spec name nothing happens when clicking on resend link.

pgaspar · 2018-05-28T12:26:47Z

spec/models/ability/guest_spec.rb

@@ -15,6 +15,8 @@

    it_behaves_like 'has access to public features'

+    it { expect(subject).not_to be_able_to(:read, other_user.email) }


I'm not experienced with CanCan, but looking at how the rule is defined, using read_email instead of read, I would assume you need to use read_email here as well. Also, the rule definition does not receive an email, right? It receives a User.

But it should be testable. I made these changes and the spec fails:

Using this makes it pass:

it { expect(subject).to be_able_to(:read_email, other_user) }

… field's

emcoding · 2018-05-28T19:06:46Z

@pgaspar Sorry for keeping you busy 🙏
What I did: I made it so that the column title is hiding now in sync with the email's visibility. That means, we have a stray mini column for guests and unconfirmed users.
I rather left it as is for now, because both the authentication is not finished, and the ability rule :read_email is not renewed. The thing is: I couldn't leave it alone, because the email addresses got exposed to guest users. My priority was to prevent that.
We already have an issue for the :read_email rule (I'll update that one with our acquired knowledge and questions).
Is that acceptable? All the changes are interrelated, so it is hard to not change everything at once, and impossible to change one thing at a time :-) I am very open to suggestions on how to handle this situation better!

emcoding · 2018-05-28T19:09:18Z

For confirmed users it looks like this now:

For unconfirmed and guests like this 💩

pgaspar · 2018-05-28T19:12:58Z

Thank you @F3PiX 👍 And thanks for your patience.

If you're willing to try one last solution, this should get rid of the empty column for guests:

- if current_user&.confirmed?
  th Email

/ ...

- if current_user&.confirmed?
  td = mail_to user.email if can?(:read_email, user)

Did a quick test and it seemed to work (and it should work, looking at the code).

emcoding · 2018-05-29T09:37:21Z

@pgaspar I updated the specs; now they do test the read_email rule. Thanks.
One thing left to do:
[ ] add github_ids to users in dev env
Should I add it to the migration? Add a separate migration? WDYT?

pgaspar · 2018-05-29T10:50:23Z

Great work @F3PiX 👍

As for the github_ids, I think both options are fine, so whatever you prefer 😸 There is probably a way to do this with SQL directly using Random() or something, but you can do it with Ruby as well...

pgaspar · 2018-05-30T17:03:08Z

db/migrate/20180524190818_destroy_non_existing_git_hub_users.rb

+      users = User.all.select { |user| user.github_id.nil? }
+      users.each_with_index do |user, index|
+        user.update!(github_id: index + 10)
+      end


@F3PiX select is doing this in ruby which is slow 😟 users = User.where(github_id: nil) does the same thing but in the database, which is fast!

users.each_with_index is not great for memory because it loads every record without a github id at the same time into memory.

Here's an alternative:

User.where(github_id: nil).find_each(batch_size: 200).with_index do |user, index| user.update!(github_id: index + 10) end

You can find a similar example in the docs here: http://api.rubyonrails.org/classes/ActiveRecord/Batches.html#method-i-find_each

emcoding · 2018-05-30T19:32:10Z

@pgaspar I got distracted before I submitted my comment.
I was going to say:

I put it in the same migration, so that the nil records don't get deleted first :-)
It will never be more than ca 30 records, so I didn't care about performance.

Do you want me to fix it anyway? I'll do that tomorrow then! Either way, I will stick to the where's in the future.

pgaspar

@F3PiX no, that’s fine 🙌 thank you for your patience! ❤️

emcoding · 2018-05-31T07:06:58Z

Thanks so much @pgaspar for having my back! 🙇 Much appreciated!

emcoding added 7 commits May 24, 2018 15:16

For better readability: separate loading and authorizing the resource

02b79bd

Let's be strict about abilities

1ff8f1b

We added a rule that all users `can :read User`; we can choose to not authorize those actions, or check the abilities anyway.

Add hint that city is required

5766aff

City field has a presence validation, but that was not visible in the view.

Spring cleaning: remove user records where github_id is nil (related to

b8a047c

#979) For real github accounts, the github_id is not nil. So if the id is nil, it is a fake account or an input error. Updated seeds with github_id

Update guest feature spec (can't see email addresses on Community page)

dde0d75

Add feature spec unconfirmed user signing in

52f84ee

On second thought: let's just load_and_resource

a17ccb8

emcoding mentioned this pull request May 25, 2018

User Registration Overhaul: break down #991

Open

16 tasks

emcoding added 4 commits May 25, 2018 19:11

👮

36e16e7

FIX only confirmed users can read email addresses in users index and …

a874e31

…show views

Muted unfixable spec

51daf41

Remove redundant method from Cancan

5ff500d

Solved in cancancan Fixed in cancancan: ryanb/cancan#835 (comment)

hola-soy-milk approved these changes May 26, 2018

View reviewed changes

pgaspar suggested changes May 27, 2018

View reviewed changes

emcoding added 3 commits May 28, 2018 12:00

Fix abilities to fix the layout in community table

08a09cd

Fix spec

6cf21b6

Update migration

cd86341

pgaspar suggested changes May 28, 2018

View reviewed changes

emcoding added 3 commits May 28, 2018 20:36

Update migration; Never underestimate a 10 seconds win :-)

9c5052a

Make the email column title's visibility behave the same as the email…

7062340

… field's

Remove comment

adcd7db

emcoding added 4 commits May 28, 2018 21:18

Get rid of the annoying column - by @pgaspar 💃

54ae630

Fix spec description

e641098

Update ability specs so they really test the read_email rule

a55b4cd

Declutter

d48ea6b

emcoding force-pushed the spring-cleaning-users branch from e6bd629 to d48ea6b Compare May 29, 2018 09:32

emcoding closed this May 29, 2018

emcoding reopened this May 29, 2018

Handle user records without github_id; skip in test env

62b49c4

emcoding force-pushed the spring-cleaning-users branch from d10b94c to 62b49c4 Compare May 30, 2018 16:51

pgaspar reviewed May 30, 2018

View reviewed changes

pgaspar approved these changes May 30, 2018

View reviewed changes

emcoding merged commit 0d50ff4 into master May 31, 2018

klappradla deleted the spring-cleaning-users branch January 25, 2019 10:36

		@@ -15,6 +15,8 @@

		it_behaves_like 'has access to public features'

		it { expect(subject).not_to be_able_to(:read, other_user.email) }

Spring cleaning users #1006

Spring cleaning users #1006

Conversation

emcoding commented May 25, 2018 • edited Loading

emcoding commented May 26, 2018

hola-soy-milk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgaspar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emcoding commented May 28, 2018

pgaspar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgaspar May 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emcoding commented May 28, 2018 • edited Loading

emcoding commented May 28, 2018

pgaspar commented May 28, 2018

emcoding commented May 29, 2018

pgaspar commented May 29, 2018

Choose a reason for hiding this comment

emcoding commented May 30, 2018

pgaspar left a comment

Choose a reason for hiding this comment

emcoding commented May 31, 2018

emcoding commented May 25, 2018 •

edited

Loading

pgaspar May 28, 2018 •

edited

Loading

emcoding commented May 28, 2018 •

edited

Loading