Skip to content

fix: Check org membership to determine external PR #46

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.

Sign up for GitHub

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

Open
hmacr wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: Check org membership to determine external PR #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
11c37d3
fix: Check org membership to determine external PR
hmacr Sep 26, 2025
59711d7
doc
hmacr Sep 26, 2025
4913203
feedback
hmacr Sep 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/VCS/Adapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,15 @@ abstract public function getCommit(string $owner, string $repositoryName, string
*/
abstract public function getLatestCommit(string $owner, string $repositoryName, string $branch): array;

/**
* 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;

Comment on lines +255 to +263
Copy link

@coderabbitai coderabbitai bot Sep 26, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 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
done

Length 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.

/**
* Call
*
Expand Down
16 changes: 15 additions & 1 deletion src/VCS/Adapter/Git/GitHub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,14 @@ public function getEvent(string $event, string $payload): array
$authorAvatarUrl = $payload['pull_request']['user']['avatar_url'] ?? '';
$commitHash = $payload['pull_request']['head']['sha'] ?? '';
$headCommitUrl = $repositoryUrl . "/commits/" . $commitHash;
$external = $payload['pull_request']['head']['user']['login'] !== $payload['pull_request']['base']['user']['login'];

$authorUsername = $payload['pull_request']['user']['login'] ?? ($payload['pull_request']['head']['user']['login'] ?? '');
$isOrgRepository = ($payload['repository']['owner']['type'] ?? '') === 'Organization';
if ($isOrgRepository) {
$external = !$this->isUserMemberOfOrganization($authorUsername, $owner);
} else {
$external = $authorUsername !== $owner;
}

return [
'branch' => $branch,
Expand Down Expand Up @@ -686,4 +693,11 @@ public function validateWebhookEvent(string $payload, string $signature, string
{
return $signature === ('sha256=' . hash_hmac('sha256', $payload, $signatureKey));
}

public function isUserMemberOfOrganization(string $username, string $organization): bool
{
$url = "/orgs/{$organization}/memberships/{$username}";
Copy link
Contributor Author

@hmacr hmacr Sep 26, 2025

Choose a reason for hiding this comment

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

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.

$response = $this->call(self::METHOD_GET, $url, ['Authorization' => "Bearer $this->accessToken"]);
return $response['headers']['status-code'] >= 200 && $response['headers']['status-code'] < 300;
}
Comment on lines +697 to +702
Copy link

@coderabbitai coderabbitai bot Sep 26, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
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;
}
}

}
9 changes: 9 additions & 0 deletions tests/VCS/Adapter/GitHubTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,4 +340,13 @@ public function testGetLatestCommit(): void
$this->assertEquals('https://avatars.githubusercontent.com/u/43381712?v=4', $commitDetails['commitAuthorAvatar']);
$this->assertEquals('https://github.com/vermakhushboo', $commitDetails['commitAuthorUrl']);
}

public function testIsUserMemberOfOrganization(): void
{
$isMember = $this->vcsAdapter->isUserMemberOfOrganization('vermakhushboo', 'test-kh');
$this->assertTrue($isMember);

$isNotMember = $this->vcsAdapter->isUserMemberOfOrganization('test-user', 'test-kh');
$this->assertFalse($isNotMember);
}
}
Loading