Temp Fix to restrict access for unconfirmed users #992

emcoding · 2018-05-13T15:08:41Z

Related issue #991 (Break Down step 2)

We need to restrict access for unconfirmed users but let them update their email address nevertheless.

Changed Ability class to catch unconfirmed users.
Added a 'confirmed user' context for controller tests
Updated the controller tests that failed; left others alone
Extra: added first feature test, for guest user.
Note: I would appreciate help with the feature tests. This is a first attempt.

Follow up issues:

check if we need the can :read, :feed_entry, or else remove its can? check in views. (Public activities shouldn't be restricted at all.)
check controller tests and use either 'confirmed user' or 'unconfirmed user' contexts
Fix pending specs and todo's in ability_spec

- Update specs, update shared contexts, update controller texts and leave some comments for things that should be addressed later, if still relevant

emcoding · 2018-05-13T15:14:26Z

spec/models/ability_spec.rb

@@ -69,6 +71,7 @@
              allow(user).to receive(:confirmed?).and_return(false)
            end
            it 'allows to see not hidden email address' do
+              pending "Fails. Unconfirmed user has no access"


??? This spec is failing, but I am not sure if we even want it to pass for this conditions?

Hmm. You're right. All an unconfirmed user should, in theory, be able to do is edit their email address.

How about we keep it pending for now? I expect that it will solve itself in the upcoming Overhaul steps.

Yes, I agree. Is there a way to mark this as a TODO? Or is the pending enough, do you reckon?

I am currently collecting all the todo's, and will add them when we have finished the discussion in #991 . Don't want to add too much detail to that.

Fair point!

emcoding · 2018-05-13T15:21:22Z

CC fails on complexity of the body of Ability#initialize. AFAIK this is quite inevitable. Can I ignore it?

emcoding · 2018-05-13T15:26:37Z

@pgaspar @klappradla @ramonh Can you take a look please?

hola-soy-milk · 2018-05-13T16:21:50Z

app/models/ability.rb

-    can :crud, User, id: user.id
-    can :crud, User if user.admin?
+    can :read, User
+    can :update, User, id: user.id


Not sure I understood correctly, but admins seem to be losing the ability to update users here. Is that correct?

I think the failing spec comes from here. Is there a reason we're removing admin access from crud'ing users?

Admin can still crud User, it has moved to line 38.

My apologies! Dunno how I missed that...

But the admin call is guarded behind a if user.confirmed?. I feel we should keep the current behaviour. If anything, we want admins to be able to fix a user's typo'd email address as a response to a support request ticket.

if user.confirmed? checks here if the admin user is confirmed. Which they should be, right?

I added very verbose expectations to check if an admin user can crud everyone.

And I tested manually if an admin user can update someone else's account. They can, and the new specs cover that now...

hola-soy-milk

Thanks @F3PiX, looks good!

Just a question on my end, and otherwise good to go, I'd say.

hola-soy-milk · 2018-05-13T16:32:32Z

app/models/ability.rb

-    can :crud, User, id: user.id
-    can :crud, User if user.admin?
+    can :read, User
+    can :update, User, id: user.id


I think the failing spec comes from here. Is there a reason we're removing admin access from crud'ing users?

pgaspar · 2018-05-13T20:03:34Z

app/models/ability.rb

-  def on_team_for_season?(user, season)
-    season && user.roles.student.joins(:team).pluck(:season_id).include?(season.id)
-  end
+      can :crud, :comments if user.admin?


This is not related to this PR specifically, and we should probably open a separate issue for this: it seems we're not testing this ability in the project comments (or anywhere I could find).

We used to have this in the teams show page, but it was removed meanwhile:

rgsoc-teams/app/views/teams/show.html.slim

Lines 17 to 22 in 4fc8dfd

- if can? :crud, :comments

.comments

a href='#' Hide comments

= render partial: 'comments/form', object: @team, as: :team

ul

= render @team.comments, as: :comment

I've not used CanCanCan a lot in the past.. do we usually use the controller helpers or just view helpers?

Should I create the issue or do we want to fix it in this PR?

We use both helpers ( PR #993 is all about the controller helpers :-) )
If it's okay with you, I'd prefer to keep the PR's as clean as possible. And collect issues for every step layed out in the Overhaul: Breakdown (#991) . So we can use that as a reference all the way.
The crud comments issue will be part of the CommentsController Overhaul (listed in nr 5 in 991). Or afterwards, in the restructuring of the Ability. I'll add it. Thanks!!

Of course! Thank you for your patience - I'm still figuring out how we structure things! 😊

pgaspar · 2018-05-13T20:08:59Z

I like these changes 👍 I didn't do a lot of testing, but I think the changes make sense so far!

I'm not sure how thoroughly we use these abilities though, there seem to be some that are not checked anywhere - which means unconfirmed users can do some things (I didn't test much other than the comments, but that's one that users can use even before confirmation).

Not sure if we should make an effort to see if thee abilities are used or not (in a separate issue, I assume) or if it's too much trouble.

emcoding · 2018-05-14T10:17:13Z

I don't have authority in CodeClimate to set the warning to 'wontfix'. Can someone disable the warning please? (@ramonh ?)

hola-soy-milk · 2018-05-14T10:18:33Z

Oh! Sure! I thought it might've been a comment. I'll take a look, as this PR otherwise looks solid to me. Thanks @F3PiX

carpodaster · 2018-05-14T22:30:52Z

@F3PiX I marked the CC issue as wontfix

carpodaster

Hey @F3PiX – actually, I think CC has a point about the complexity 🙃 : I noticed that the helper methods are all inlined into the constructor … which is valid ruby, but not very readable and not needed, either.

This is probably also the reason for the diff being so noisy and making me lose track of which line was moved where. Can you move the helper functions back to the bottom of the file please?

carpodaster · 2018-05-14T22:49:57Z

app/models/ability.rb

-    can :crud, User, id: user.id
-    can :crud, User if user.admin?
+    can :read, User
+    can :update, User, id: user.id


But the admin call is guarded behind a if user.confirmed?. I feel we should keep the current behaviour. If anything, we want admins to be able to fix a user's typo'd email address as a response to a support request ticket.

In the user factory a user has a confirmed_at date, so we treat users as `confirmed` by default. Adding the trait makes it explicit if we are testing an unconfirmed user.

With extra checks to make sure we are testing what we intend to test.

Specs are flickering because of a vailidation error on the Factory.

emcoding · 2018-05-15T09:45:30Z

@carpodaster THANKS! and sorry for the mess.

Moved the helper_methods back home. 🙈
the if user.confirmed? checks if the admin user herself is confirmed. Which I expected always to be true...?
can :crud, User if user.admin? should check if an admin can :crud each and every @user. Because we are not passing in a restriction with the user ids.
(As opposed to a user who can only crud their own record:
can :crud, User, id: user.id)
I added very verbose expectations in 26cc087, to double check if we are testing what we want to test, and the behaviour we want. I think the user.admin crudding users is covered now?
All this reveals two bigger problems: I didn't update the specs properly, my mistake. And the specs are confusing.
My proposal: close this PR for now, first make sure that we can trust the abilities_specs before changing the behaviour.
WDYT?

emcoding · 2018-05-21T09:38:41Z

This is covered now by #997
Admin specs are covered too:

rgsoc-teams/spec/models/ability/admin_spec.rb

Lines 14 to 24 in 9d685da

    
           describe "Admin" do 
        
             before { allow(admin).to receive(:admin?).and_return true } 
        
             it { expect(subject).not_to be_able_to(:create, User.new) } # happens only via GitHub 
        
             # it "has access to almost everything else" 
        
             # Only test the most exclusive, the most sensitive and the 'cannots': 
        
             it { expect(subject).to be_able_to(:crud, Team) } 
        
             it { expect(subject).to be_able_to([:read, :update, :destroy], User) } 
        
             it { expect(subject).to be_able_to(:read_email, other_user) } 
        
             it { expect(subject).to be_able_to(:read, :users_info, other_user) } 
        
           end

Closing this.

Update Ability to restrict access for unconfirmed users.

45fe849

- Update specs, update shared contexts, update controller texts and leave some comments for things that should be addressed later, if still relevant

emcoding commented May 13, 2018

View reviewed changes

emcoding force-pushed the fixUnconf branch from f9b1b88 to d42fccd Compare May 13, 2018 15:22

hola-soy-milk mentioned this pull request May 13, 2018

Remove authentication logic from abilities #990

Open

3 tasks

emcoding mentioned this pull request May 13, 2018

Allow infinite confirmation period #989

Merged

Remove redundant comments; cosmetic changes

3772ed0

emcoding force-pushed the fixUnconf branch from d42fccd to 3772ed0 Compare May 13, 2018 15:31

emcoding mentioned this pull request May 13, 2018

User Registration Overhaul: break down #991

Open

16 tasks

hola-soy-milk reviewed May 13, 2018

View reviewed changes

emcoding mentioned this pull request May 13, 2018

Fix setting the @user in UsersController #993

Closed

1 task

emcoding added this to the User Registration Overhaul milestone May 13, 2018

pgaspar reviewed May 13, 2018

View reviewed changes

carpodaster suggested changes May 14, 2018

View reviewed changes

emcoding added 4 commits May 15, 2018 09:57

Move helper methods back, which is outside of initializer

ddb193a

Add :unconfirmed trait to user factory

ab6c75c

In the user factory a user has a confirmed_at date, so we treat users as `confirmed` by default. Adding the trait makes it explicit if we are testing an unconfirmed user.

Added extra expectations to test that admin.user can CRUD every user

26cc087

With extra checks to make sure we are testing what we intend to test.

FIX: Make sure user factory creates unique GitHub handles

2564d86

Specs are flickering because of a vailidation error on the Factory.

emcoding closed this May 21, 2018

klappradla deleted the fixUnconf branch January 25, 2019 10:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Temp Fix to restrict access for unconfirmed users #992

Temp Fix to restrict access for unconfirmed users #992

emcoding commented May 13, 2018 •

edited

Loading

emcoding May 13, 2018

hola-soy-milk May 13, 2018

emcoding May 14, 2018

hola-soy-milk May 14, 2018

emcoding May 14, 2018 •

edited

Loading

hola-soy-milk May 14, 2018

emcoding commented May 13, 2018

emcoding commented May 13, 2018

hola-soy-milk May 13, 2018

hola-soy-milk May 13, 2018

emcoding May 13, 2018

hola-soy-milk May 14, 2018

carpodaster May 14, 2018

emcoding May 15, 2018

hola-soy-milk left a comment

hola-soy-milk May 13, 2018

pgaspar May 13, 2018

emcoding May 13, 2018

pgaspar May 13, 2018

pgaspar commented May 13, 2018

emcoding commented May 14, 2018

hola-soy-milk commented May 14, 2018

carpodaster commented May 14, 2018

carpodaster left a comment •

edited

Loading

carpodaster May 14, 2018

emcoding commented May 15, 2018 •

edited

Loading

emcoding commented May 21, 2018

	- if can? :crud, :comments
	.comments
	a href='#' Hide comments
	= render partial: 'comments/form', object: @team, as: :team
	ul
	= render @team.comments, as: :comment

Temp Fix to restrict access for unconfirmed users #992

Temp Fix to restrict access for unconfirmed users #992

Conversation

emcoding commented May 13, 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

emcoding May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emcoding commented May 13, 2018

emcoding commented May 13, 2018

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

hola-soy-milk 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

pgaspar commented May 13, 2018

emcoding commented May 14, 2018

hola-soy-milk commented May 14, 2018

carpodaster commented May 14, 2018

carpodaster left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emcoding commented May 15, 2018 • edited Loading

emcoding commented May 21, 2018

emcoding commented May 13, 2018 •

edited

Loading

emcoding May 14, 2018 •

edited

Loading

carpodaster left a comment •

edited

Loading

emcoding commented May 15, 2018 •

edited

Loading