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

remove auto_owner_org_by_app_id #937

Open
riderx opened this issue Dec 21, 2024 · 5 comments · May be fixed by #950
Open

remove auto_owner_org_by_app_id #937

riderx opened this issue Dec 21, 2024 · 5 comments · May be fixed by #950

Comments

@riderx
Copy link
Contributor

riderx commented Dec 21, 2024

Describe the bug/issue
This function is against the philosophy of Capgo, RLS should never modify row unless its field always set by db like updated_at.
This should be removed in favor of always providing it explicitly

@riderx
Copy link
Contributor Author

riderx commented Dec 21, 2024

guard_r2_path is same it shouldn't run in trigger it's a RLS

@riderx
Copy link
Contributor Author

riderx commented Dec 21, 2024

Other triggers who should probably be removed.

prevent_steal_org, noupdate, generate_org_on_user_create, force_valid_user_id_apps, check_if_org_can_exist_org_users, force_valid_owner_org_app_versions, force_valid_owner_org_channel_devices, force_valid_owner_org_channels, force_valid_apikey_name, sync_min_update_version, sync_disable_auto_update, check_privilages

they are run anyway and don't even let admin bypass them so it's a bad design

@WcaleNieWolny
Copy link
Contributor

guard_r2_path is same it shouldn't run in trigger it's a RLS

guard_r2_path is disabled in prod. It fucks up local development (particularly partial upload). I always disable guard_r2_path when I am doing a partial upload locally.

@WcaleNieWolny
Copy link
Contributor

prevent_steal_org - this is a legacy trigger dating back to orgs v1. I agree, we can remove it.
noupdate - It is a security fix. It's something about letting upload assign a version to a channel, but not edit the channel settings, I think.
generate_org_on_user_create - This is very important. Without this code, the registration flow will fail.
force_valid_user_id_apps- Legacy orgs v1 trigger, can be removed
check_if_org_can_exist - important, because you don't want to have an organization that doesn't have at least one super_admin

force_valid_owner_org_app_versions, force_valid_owner_org_channel_devices, force_valid_owner_org_channels - It's important because all of these triggers callauto_owner_org_by_app_id. This prevents a "steal". Imagine that you are a rouge admin user in an organziation. Without these triggers, you could MOVE the channel to your own organization. This would remove access to it from your supervisor (the super_admin users) and make it so that the only way to recover the channel would be via the capgo support.

force_valid_apikey_name - why not? It prevents an API key with a null name or a name equal to an empty string. It's required because the name column is marked as not null.

check_privilages is important as it fixes this advisory

sync_min_update_version, sync_disable_auto_update - likely some legacy code, but the cleanup of this is quite recent

@WcaleNieWolny
Copy link
Contributor

As for auto_owner_org_by_app_id⁣ — it's just a LOT simpler than having to set owner_org manually everywhere.

@WcaleNieWolny WcaleNieWolny moved this to In Progress in Roadmap Jan 1, 2025
@WcaleNieWolny WcaleNieWolny linked a pull request Jan 1, 2025 that will close this issue
@WcaleNieWolny WcaleNieWolny moved this from In Progress to Alpha in Roadmap Jan 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Alpha
Development

Successfully merging a pull request may close this issue.

2 participants