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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Our analytics" can be confusing when user does not have access to that collection #20716

Closed
flamber opened this issue Feb 24, 2022 · 7 comments · Fixed by #23887
Closed

"Our analytics" can be confusing when user does not have access to that collection #20716

flamber opened this issue Feb 24, 2022 · 7 comments · Fixed by #23887
Assignees
Milestone

Comments

@flamber
Copy link
Contributor

flamber commented Feb 24, 2022

Describe the bug
"Our analytics" can be confusing when user does not have access to that collection.

To Reproduce

  1. Admin > Permissions > Collections - set "Our analytics" to No access for All Users
  2. Login as user
  3. The homepage should probably not show the Your teams’ most important dashboards go here (this is new in 0.42) and perhaps not show the text Access dashboards, questions, and collections in "Our analytics"
    image
  4. Click "Browse all items" - that will take you to collections hierarchy page, but it should probably hide the "Our analytics" in the sidebar and auto-select the first available collection by default:
    image
    Example with a few collections available to the user:
    image

Information about your Metabase Installation:
Tested 0.41.6 thru 0.42.1

Additional context
Related to #19070 and #8747

@kulyk
Copy link
Member

kulyk commented Apr 22, 2022

This requires both frontend and backend work. For non-root collections, the BE responds with 403 on GET /api/collection/:id. However, it returns 200 for GET /api/collection/root even if a user doesn't have permission to it at all (aka "No access" permission). At the same time, the FE can't distinguish the "View" and "No access" permissions for the root collection. Perhaps we should better do #19070 and close this issue together with it

@nemanjaglumac nemanjaglumac changed the title "Our analytics" can be confusing, then user does not have access to that collection "Our analytics" can be confusing when user does not have access to that collection Jul 11, 2022
@nemanjaglumac nemanjaglumac added the Priority:P2 Average run of the mill bug label Jul 11, 2022
@nemanjaglumac
Copy link
Member

This issue became much worse with the addition of some features from v43.
The existence of "Our analytics" is not only confusing, but it also leads user to some dead-end pages and it prompts one to perform actions which they are not allowed to.

This is how /collection/root looks for a user with no access now:
image

  1. It prompts you to click on events (which is not allowed) and only leads to weird errors and empty modals
  2. It also completely unnecessarily sends requesst to:
    • /api/collection/root/timelines - returns 200 (wrong)
    • api/timeline/1?include=eventswhich (correctly) returns 403
  3. It prompts you to create new:
    • Dashboard (ok)
    • Collection (can result in a weird state error and a blank page in production!)

This is how it looks like in the local dev environment:
image

Clicking on events:
image

@calherries
Copy link
Contributor

While working on this I came across two other smaller bugs with users without root permissions.

When joining a table with a saved question, "Our analytics" appears as an empty collection:
image

When saving a question, "Our analytics" appears on the search header:
image

@flamber do these need separate issues made?

calherries added a commit that referenced this issue Jul 15, 2022
…ave access to that collection (#23887)

* Change collection endpoints to not return root collection if user doesn't have Root permission

* hide our analytics collection in saved question picker when users do not have access

* Fix tests

Co-authored-by: Aleksandr Lesnenko <alxnddr@gmail.com>
@calherries
Copy link
Contributor

calherries commented Jul 15, 2022

I've closed this issue and created a separate issue for the last screenshot in my comment above.

@nemanjaglumac
Copy link
Member

nemanjaglumac commented Jul 15, 2022

Note to @calherries and probably to @luizarakaki or @brunobergher

This issue was planned for the end of 43 cycle. We're already in the next cycle and will most likely not issue another 43.x release.
Should this be moved to the 44 cycle Project? If so, #23887 needs to be backported.

nemanjaglumac added a commit that referenced this issue Jul 15, 2022
- #16555 is not relevant anymore
- add repro for #20716
@nemanjaglumac
Copy link
Member

I've closed this issue and created a separate issue for the last screenshot in my comment above.

There is one more bug that's not solved and it seems like a frontend not handling exceptions.
I warned about it here, but now I filed it as a separate issue - #24015

nemanjaglumac added a commit that referenced this issue Jul 15, 2022
* Fix broken collection tests

* Merge related tests together

- #16555 is not relevant anymore
- add repro for #20716
github-actions bot pushed a commit that referenced this issue Jul 18, 2022
…ave access to that collection (#23887)

* Change collection endpoints to not return root collection if user doesn't have Root permission

* hide our analytics collection in saved question picker when users do not have access

* Fix tests

Co-authored-by: Aleksandr Lesnenko <alxnddr@gmail.com>
@calherries
Copy link
Contributor

@nemanjaglumac Yes, it should have been backported to 44. Here's the backport PR #24048

@flamber flamber added this to the 0.44 milestone Jul 21, 2022
This was referenced May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment