fix(connections): admin-gate app profile updates + member account-profile rebind UI#812
Conversation
…rebind handleUpdate previously had no role check on auth_profile_slug or app_auth_profile_slug — non-admins could pivot a connection onto another user's OAuth app or another member's account profile by passing the slug directly. Mirror handleCreate's existing gates: - app_auth_profile_slug: non-admins must pass the pinned default; clearing is admin-only. - auth_profile_slug: caller must be the connection's created_by (or admin/owner); target profile must be owned by the caller for oauth_account / browser_session, and env profiles are admin-only. UI surfaces the new account-profile picker on the connection settings tab (web submodule bump) so members can rebind without dropping the connection.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds role-gated ownership and profile validation to connection updates. The handler now fetches ChangesConnection Update Authorization
Owletto Submodule Update
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba83ee0ccd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (hasAuthProfileArg && !callerIsAdmin) { | ||
| if (!ctx.userId || existing.created_by !== ctx.userId) { | ||
| return { | ||
| error: 'You can only change the auth profile of a connection you created.', |
There was a problem hiding this comment.
This new non-admin ownership gate is unreachable for the member rebind flow because manage_connections.update is still classified as owner/admin-only in packages/server/src/auth/tool-access.ts lines 44-49, and routeAction() enforces that before handleUpdate() runs. As a result, a member who created a connection and selects one of their own profiles will still get the generic admin-access error instead of this intended path; move update into the member-write policy or add an action/field-specific access exception before relying on these handler checks.
Useful? React with 👍 / 👎.
…o member-write) The server-side gates added in the previous commit were unreachable — `manage_connections action='update'` lived in OWNER_ADMIN_ACTIONS so routeAction rejected members before the handler ran. Move 'update' into MEMBER_WRITE_ACTIONS and add a top-level 'caller is admin OR created_by === ctx.userId' gate so the per-field role checks (app profile pinned-default, target-profile ownership) actually fire for members editing their own connection. Members still can't touch other members' connections. Caught by pi PR review of #812.
|
Pushed fixup commit Critical fix from pi review: Other pi findings (not addressed in this PR):
All addressed in conversation; flagging here for follow-up tickets. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/server/src/tools/admin/manage_connections.ts`:
- Around line 1668-1687: The current guard for app_auth_profile_slug only blocks
non-admins from clearing or picking an unpinned profile but does not prevent a
non-admin from rebinding another member's connection to the pinned default;
inside the existing if (hasAppAuthProfileArg && !callerIsAdmin) block add a
check that the caller is the connection owner (compare the request caller's id
to existing.created_by) and return an error if they are not (so only the owner
or an admin can change app_auth_profile_slug), then proceed to call
getAuthProfileBySlug and the pinned checks as before.
In `@packages/web`:
- Line 1: The submodule pointer for packages/web is set to an unreachable SHA
(a63fba52d10ca72ee6751e409ce59980433eddbf) causing Submodule Drift failures; fix
it by repinning the packages/web submodule to a commit that exists on the
tracked branch (owletto/main) or merge the referenced submodule commit into that
branch, then update the parent repo's submodule pointer to that reachable commit
and commit the updated gitlink. Locate the packages/web submodule entry in the
repo's git modules/submodule config and update the gitlink (or perform the merge
into owletto/main) so CI and FluxCD can checkout the submodule successfully.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1bdd6e36-d5f7-487e-8506-d349c705d00b
📒 Files selected for processing (2)
packages/server/src/tools/admin/manage_connections.tspackages/web
| @@ -1 +1 @@ | |||
| Subproject commit b01682708902e51fc33b3f68edbc0f4319836509 | |||
| Subproject commit a63fba52d10ca72ee6751e409ce59980433eddbf | |||
There was a problem hiding this comment.
Pin packages/web to a reachable commit.
Line 1 points the submodule at a63fba52d10ca72ee6751e409ce59980433eddbf, but the Submodule Drift job already proves that SHA is not reachable from owletto/main. That leaves CI red and can break FluxCD checkout/deploys. Please either repin to a commit on the tracked branch or merge this submodule commit there before updating the parent pointer.
🧰 Tools
🪛 GitHub Actions: Submodule Drift / 0_check-drift.txt
[error] 1-1: Pinned SHA $PINNED is not reachable from owletto/main. FluxCD deployment may break because the parent pinned commit is not on origin/main.
🪛 GitHub Actions: Submodule Drift / check-drift
[error] 1-1: Pinned SHA is not reachable from owletto/main. Step failed on check: 'if ! git -C packages/web merge-base --is-ancestor "$PINNED" origin/main; then echo "::error::Pinned SHA $PINNED is not reachable from owletto/main." fi'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/web` at line 1, The submodule pointer for packages/web is set to an
unreachable SHA (a63fba52d10ca72ee6751e409ce59980433eddbf) causing Submodule
Drift failures; fix it by repinning the packages/web submodule to a commit that
exists on the tracked branch (owletto/main) or merge the referenced submodule
commit into that branch, then update the parent repo's submodule pointer to that
reachable commit and commit the updated gitlink. Locate the packages/web
submodule entry in the repo's git modules/submodule config and update the
gitlink (or perform the merge into owletto/main) so CI and FluxCD can checkout
the submodule successfully.
Submodule path moved from packages/web to packages/owletto in #817. Updated the pointer to 8ede17c (lobu-ai/owletto#149 merged into main), which preserves the account-profile rebind UI from this PR.
2 participants
Summary
manage_connections.handleUpdatehad no role check onauth_profile_slug/app_auth_profile_slug. A non-admin who knew any oauth_app slug for the org could PATCH a connection to use it, and the connection owner could pivot the connection onto another member'soauth_accountprofile. Mirrors the gates that already exist inhandleCreate:app_auth_profile_slug: non-admin must pass the connector's pinned default; clearing is admin-only.auth_profile_slug: caller must be the connection'screated_by(or admin/owner), and the target profile must be owned by the caller foroauth_account/browser_session.envprofiles stay admin-only.AccountProfileSectionon the connection settings tab lets the connection owner (and admins) switch which credential a connection runs under without dropping and re-creating it. Non-admins see only profiles they own — matches the new server target-profile check so we never show a picker the server would reject.is_default_for_connector) UX is unchanged: the/oauth-appsadmin page is still the only place to pin it, and the connection settings tab shows the "Org default" badge read-only.What this addresses
The "doubling" / asymmetric-permissions audit on the connection page surfaced two real gaps:
Plan iterated with
pi --provider googlereview, which caught that an initial "owner of connection" check wasn't enough — also needed target-profile ownership. That's now in.Test plan
make build-packagescleanmake typecheckcleanoauth_accountconnection on the settings tab; status inherits the new profile's status.created_by): same rebind works; picker only shows profiles I own.created_by): no picker rendered.manage_connections action=updatewith another user'sauth_profile_slugas a member → 4xx with the new error.manage_connections action=updatewith a non-defaultapp_auth_profile_slugas a member → 4xx.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes