fix: Check org membership to determine external PR#46
Conversation
WalkthroughAdds an abstract organization-membership method to the VCS Adapter, implements it in the GitHub adapter, updates pull_request handling to compute the "external" flag differently for organization-owned repositories, and adds a unit test for membership checks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Webhook as GitHub Webhook
participant Adapter as GitHub Adapter
participant API as GitHub API
rect rgba(235,245,255,0.6)
Webhook->>Adapter: pull_request payload
Adapter->>Adapter: isOrgRepository(repo.owner)
alt owner is Organization
Adapter->>API: isUserMemberOfOrganization(username, org)
API-->>Adapter: isMember (bool)
Adapter->>Adapter: external = !isMember
else owner is User
Adapter->>Adapter: external = pr.head.user.login != repo.owner.login
end
Adapter-->>Webhook: proceed handling PR with computed external
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
||
| public function isUserMemberOfOrganization(string $username, string $organization): bool | ||
| { | ||
| $url = "/orgs/{$organization}/memberships/{$username}"; |
There was a problem hiding this comment.
This API needs "Org: read member" permission for the GitHub app, so that's something we need to change for Appwrite's GH app before deploying this.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/VCS/Adapter/Git/GitHub.php (1)
696-701: Tighten membership evaluation to “active” state and handle non-200 explicitly.Relying on any 2xx can misclassify pending/invited states. Use 200 + state=active.
- public function isUserMemberOfOrganization(string $username, string $organization): bool - { - $url = "/orgs/{$organization}/memberships/{$username}"; - $response = $this->call(self::METHOD_GET, $url, ['Authorization' => "Bearer $this->accessToken"]); - return $response['headers']['status-code'] >= 200 && $response['headers']['status-code'] < 300; - } + public function isUserMemberOfOrganization(string $username, string $organization): bool + { + $url = "/orgs/{$organization}/memberships/{$username}"; + $response = $this->call(self::METHOD_GET, $url, ['Authorization' => "Bearer $this->accessToken"]); + $status = (int)($response['headers']['status-code'] ?? 0); + if ($status !== 200) { + return false; + } + return strtolower($response['body']['state'] ?? '') === 'active'; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/VCS/Adapter.php(1 hunks)src/VCS/Adapter/Git/GitHub.php(2 hunks)tests/VCS/Adapter/GitHubTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/VCS/Adapter.php (1)
src/VCS/Adapter/Git/GitHub.php (1)
isUserMemberOfOrganization(696-701)
tests/VCS/Adapter/GitHubTest.php (2)
src/VCS/Adapter.php (1)
isUserMemberOfOrganization(262-262)src/VCS/Adapter/Git/GitHub.php (1)
isUserMemberOfOrganization(696-701)
src/VCS/Adapter/Git/GitHub.php (1)
src/VCS/Adapter.php (2)
isUserMemberOfOrganization(262-262)call(278-387)
🪛 GitHub Actions: Tests
tests/VCS/Adapter/GitHubTest.php
[error] 347-347: Utopia\Tests\VCS\Adapter\GitHubTest::testIsUserMemberOfOrganization: Failed asserting that false is true.
🔇 Additional comments (1)
src/VCS/Adapter/Git/GitHub.php (1)
696-701: Ensure GitHub App has “Organization members: read” permission.This endpoint requires org membership read for the app installation token; otherwise, it returns 403/404 and you’ll treat all as external.
| /** | ||
| * Check if user is a member of an organization | ||
| * | ||
| * @param string $username Username of the user | ||
| * @param string $organization Name of the organization | ||
| * @return bool True if user is a member of the organization, false otherwise | ||
| */ | ||
| abstract public function isUserMemberOfOrganization(string $username, string $organization): bool; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
New abstract contract looks good; verify all concrete adapters implement it.
Ensure no other non-abstract adapters are missing this method.
Run to confirm coverage:
🏁 Script executed:
#!/bin/bash
# Find concrete adapters that might need to implement the new abstract method
rg -nP --type=php -C2 'class\s+\w+\s+extends\s+Utopia\\VCS\\Adapter\\\w+' || true
rg -nP --type=php -C2 '\bfunction\s+isUserMemberOfOrganization\s*\('Length of output: 669
🏁 Script executed:
#!/bin/bash
# Check each concrete adapter for implementation of isUserMemberOfOrganization
find src/VCS/Adapter -type f -name '*.php' ! -name 'Adapter.php' | while read -r file; do
if grep -qP 'function\s+isUserMemberOfOrganization\s*\(' "$file"; then
echo "$file: implements"
else
echo "$file: MISSING isUserMemberOfOrganization"
fi
doneLength of output: 102
Implement isUserMemberOfOrganization in Git adapter
src/VCS/Adapter/Git.php is missing the new abstract method isUserMemberOfOrganization(…); add it to satisfy the Adapter contract.
🤖 Prompt for AI Agents
In src/VCS/Adapter/Git.php around where other Adapter methods are implemented
(add near existing user/org checks), implement the new abstract method
isUserMemberOfOrganization(string $username, string $organization): bool to
satisfy the Adapter contract; the method should call the underlying Git provider
API/client to check whether the given username is a member of the specified
organization and return true or false, gracefully handle API errors (catch
exceptions and return false or propagate according to existing error-handling
patterns used in this adapter), and include appropriate logging consistent with
other methods.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/VCS/Adapter/Git/GitHub.php (1)
644-650: Guard against missing usernames/org to avoid bad API calls.If either
authorUsernameorowneris empty, default to external and skip the API call.Apply this diff:
- if ($isOrgRepository) { - $external = !$this->isUserMemberOfOrganization($authorUsername, $owner); - } else { - $external = $authorUsername !== $owner; - } + if ($isOrgRepository) { + if (empty($authorUsername) || empty($owner)) { + $external = true; + } else { + $external = !$this->isUserMemberOfOrganization($authorUsername, $owner); + } + } else { + $external = ($authorUsername === '') ? true : ($authorUsername !== $owner); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/VCS/Adapter/Git/GitHub.php(2 hunks)tests/VCS/Adapter/GitHubTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/VCS/Adapter/GitHubTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/VCS/Adapter/Git/GitHub.php (1)
src/VCS/Adapter.php (2)
isUserMemberOfOrganization(262-262)call(278-387)
🔇 Additional comments (2)
src/VCS/Adapter/Git/GitHub.php (2)
644-650: Good fix: membership-based external check for org repos.You now correctly derive
authorUsernameand decideexternalby checking org membership. This addresses the prior logic bug.
697-702: GitHub App permission requirement (read org members).This endpoint needs the app permission “Organization members: Read”. Ensure it’s enabled before rollout; otherwise membership checks will 404/403 and everything becomes “external” (safe default).
| public function isUserMemberOfOrganization(string $username, string $organization): bool | ||
| { | ||
| $url = "/orgs/{$organization}/memberships/{$username}"; | ||
| $response = $this->call(self::METHOD_GET, $url, ['Authorization' => "Bearer $this->accessToken"]); | ||
| return $response['headers']['status-code'] >= 200 && $response['headers']['status-code'] < 300; | ||
| } |
There was a problem hiding this comment.
Security bug: treating pending org invites as internal.
The memberships API returns 2xx for both active and pending memberships. You must ensure state === 'active'. Also, avoid failing event parsing on transient API errors; default to “not a member”.
Apply this diff:
- public function isUserMemberOfOrganization(string $username, string $organization): bool
- {
- $url = "/orgs/{$organization}/memberships/{$username}";
- $response = $this->call(self::METHOD_GET, $url, ['Authorization' => "Bearer $this->accessToken"]);
- return $response['headers']['status-code'] >= 200 && $response['headers']['status-code'] < 300;
- }
+ public function isUserMemberOfOrganization(string $username, string $organization): bool
+ {
+ if ($username === '' || $organization === '') {
+ return false;
+ }
+ try {
+ $url = "/orgs/{$organization}/memberships/{$username}";
+ $response = $this->call(self::METHOD_GET, $url, ['Authorization' => "Bearer $this->accessToken"]);
+ $status = $response['headers']['status-code'] ?? 0;
+ if ($status >= 200 && $status < 300) {
+ return (($response['body']['state'] ?? '') === 'active');
+ }
+ return false;
+ } catch (\Throwable $e) {
+ return false;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function isUserMemberOfOrganization(string $username, string $organization): bool | |
| { | |
| $url = "/orgs/{$organization}/memberships/{$username}"; | |
| $response = $this->call(self::METHOD_GET, $url, ['Authorization' => "Bearer $this->accessToken"]); | |
| return $response['headers']['status-code'] >= 200 && $response['headers']['status-code'] < 300; | |
| } | |
| public function isUserMemberOfOrganization(string $username, string $organization): bool | |
| { | |
| if ($username === '' || $organization === '') { | |
| return false; | |
| } | |
| try { | |
| $url = "/orgs/{$organization}/memberships/{$username}"; | |
| $response = $this->call(self::METHOD_GET, $url, ['Authorization' => "Bearer $this->accessToken"]); | |
| $status = $response['headers']['status-code'] ?? 0; | |
| if ($status >= 200 && $status < 300) { | |
| return (($response['body']['state'] ?? '') === 'active'); | |
| } | |
| return false; | |
| } catch (\Throwable $e) { | |
| return false; | |
| } | |
| } |
1 participant
Noticed that we don't check org membership when determining external pull request contributions.
Related task: https://linear.app/appwrite/issue/SER-355/fix-external-vcs-deployments
Summary by CodeRabbit
New Features
Bug Fixes
Tests