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

Prevent workspace admins to modify the system user on a workspace #5664

Merged
merged 2 commits into from
Nov 19, 2024

Conversation

norkans7
Copy link

No description provided.

@norkans7 norkans7 force-pushed the system-user-2 branch 2 times, most recently from d111b9a to 8bfb28c Compare November 19, 2024 18:42
Copy link

codecov bot commented Nov 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (131d0c9) to head (723e384).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##              main     #5664    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          565       565            
  Lines        25836     26175   +339     
==========================================
+ Hits         25836     26175   +339     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@@ -72,6 +72,10 @@ def setUp(self):
self.agent = self.create_user("[email protected]", first_name="Agnes")
self.customer_support = self.create_user("[email protected]", is_staff=True)

self.system_user = self.create_user("[email protected]")
self.system_user.settings.is_system = True
self.system_user.settings.save(update_fields=("is_system",))
Copy link
Member

Choose a reason for hiding this comment

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

let's just create this user in the tests that need it.. it's not a common case

.exclude(id=user.id)
.exclude(settings__is_system=True)
.exists()
and not user.settings.is_system
Copy link
Member

Choose a reason for hiding this comment

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

I think replace this line with .filter(settings__is_system=False) on line 567

Copy link
Author

Choose a reason for hiding this comment

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

I am adjust that to actually allow staff to manage that on UI

Copy link
Member

Choose a reason for hiding this comment

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

I think how this will work is that we create the user with an email we control and then invite them to workspaces as necessary. We don't need to update them or remove them (for now)

Copy link
Author

@norkans7 norkans7 Nov 19, 2024

Choose a reason for hiding this comment

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

yes, staff no longer allowed to post so that could not work

super(BaseListView, self)
.derive_queryset(**kwargs)
.filter(id__in=self.request.org.get_users().values_list("id", flat=True))
.order_by(Lower("email"))
.select_related("settings")
)

if not self.request.user.is_staff:
qs = qs.exclude(settings__is_system=True)
Copy link
Member

Choose a reason for hiding this comment

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

need to update admin_count below on line 435

@rowanseymour rowanseymour merged commit bd7c303 into main Nov 19, 2024
5 checks passed
@rowanseymour rowanseymour deleted the system-user-2 branch November 19, 2024 21:30
@github-actions github-actions bot locked and limited conversation to collaborators Nov 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants