Skip to content

Commit e0fb5d7

Browse files
gusfcarvalhogusfcarvalho
committed
fix: copilot issues
Signed-off-by: Gustavo Carvalho <gustavo.carvalho@container-solutions.com>
1 parent 5542e63 commit e0fb5d7

2 files changed

Lines changed: 217 additions & 38 deletions

File tree

‎internal/data.go‎

Lines changed: 71 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ func (df DataFetcher) FetchData(ctx context.Context, organization string) (*Gith
7373
steps = append(steps, &proto.Step{
7474
Title: "Get SSO Configuration",
7575
Description: "Fetches the SAML SSO configuration for the organization to verify identity provider enforcement",
76-
Remarks: policy_manager.Pointer("More information: https://docs.github.com/en/rest/orgs/orgs?apiVersion=2022-11-28#get-an-organization"),
76+
Remarks: policy_manager.Pointer("More information: https://docs.github.com/en/enterprise-cloud@latest/organizations/managing-saml-single-sign-on-for-your-organization/about-identity-and-access-management-with-saml-single-sign-on"),
7777
})
7878

7979
steps = append(steps, &proto.Step{
@@ -127,20 +127,14 @@ func (df DataFetcher) FetchData(ctx context.Context, organization string) (*Gith
127127

128128
ssoData, err := df.fetchSSO(ctx, organization)
129129
if err != nil {
130-
df.logger.Warn("Could not fetch SSO configuration; marking SSO as disabled", "org", organization, "error", err)
131-
ssoData = &OrgSSO{Enabled: false}
130+
df.logger.Error("Error getting SSO configuration", "org", organization, "error", err)
131+
return nil, nil, err
132132
}
133133

134-
var ipAllowList []IPAllowListEntry
135-
if org.Plan != nil && org.Plan.Name != nil && *org.Plan.Name == "enterprise" {
136-
ipAllowList, err = df.fetchIPAllowList(ctx, organization)
137-
if err != nil {
138-
df.logger.Warn("Could not fetch IP allow-list; treating as empty", "org", organization, "error", err)
139-
ipAllowList = []IPAllowListEntry{}
140-
}
141-
} else {
142-
df.logger.Info("Skipping IP allow-list fetch: requires GitHub Enterprise Cloud", "org", organization, "plan", org.Plan)
143-
ipAllowList = []IPAllowListEntry{}
134+
ipAllowList, err := df.fetchIPAllowList(ctx, organization)
135+
if err != nil {
136+
df.logger.Error("Error getting IP allow-list", "org", organization, "error", err)
137+
return nil, nil, err
144138
}
145139

146140
return &GithubData{
@@ -162,7 +156,7 @@ func (df DataFetcher) fetchSSO(ctx context.Context, organization string) (*OrgSS
162156
SAMLIdentityProvider *samlIdentityProvider `json:"saml_identity_provider"`
163157
}
164158

165-
url := fmt.Sprintf("https://api.github.com/orgs/%s/sso", organization)
159+
url := fmt.Sprintf("orgs/%s/sso", organization)
166160
req, err := df.client.NewRequest(http.MethodGet, url, nil)
167161
if err != nil {
168162
return nil, fmt.Errorf("building SSO request: %w", err)
@@ -190,7 +184,8 @@ func (df DataFetcher) fetchSSO(ctx context.Context, organization string) (*OrgSS
190184

191185
func (df DataFetcher) fetchIPAllowList(ctx context.Context, organization string) ([]IPAllowListEntry, error) {
192186
type graphqlRequest struct {
193-
Query string `json:"query"`
187+
Query string `json:"query"`
188+
Variables map[string]interface{} `json:"variables"`
194189
}
195190
type ipAllowListEntryNode struct {
196191
AllowListValue string `json:"allowListValue"`
@@ -201,7 +196,11 @@ func (df DataFetcher) fetchIPAllowList(ctx context.Context, organization string)
201196
Node ipAllowListEntryNode `json:"node"`
202197
}
203198
type ipAllowListConnection struct {
204-
Edges []ipAllowListEdge `json:"edges"`
199+
Edges []ipAllowListEdge `json:"edges"`
200+
PageInfo struct {
201+
HasNextPage bool `json:"hasNextPage"`
202+
EndCursor *string `json:"endCursor"`
203+
} `json:"pageInfo"`
205204
}
206205
type orgNode struct {
207206
IPAllowListEntries ipAllowListConnection `json:"ipAllowListEntries"`
@@ -216,32 +215,66 @@ func (df DataFetcher) fetchIPAllowList(ctx context.Context, organization string)
216215
} `json:"errors"`
217216
}
218217

219-
gqlQuery := graphqlRequest{
220-
Query: fmt.Sprintf(`{ organization(login: "%s") { ipAllowListEntries(first: 100) { edges { node { allowListValue isActive name } } } } }`, organization),
221-
}
218+
query := `query($login: String!, $after: String) {
219+
organization(login: $login) {
220+
ipAllowListEntries(first: 100, after: $after) {
221+
edges {
222+
node {
223+
allowListValue
224+
isActive
225+
name
226+
}
227+
}
228+
pageInfo {
229+
hasNextPage
230+
endCursor
231+
}
232+
}
233+
}
234+
}`
222235

223-
req, err := df.client.NewRequest(http.MethodPost, "https://api.github.com/graphql", gqlQuery)
224-
if err != nil {
225-
return nil, fmt.Errorf("building IP allow-list GraphQL request: %w", err)
226-
}
236+
var entries []IPAllowListEntry
237+
var after *string
238+
for {
239+
gqlQuery := graphqlRequest{
240+
Query: query,
241+
Variables: map[string]interface{}{
242+
"login": organization,
243+
"after": after,
244+
},
245+
}
227246

228-
var gqlResp graphqlResponse
229-
_, err = df.client.Do(ctx, req, &gqlResp)
230-
if err != nil {
231-
return nil, fmt.Errorf("executing IP allow-list GraphQL query: %w", err)
232-
}
247+
req, err := df.client.NewRequest(http.MethodPost, "graphql", gqlQuery)
248+
if err != nil {
249+
return nil, fmt.Errorf("building IP allow-list GraphQL request: %w", err)
250+
}
233251

234-
if len(gqlResp.Errors) > 0 {
235-
return nil, fmt.Errorf("GraphQL error: %s", gqlResp.Errors[0].Message)
236-
}
252+
var gqlResp graphqlResponse
253+
_, err = df.client.Do(ctx, req, &gqlResp)
254+
if err != nil {
255+
return nil, fmt.Errorf("executing IP allow-list GraphQL query: %w", err)
256+
}
257+
258+
if len(gqlResp.Errors) > 0 {
259+
return nil, fmt.Errorf("GraphQL error: %s", gqlResp.Errors[0].Message)
260+
}
261+
262+
connection := gqlResp.Data.Organization.IPAllowListEntries
263+
for _, edge := range connection.Edges {
264+
entries = append(entries, IPAllowListEntry{
265+
AllowListValue: edge.Node.AllowListValue,
266+
IsActive: edge.Node.IsActive,
267+
Name: edge.Node.Name,
268+
})
269+
}
237270

238-
entries := make([]IPAllowListEntry, 0, len(gqlResp.Data.Organization.IPAllowListEntries.Edges))
239-
for _, edge := range gqlResp.Data.Organization.IPAllowListEntries.Edges {
240-
entries = append(entries, IPAllowListEntry{
241-
AllowListValue: edge.Node.AllowListValue,
242-
IsActive: edge.Node.IsActive,
243-
Name: edge.Node.Name,
244-
})
271+
if !connection.PageInfo.HasNextPage {
272+
break
273+
}
274+
if connection.PageInfo.EndCursor == nil {
275+
return nil, fmt.Errorf("GraphQL response indicated another IP allow-list page without an end cursor")
276+
}
277+
after = connection.PageInfo.EndCursor
245278
}
246279
return entries, nil
247280
}

‎internal/data_test.go‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
package internal
2+
3+
import (
4+
"context"
5+
"encoding/json"
6+
"net/http"
7+
"net/http/httptest"
8+
"net/url"
9+
"testing"
10+
11+
"github.com/google/go-github/v71/github"
12+
"github.com/hashicorp/go-hclog"
13+
)
14+
15+
func testGithubClient(t *testing.T, handler http.Handler) (*github.Client, func()) {
16+
t.Helper()
17+
18+
server := httptest.NewServer(handler)
19+
client := github.NewClient(server.Client())
20+
21+
baseURL, err := url.Parse(server.URL + "/")
22+
if err != nil {
23+
t.Fatalf("parsing test server URL: %v", err)
24+
}
25+
client.BaseURL = baseURL
26+
27+
return client, server.Close
28+
}
29+
30+
func TestFetchSSOUsesRelativeURL(t *testing.T) {
31+
client, cleanup := testGithubClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
32+
if r.Method != http.MethodGet {
33+
t.Fatalf("method = %s, want GET", r.Method)
34+
}
35+
if r.URL.Path != "/orgs/acme/sso" {
36+
t.Fatalf("path = %s, want /orgs/acme/sso", r.URL.Path)
37+
}
38+
39+
w.Header().Set("Content-Type", "application/json")
40+
_, _ = w.Write([]byte(`{"saml_identity_provider":{"sso_url":"https://idp.example/sso","issuer":"https://idp.example"}}`))
41+
}))
42+
defer cleanup()
43+
44+
fetcher := NewDataFetcher(hclog.NewNullLogger(), client)
45+
sso, err := fetcher.fetchSSO(context.Background(), "acme")
46+
if err != nil {
47+
t.Fatalf("fetchSSO returned error: %v", err)
48+
}
49+
if !sso.Enabled {
50+
t.Fatal("SSO should be enabled")
51+
}
52+
if sso.SSOURL != "https://idp.example/sso" {
53+
t.Fatalf("SSOURL = %q, want https://idp.example/sso", sso.SSOURL)
54+
}
55+
if sso.IDPIssuer != "https://idp.example" {
56+
t.Fatalf("IDPIssuer = %q, want https://idp.example", sso.IDPIssuer)
57+
}
58+
}
59+
60+
func TestFetchSSONotFoundMeansDisabled(t *testing.T) {
61+
client, cleanup := testGithubClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
62+
http.NotFound(w, r)
63+
}))
64+
defer cleanup()
65+
66+
fetcher := NewDataFetcher(hclog.NewNullLogger(), client)
67+
sso, err := fetcher.fetchSSO(context.Background(), "acme")
68+
if err != nil {
69+
t.Fatalf("fetchSSO returned error: %v", err)
70+
}
71+
if sso.Enabled {
72+
t.Fatal("SSO should be disabled when the endpoint returns 404")
73+
}
74+
}
75+
76+
func TestFetchIPAllowListUsesVariablesAndPaginates(t *testing.T) {
77+
page := 0
78+
var afterValues []interface{}
79+
client, cleanup := testGithubClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
80+
if r.Method != http.MethodPost {
81+
t.Fatalf("method = %s, want POST", r.Method)
82+
}
83+
if r.URL.Path != "/graphql" {
84+
t.Fatalf("path = %s, want /graphql", r.URL.Path)
85+
}
86+
87+
var request struct {
88+
Query string `json:"query"`
89+
Variables map[string]interface{} `json:"variables"`
90+
}
91+
if err := json.NewDecoder(r.Body).Decode(&request); err != nil {
92+
t.Fatalf("decoding GraphQL request: %v", err)
93+
}
94+
if request.Variables["login"] != "acme" {
95+
t.Fatalf("login variable = %v, want acme", request.Variables["login"])
96+
}
97+
afterValues = append(afterValues, request.Variables["after"])
98+
99+
w.Header().Set("Content-Type", "application/json")
100+
switch page {
101+
case 0:
102+
_, _ = w.Write([]byte(`{"data":{"organization":{"ipAllowListEntries":{"edges":[{"node":{"allowListValue":"192.0.2.0/24","isActive":true,"name":"office"}}],"pageInfo":{"hasNextPage":true,"endCursor":"cursor-1"}}}}}`))
103+
case 1:
104+
_, _ = w.Write([]byte(`{"data":{"organization":{"ipAllowListEntries":{"edges":[{"node":{"allowListValue":"198.51.100.0/24","isActive":false,"name":"vpn"}}],"pageInfo":{"hasNextPage":false,"endCursor":null}}}}}`))
105+
default:
106+
t.Fatalf("unexpected GraphQL page request %d", page)
107+
}
108+
page++
109+
}))
110+
defer cleanup()
111+
112+
fetcher := NewDataFetcher(hclog.NewNullLogger(), client)
113+
entries, err := fetcher.fetchIPAllowList(context.Background(), "acme")
114+
if err != nil {
115+
t.Fatalf("fetchIPAllowList returned error: %v", err)
116+
}
117+
if len(entries) != 2 {
118+
t.Fatalf("len(entries) = %d, want 2", len(entries))
119+
}
120+
if entries[0].AllowListValue != "192.0.2.0/24" || entries[1].AllowListValue != "198.51.100.0/24" {
121+
t.Fatalf("entries = %#v", entries)
122+
}
123+
if len(afterValues) != 2 {
124+
t.Fatalf("after values = %#v, want two requests", afterValues)
125+
}
126+
if afterValues[0] != nil {
127+
t.Fatalf("first after = %#v, want nil", afterValues[0])
128+
}
129+
if afterValues[1] != "cursor-1" {
130+
t.Fatalf("second after = %#v, want cursor-1", afterValues[1])
131+
}
132+
}
133+
134+
func TestFetchIPAllowListErrorsWithoutEndCursor(t *testing.T) {
135+
client, cleanup := testGithubClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
136+
w.Header().Set("Content-Type", "application/json")
137+
_, _ = w.Write([]byte(`{"data":{"organization":{"ipAllowListEntries":{"edges":[],"pageInfo":{"hasNextPage":true,"endCursor":null}}}}}`))
138+
}))
139+
defer cleanup()
140+
141+
fetcher := NewDataFetcher(hclog.NewNullLogger(), client)
142+
_, err := fetcher.fetchIPAllowList(context.Background(), "acme")
143+
if err == nil {
144+
t.Fatal("fetchIPAllowList should error when a next page has no end cursor")
145+
}
146+
}

0 commit comments

Comments
 (0)