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

chore: improve the rbac teams migration #28307

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

zlwaterfield
Copy link
Contributor

Changes

Follow along for RBAC: #24512

Running the migration on posthog org brought up a few edge cases

  • we have some inactive users that still have org and team memberships, so skipping those
  • if a user was first an org member and then adding to a private project as a member and then bumped to an admin on the org they no longer need the team membership but still have one so it skips those
  • it also makes sure the user still has an org membership in case it was removed and the team membership was not

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Does this work well for both Cloud and self-hosted?

How did you test this code?

@zlwaterfield zlwaterfield self-assigned this Feb 4, 2025
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Here's a concise summary of the changes in this PR focused on improving RBAC team migration:

This PR enhances the RBAC migration process for teams and feature flags, addressing edge cases discovered during PostHog organization migration.

  • Added filtering in /posthog/rbac/migrations/rbac_team_migration.py to skip inactive users and redundant team memberships for org admins
  • Enhanced error handling with nested try-except blocks and Sentry exception capturing in both migration files
  • Added support for organization-wide view-only access level in feature flag migrations
  • Added additional check in FeatureFlagSerializer.get_can_edit() to verify organization's default access level during migration
  • Improved cleanup by removing leftover explicit team memberships after successful migration

4 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +81 to +82
# Delete any left over explicit team memberships
ExplicitTeamMembership.objects.filter(team_id=team.id).delete()
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: deleting leftover memberships could delete memberships that were skipped intentionally (e.g. org admins)

Suggested change
# Delete any left over explicit team memberships
ExplicitTeamMembership.objects.filter(team_id=team.id).delete()
# Delete migrated explicit team memberships
ExplicitTeamMembership.objects.filter(
team_id=team.id,
parent_membership__level__lt=OrganizationMembership.Level.ADMIN
).delete()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants