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

fix: remove "All" permission for Workspace #19496

Merged
merged 1 commit into from Jan 7, 2023

Conversation

barredterra
Copy link
Collaborator

Before:

  • All can access Workspace list and form view (but not do anything there)

After:

  • Only Workspace Manager can access Workspace list and form view

Everything else should remain unchanged.


"All" users were able to open the workspace list and see everyone else's workspace and, by extension, their email id's. This becomes a problem in big systems where users cannot, for legal reasons, see everybody's email ids.

We had some get_list calls that workspace.py uses internally, which I had to change to get_all. From a security perspective, this changes nothing (before, everyone had permissions, now they get ignored).

Internal reference: LAN-619

@github-actions github-actions bot added the add-test-cases Add test case to validate fix or enhancement label Jan 5, 2023
@barredterra barredterra removed the add-test-cases Add test case to validate fix or enhancement label Jan 5, 2023
@codecov
Copy link

codecov bot commented Jan 5, 2023

Codecov Report

Merging #19496 (033c6b3) into develop (efeed5c) will increase coverage by 0.08%.
The diff coverage is 83.33%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #19496      +/-   ##
===========================================
+ Coverage    65.41%   65.49%   +0.08%     
===========================================
  Files          755      755              
  Lines        74475    73895     -580     
  Branches      6126     6133       +7     
===========================================
- Hits         48715    48398     -317     
+ Misses       22266    21921     -345     
- Partials      3494     3576      +82     
Flag Coverage Δ
server 68.59% <0.00%> (-0.02%) ⬇️
server-ui 31.77% <100.00%> (+0.04%) ⬆️
ui-tests 51.12% <100.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Member

@shariquerik shariquerik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work, since the user doesn't have access to Workspace he/she cannot use get_all or get_list

@barredterra
Copy link
Collaborator Author

barredterra commented Jan 7, 2023

Did you test it? It does work 😅

get_list, which was previously used, will check permissions, hence wouldn't work. But get_all ignores permissions, hence works.

@shariquerik
Copy link
Member

shariquerik commented Jan 7, 2023

Did you test it? It does work 😅

get_list, which was previously used, will check permissions, hence wouldn't work. But get_all ignores permissions, hence works.

My bad, I works 😅
LGTM

@shariquerik shariquerik merged commit 0a8591f into frappe:develop Jan 7, 2023
@shariquerik shariquerik added the backport version-14-hotfix backport to version 14 label Jan 7, 2023
@barredterra barredterra deleted the workspace-perms branch January 7, 2023 13:25
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport version-14-hotfix backport to version 14
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants