Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guests can see user profile regardless of the permission "view user list" #1680

Open
MichaelBelgium opened this issue Dec 4, 2018 · 3 comments 路 May be fixed by #1725

Comments

@MichaelBelgium
Copy link

@MichaelBelgium MichaelBelgium commented Dec 4, 2018

Bug Report

Related to #1676 and https://discuss.flarum.org/d/17835-the-requested-resource-was-not-found-on-refresh and cuz toby asked me to 馃槢

Current Behavior
Guests can view user profiles regardless of the permission "view user list". It only triggers the permission on refreshing the page of a user or when directly navigating to a user profile url.

Steps to Reproduce

  1. Login and go the the permission page in the admin section
  2. Set the permission "View user list" to "Members"
  3. Log out
  4. Navigate to a discussion
  5. Click on someone's name
  6. You can view their profile.

Optional:
7. Refresh the page or navigate directly to a user profile and see it working nonetheless, but displaying a vague error.

Expected Behavior

  • A more clear error message after step 7. You get a The requested resource was not found and not some kind of Permission denied error
  • Guests can't browse through a discussion to a user profile if the permission is set to members only

Environment

  • Flarum version: beta 8
  • Website URL: local
  • Webserver: nginx
  • Hosting environment: Laravel homestead
  • PHP version: 7.2.9
  • Browser: latest version of chrome
Flarum core 0.1.0-beta.8
PHP version: 7.2.9-1+ubuntu18.04.1+deb.sury.org+1
Loaded extensions: Core, date, libxml, openssl, pcre, zlib, filter, hash, pcntl, Reflection, SPL, sodium, session, standard, mysqlnd, PDO, xml, bcmath, calendar, ctype, curl, dom, mbstring, fileinfo, ftp, gd, gettext, iconv, igbinary, imap, intl, json, ldap, exif, msgpack, mysqli, pdo_mysql, pdo_pgsql, pdo_sqlite, pgsql, Phar, posix, readline, shmop, SimpleXML, soap, sockets, sqlite3, sysvmsg, sysvsem, sysvshm, tokenizer, wddx, xmlreader, xmlwriter, xsl, zip, memcached, blackfire, Zend OPcache, xdebug
+------------------------------+-------------------+------------------------------------------+
| Flarum Extensions            |                   |                                          |
+------------------------------+-------------------+------------------------------------------+
| ID                           | Version           | Commit                                   |
+------------------------------+-------------------+------------------------------------------+
| flarum-emoji                 | v0.1.0-beta.8     |                                          |
| flarum-lang-english          | v0.1.0-beta.8     |                                          |
| flarum-flags                 | v0.1.0-beta.8     |                                          |
| flarum-likes                 | v0.1.0-beta.8     |                                          |
| flarum-lock                  | v0.1.0-beta.8     |                                          |
| flarum-mentions              | v0.1.0-beta.8     |                                          |
| flarum-statistics            | v0.1.0-beta.8     |                                          |
| flarum-sticky                | v0.1.0-beta.8     |                                          |
| flarum-subscriptions         | v0.1.0-beta.8     |                                          |
| flarum-suspend               | v0.1.0-beta.8     |                                          |
| flarum-tags                  | v0.1.0-beta.8.1   |                                          |
| flarum-bbcode                | v0.1.0-beta.8     |                                          |
| michaelbelgium-profile-views | dev-flarum-beta-8 | 3350771156941554978c4692e3eef54b0adbda38 |
+------------------------------+-------------------+------------------------------------------+
Base URL: http://flarum.devel
Installation path: /home/vagrant/flarum
Debug mode: ON
@tobyzerner

This comment has been minimized.

Copy link
Contributor

@tobyzerner tobyzerner commented Dec 4, 2018

This is a regression caused by 40e4c0a. It turns out that the viewDiscussions permission is more like a global viewForum permission to restrict access from your whole forum (including discussions and user profiles), which is why we had been using it in UserPolicy::find. Whereas viewUserList is literally just for viewing the GET /api/users endpoint, not individual user profiles (because you can indeed access them anyway if you can see discussions).

So plan of attack from here:

  • Change viewDiscussions permission to viewForum
  • Revert 40e4c0a
@tobyzerner tobyzerner added this to the 0.1.0-beta.9 milestone Dec 4, 2018
@tobyzerner tobyzerner added the type/bug label Dec 4, 2018
@luceos

This comment has been minimized.

Copy link
Member

@luceos luceos commented Dec 5, 2018

New permissions should be thoroughly scrutinised in the future, in my opinion the viewUserList permission caused too much confusion anyway, I'm glad we can revert this and apply something more transparent 馃憦

@luceos luceos referenced a pull request that will close this issue Jan 9, 2019
0 of 4 tasks complete
@luceos

This comment has been minimized.

Copy link
Member

@luceos luceos commented Jan 9, 2019

@tobscure what are we going to do with the viewUserList permission?

What I've considered is that the permission (even if named incorrectly) was introduced because it was so easy to scrape for users. If we'd tackle the #1356 issue, the permission could probably be modified to suit the needs better?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can鈥檛 perform that action at this time.