Conversation
|
It'd be better if these were separate PRs, though tbh I don't think allowing admins to edit user emails is the way to resolve e.g. "user typo'd their email address and isn't getting the verification email" to begin with/I'm disinclined to accept that change at all. One idea that came up in the server was just having a separate view where unverified users can check/change it, which would probably be preferable... |
|
I can see the benefit of giving non-admins admin rights; means that some of the sites I'm on can just give me my own role, haha.. But I'm not sure about the email editing either.. |
any particular reason why?
I can raise another CR with this change, would like to hear the reasoning for the above though, I don't think having both is bad necessarily |
|
It's essentially still a security thing that's currently treated as an inconvenience because sometimes it is a genuine mistake... but e.g. I don't particularly enjoy opening the door to someone social engineering their way into getting an admin to change an account's email address. And while, yes, it will always be possible to edit directly via the DB, there's a difference between it being possible and being convenient. |
|
I'm in agreeance with Mercury here and was just about to come in and say the same thing. It's absolutely a vector for social engineering -- I've heard of people doing similar things with Toyhouse or Discord accounts, as well as at my job. I think the suggestion of having a separate view where unverified users can check/change it would be the right call to solving the "I typo'd my email and I'm not getting verification emails" problem. Maybe make it visible on the verify email page that people get redirected to? |
|
Happy to make the change for it to be user-initiated then! Hadn't thought about the social engineering aspect of it, I guess I have too much trust in the thought process of people with admin privileges 😭 |
…lete unused controller, fix routes that redirect to 'home' to point to name
ScuffedNewt
force-pushed
the
feature/email-qol
branch
from
e6af30b to
cf637fb
Compare
SpeedyD
left a comment
SpeedyD
left a comment
There was a problem hiding this comment.
I like this fix- as long as a user has no verified alias, they can fix the email address themselves. Means they have to be logged in already- and that occurs right after registration.
Good call! :)
| // Updating email from verification notice | ||
| Route::middleware('auth')->group(function () { | ||
| Route::get('/email/update', 'HomeController@getEmail'); | ||
|
|
There was a problem hiding this comment.
This is 100% a nitpick- wondering why there's an empty line in between is all :)
|
Oh yeah, does this also fix #1393 ? |
| */ | ||
| public function getIsAdminAttribute() { | ||
| return $this->attributes['is_admin']; | ||
| return $this->attributes['is_admin'] || $this->powers()->where('power', 'admin')->exists(); |
There was a problem hiding this comment.
See, I understand why you've done this. But you've put back the query that's going to be everywhere...
There was a problem hiding this comment.
do you prefer another attribute?
There was a problem hiding this comment.
Unsure what the ideal solution is (brainfog's been bad lately), beyond wanting to avoid a query if possible for performance reasons.
There was a problem hiding this comment.
Might be a bit redundant and roundabout, but why not add a column 'fauxAdmin' with a bool.. that way you only need to pull that attribute..?
Does mean you'd have to slightly rewrite the rank controller, so that it actually gets set or unset depending on whether the power is there..
There was a problem hiding this comment.
I was actually thinking on this morning and I agree that's probably the best approach, even if it's technically data duplication. Alternatively, would always eager loading powers alongside Rank solve the issue? aka setting the $with on the Rank model. To some degree I'm surprised it doesn't do that already
|
Once again, I would like to voice that these should really be separate PRs. |
SpeedyD
left a comment
SpeedyD
left a comment
There was a problem hiding this comment.
I believe I'll swap my approval into an RFC. Merc isn't going to drop the 'split into two PRs' point, and after thinking it through, I agree with it.
I strongly suggest moving the 'user can change own email address if unverified' one to a new PR, since the most recent comments are about the faux admin ability. ;)
5 participants
Uh oh!
There was an error while loading. Please reload this page.