Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

Fix cancancan implementation of Supervisor ability #1001

Closed
7 tasks
emcoding opened this issue May 18, 2018 · 4 comments
Closed
7 tasks

Fix cancancan implementation of Supervisor ability #1001

emcoding opened this issue May 18, 2018 · 4 comments

Comments

@emcoding
Copy link
Member

emcoding commented May 18, 2018

Related to PR #997
Updated

I am a supervisor of user1 and not user2
Both students have hide_email = true

Current behaviour (in master):
When logged in as student1's supervisor, I can view user1's email only 👍
However, this is done via view helpers, not with proper abilities.
We shouldn't trust on view helpers to guard privacy sensitive data.

Supervisor-in-current-season can read emails of members of their own teams-in-current-season only.

  • Fix ability and cancancan authorisations in all the relevant places
  • Remove the 'signed_in?' conditional in the supervisors/base_controller
  • Doublecheck if it works for students who where supervised in previous years

(Note to self:)

ability = Ability.new(maud)
student1.hide_email # => true
student2.hide_email  # => true
ability.can?(:read, student1.email) # => true
ability.can?(:read, student2.email) # => false
  • There are two view helpers in Ability addressing this behaviour: :read_email and :users_info. They overlap, so at least one of them seems redundant. After implementation is fixed: probably both obsolete.
  • Remove/revisit the old code in Ability (marked FIXME)
  • Check if users_info covers more than reading hidden email address field, reading blogpost_info field (whatever that may be :-) ), and add those to Ability and specs

bonus:

  • When driving by 🐎 : set default for hide_email in database - there are 3 possible values now
@hola-soy-milk
Copy link
Member

hola-soy-milk commented May 20, 2018

Thanks for documenting this!

Hmm, if I understood correctly, we use the helpers to dynamically show and hide email addresses. Can this be reproduced or moved to the abilities? Can we call abilities from views?

Update:

Yes we can! It's taken care of here

@hola-soy-milk
Copy link
Member

Is this something I can help with, @F3PiX? I'd love to dip my toes into cancancan 😊

@emcoding
Copy link
Member Author

I am not sure we are even able to fix this at this point. I'd recommend waiting until the User Registration Overhaul is done.

@emcoding
Copy link
Member Author

emcoding commented Jun 4, 2018

I am closing this; it's two different issues now: #1012 en #1013
@ramonh #1012 is ready to go, if you like. 🚀

@emcoding emcoding closed this as completed Jun 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants