Open thebrucecgit opened 1 year ago
Hi Bruce, thanks for raising this. Some notes:
Regarding performance, I agree that it's very slow. I think it would be very easy to make it significantly faster (it's a classic N+1 query problem, see #151 for a similar issue). I haven't been doing a good job at getting round to things, but I'm happy to take a quick look at that if we want it fixed sooner rather later/never.
The version of /users shown to non-admins is different from the version shown to admins. Non-admins see columns Username, Roles, Solved Problems, and Brownie Points, while admins also see Name and Email.
The information shown on /users to non-admins is also shown on /user/:id, so non-admins could get this information (with a little work) by trying all id's even if we hide /users. So if we decide to hide /users, we should also think about /user/:id, e.g. hide some fields, and/or replace the incrementing ID with a long random string to prevent enumeration (it's possible to implement without changing the database, using encryption).
I think my personal preference is to hide /users. We can consider showing the top N users instead.
/users/newest shows time of registration. I don't really mind whether we show it or hide it.
/users/online shows the last login time, it is accessible even though there is no link to it in the menu. We should forbid access (or add a link to the menu if there is a good reason for making it visible).
Hi Tom,
Thanks for the quick response. If it's easy to fix the performance issue, then sure that would be great. I'm hoping to redesign the NZIC signup process to have instant user validation, which is probably going to call /users
, so making it faster would be helpful.
Displaying the top N users with the most problems solved would also be neat - creating some sort of a leaderboard system which the Aussies have.
I imagine hiding /users is a fairly straightforward change. Replacing the incrementing ID would be a nice-to-have but would probably need widespread across the codebase?
We've finally started making progress on this (sorry for the delay), so far I've just limited /users/online (#210) and /users/newest (#211) to staff but we intend to do more.
Leaving this here as a note. Feel free to ignore.
I think
/users
,/users/online
,/users/newest
(can still be accessed via direct link) should be limited to admin only.