rgsoc-teams
rgsoc-teams copied to clipboard
rails-girls-summer-of-code
Revisit read_email and user_info ability rule
Supervisor-in-current-season can read emails of members of their own teams only. However, guarding the access to hidden emails is done via cancancan view helpers.
QUESTION: I am not sure if we should trust on view helpers to guard privacy sensitive data.
One way to let cancancan manage this is to add a link to an email address instead of displaying the email address directly. For instance by adding a ✉️ icon, that links to sending an email . We could still use the view helpers for whether or not to display the icon, but unauthorized access would throw an AccessDenied error. I like that better for guarding privacy issues. I think view helpers are more suitable for UX things.
Todo 1)
- [ ] make changes accordingly
- [ ] set default: false for hide_email in database - there are 3 possible values now
Apart from the :read_email rule there is also a :users_info rule. IIRC they overlap. Let's check if we can get rid of one.
- [ ] 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
Hello 👋 I'm not sure I understood the reason for not trusting view helpers. Are we worried someone will mindlessly remove them from the code in the future and so expose emails?
Also, I'm not sure I understand the ✉️ icon suggestion. Where would it link to? Would it be a mailto? (If so, wouldn't that be virtually the same as showing the email?).
Also, I'm not sure I understand the ✉️ icon suggestion. Where would it link to? Would it be a mailto? (If so, wouldn't that be virtually the same as showing the email?).
It can be a mail-to, but clicking the link would be authorized, no matter of the link is visible or not.
About the view_helpers: it's more about a separation of concerns: I like it view_helpers to manage the user interface, for example hiding links someone has no access to. And I don't like view_helpers to guard that access. Is that too strict?
👍 fully agree that the access to this should not only be managed via conditionals in views 😉
What I usually use in pundit is "scopes" - so depending on the authorization policy one would not even query for the things a given user is not allowed to access. Just as an example.
Interesting 👍 Can we do something similar with CanCanCan?
I like the idea of making the check more robust in the code, but I think changing the interface so that you can only access an email through a separate request to an authorized controller or something like that may be overkill and leads to a worse experience for the user.
Something like grabbing 3 or more emails from the community list becomes a much harder task than if we just showed them in the list, for example.
Thanks for your inputs!
Cancancan works the same for queries on models, so user = User.all in #index will only return the users you are authorised to view. I am curious how we can use this 'scope' like feature to only show authorised attributes as well in Cancancan. That can be investigated in this issue.
Re: changing the interface. That sounds more complex than I meant it to be. We can use the same link_to in the view, but currently we already reveal the email address in the link name. The solution I mentioned earlier would be something like : link_to "mail this user" instead of link_to "[email protected]". Link is the same. No extra controllers/logic.
A different solution could be to use the user name to (authorized-)link to an email address, and use a separate column for 'view profile'. That is basically reversing the behaviour of the first two columns in the community table. Downside: I'd say people want to view the profile more often than send an email. Would you like this solution better, Pedro?
Re: grabbing emails from the community list. Pedro, you mean when applicants want to email a bunch of coaches? Interesting feature, why don't we have that? We do have some places for sending emails to multiple users, like you can send an email to all your own team members, and orga's can send emails to filtered groups of users.
TL;DR Prelim conclusion:
- Try to find different solution than view helpers only to guard the user email address visibility
- Maybe Cancancan offers a pundit like scope on attributes as well; if so, use it
- Inconclusive on if/how to show the email address in the view. Adding an extra click to reveal the email address or not?
Cancancan works the same for queries on models, so user = User.all in #index will only return the users you are authorised to view. I am curious how we can use this 'scope' like feature to only show authorised attributes as well in Cancancan. That can be investigated in this issue.
Maybe @klappradla can give us an example of how it works with Pundit (like how the view code looks like)?
We can use the same link_to in the view, but currently we already reveal the email address in the link name. The solution I mentioned earlier would be something like : link_to "mail this user" instead of link_to "[email protected]". Link is the same. No extra controllers/logic.
Ok, I understand your suggestion better now, thank you. But does this really protect the info? We would still have the CanCanCan view helpers managing access through conditionals, no? The only difference I think this would make is the emails not being as easy to see and copy to the user. Personally, as a user, I would rather see the email upfront than needing to get it from a mailto link.
A different solution could be to use the user name to (authorized-)link to an email address, and use a separate column for 'view profile'. That is basically reversing the behaviour of the first two columns in the community table. Downside: I'd say people want to view the profile more often than send an email. Would you like this solution better, Pedro?
I think it's a pretty established pattern throughout the web that the user's name links to their profile. Changing that would likely lead to confusion and frustration 😟
Re: grabbing emails from the community list. Pedro, you mean when applicants want to email a bunch of coaches? Interesting feature, why don't we have that?
Coaches, other students, whoever really. And it's likely that some organizers and coaches also grab some emails from this list occasionally. I'm not saying we should support emailing directly from the app - I just think we shouldn't make it hard to read and copy emails from the community list. If we hide the emails under links I'm afraid that task would become unnecessarily hard and frustrating.
For example, to email the first 3 people in this list at the same time I just have to copy their emails and use them in my email program/website. If we hide the emails from the list behind a mailto link that doesn't show the email I would have to click the mailto link, which would open a window or tab a few seconds later. I would then be able to copy the email from the "to:" field. And do that 3 times.
Any malicious user or bot would still be able to get any emails we may be showing incorrectly with a very simple JS script, while regular users would have a harder time using the system for tasks like emailing a few coaches. I just think that we'll always have that risk - we can't control what extensions a user has, for example. They can be mining email addresses without even realizing.
Where I think we should focus is on making sure it's not easy to mistakenly include an email the user has no read access to in the rendered HTML. But even this idea can be taken to the extreme (for example disabling the User#email method and forcing everyone to use something like User#email_visible_to(user) everywhere). I'm curious to see what the Pundit scope solution looks like. From what I saw in the docs I think instead of calling user.email you go through the scope and it either shows you the email or not, depending on your permissions. It's still not failproof, I'd say, since just as you can forget to add the CanCanCan can? check, you could also forget not to use the standard user.email.
Let me know your thoughts @klappradla @F3PiX! I may be missing some context or something! I haven't worked with CanCanCan or Pundit in recent projects so I can be failing to see something obvious 😅
Thanx for you explanation @pgaspar 🙏
I'm not sure if I really understood the problem before... 🙈
As for showing the emails: I'm 100% with you, it should be easy to copy / use the email address without additional views, etc. 👍
For pundit's scopes: they're just part of a policy and whenever querying for entities in e.g. a controller, one can go via these scopes to "scope" according to a user's access rights. The granularity level this is usually used for are DB-rows. Such as: an admin can see every user's post, an ordinary user only their own. I have never used it on a column level... But it would of course also work: one would just write the select statement by hand. Using these scopes doesn't solve eventual problems in views. One still has to check by oneself if data is available or not and the usual things.
For the visibility of email addresses in the views what you described above, all your solutions sound decent. Using can? is totally ok, using a decorator and implementing email there is totally fine (I use draper a lot) and having a separate email_visible_to method as well.
I'd personally probably go for using a decorator or just can, rather shoving more non-business related logic is the already majestic user model 😉
Thanks for your thoughts! Much appreciated. Let me summarise:
- The access to the email address should not only be managed via
can?conditionals in views - We also don't want to make copying email addresses from the Community list any harder than what is currently needed: 1 click and 1 copy-paste.
And we already have a couple of suggestions for the implementation.
If this summary is reflecting your thoughts, @pgaspar and @klappradla, I'd say we have a clear scope for this issue? We can ask whoever is going to work on this to come up with a solution that fits these requirements. Right?