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

fix(github): handle paginated response #1589

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,6 @@ swagger.json
swagger/swagger.json
bin/
plugins/**

.DS_Store
.vscode
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ import org.springframework.security.oauth2.common.DefaultOAuth2AccessToken
import org.springframework.security.oauth2.common.OAuth2AccessToken
import org.springframework.security.oauth2.client.resource.BaseOAuth2ProtectedResourceDetails
import org.springframework.stereotype.Component
import org.springframework.http.ResponseEntity
import org.springframework.http.HttpHeaders

@Slf4j
@Component
Expand All @@ -39,7 +41,7 @@ class GithubProviderTokenServices implements SpinnakerProviderTokenServices {
GithubRequirements requirements

private String tokenType = DefaultOAuth2AccessToken.BEARER_TYPE
private OAuth2RestOperations restTemplate
OAuth2RestOperations restTemplate
boonware marked this conversation as resolved.
Show resolved Hide resolved

@Component
@ConfigurationProperties("security.oauth2.provider-requirements")
Expand Down Expand Up @@ -71,8 +73,16 @@ class GithubProviderTokenServices implements SpinnakerProviderTokenServices {
token.setTokenType(this.tokenType)
restTemplate.getOAuth2ClientContext().setAccessToken(token)
}
List<Map<String, String>> organizations = restTemplate.getForEntity(organizationsUrl, List.class).getBody()
return githubOrganizationMember(organization, organizations)
ResponseEntity<List<Map<String, String>>> response = restTemplate.getForEntity(organizationsUrl, List.class);
HttpHeaders headers = response.getHeaders();
boolean isMember = githubOrganizationMember(organization, response.getBody())
while (!isMember && hasNextPage(headers)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like there's be less duplication with do/while as opposed to while/do.

Copy link
Author

Choose a reason for hiding this comment

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

Groovy does not have do ... while. We can construct something similar, but the syntax is ugly, IMHO, for example: https://stackoverflow.com/a/46474198

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless you're fired up to convert this file to java, it's fine to leave it as is.

log.debug('Checking next page of user organizations')
response = restTemplate.getForEntity(nextPageUrl(nextLink(headers)), List.class)
isMember = githubOrganizationMember(organization, response.getBody())
headers = response.getHeaders();
}
return isMember
}
catch (Exception e) {
log.warn("Could not fetch user organizations", e)
Expand All @@ -91,4 +101,18 @@ class GithubProviderTokenServices implements SpinnakerProviderTokenServices {
}
return hasRequirements
}

private boolean hasNextPage(HttpHeaders headers) {
return headers.containsKey('link') ? nextLink(headers) != null : false
}

private String nextPageUrl(String nextLink) {
def urlPart = nextLink.split(';')[0]
return urlPart.substring(1, urlPart.length() - 1)
}

private String nextLink(HttpHeaders headers) {
String[] links = headers.getFirst('link').split(',')
return links.find { it.contains('rel="next"') }
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

We try to avoid new groovy code. Any chance you have the stamina to rewrite this in java?

Copy link
Author

@boonware boonware Oct 3, 2024

Choose a reason for hiding this comment

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

I wrote the tests in Groovy to be consistent as the existing test files in the package were also in Groovy, and still are (in fact all of the package's code is Groovy). Would this be a blocker on this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to say, but yes. Please no more groovy code.

* Copyright 2022 Armory, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License")
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.netflix.spinnaker.gate.security.oauth2.provider

import spock.lang.Specification
import spock.lang.Subject
import org.springframework.security.oauth2.client.OAuth2RestTemplate
import org.springframework.security.oauth2.client.OAuth2ClientContext
import org.springframework.security.oauth2.common.OAuth2AccessToken
import org.springframework.boot.autoconfigure.security.oauth2.resource.ResourceServerProperties
import com.netflix.spinnaker.gate.security.oauth2.provider.GithubProviderTokenServices.GithubRequirements
import org.springframework.http.HttpMethod
import org.springframework.http.HttpStatus
import org.springframework.http.HttpHeaders
import org.springframework.http.MediaType
import org.springframework.http.ResponseEntity

class GithubProviderTokenServicesSpec extends Specification {

def 'should find org membership for single-page API response'() {
setup:
def sso = Mock(ResourceServerProperties)
sso.getClientId() >> 'testClientId'
def requirements = new GithubRequirements()
requirements.organization = 'testOrg'
def restTemplate = Mock(OAuth2RestTemplate)
def clientContext = Mock(OAuth2ClientContext)
restTemplate.getOAuth2ClientContext() >> clientContext
clientContext.getAccessToken() >> Mock(OAuth2AccessToken)
@Subject tokenServices = new GithubProviderTokenServices(sso: sso, requirements: requirements)
tokenServices.restTemplate = restTemplate

when: 'the user orgs can be contained in a single-page API response'
HttpHeaders headers = new HttpHeaders();
headers.setContentType(MediaType.APPLICATION_JSON);
def responseEntity = new ResponseEntity<List<Map<String, String>>>([['login': 'testOrg']], headers, HttpStatus.OK);
restTemplate.getForEntity('https://github.com/api/v3/users/1234/orgs', List.class) >> responseEntity

and: 'an API request is made'
boolean isMember = tokenServices.checkOrganization('testToken', 'https://github.com/api/v3/users/1234/orgs', 'testOrg')

then: 'the organization membership is found'
assert isMember
}

def 'should not find org membership for single-page API response'() {
setup:
def sso = Mock(ResourceServerProperties)
sso.getClientId() >> 'testClientId'
def requirements = new GithubRequirements()
requirements.organization = 'testOrg'
def restTemplate = Mock(OAuth2RestTemplate)
def clientContext = Mock(OAuth2ClientContext)
restTemplate.getOAuth2ClientContext() >> clientContext
clientContext.getAccessToken() >> Mock(OAuth2AccessToken)
@Subject tokenServices = new GithubProviderTokenServices(sso: sso, requirements: requirements)
tokenServices.restTemplate = restTemplate

when: 'the user orgs can be contained in a single-page API response'
HttpHeaders headers = new HttpHeaders();
headers.setContentType(MediaType.APPLICATION_JSON);
def response = new ResponseEntity<List<Map<String, String>>>([['login': 'otherOrg']], headers, HttpStatus.OK);
restTemplate.getForEntity('https://github.com/api/v3/users/1234/orgs', List.class) >> response

and: 'an API request is made'
boolean isMember = tokenServices.checkOrganization('testToken', 'https://github.com/api/v3/users/1234/orgs', 'testOrg')

then: 'the organization membership is not found'
assert !isMember
}

def 'should find org membership for multi-page API response'() {
setup:
def sso = Mock(ResourceServerProperties)
sso.getClientId() >> 'testClientId'
def requirements = new GithubRequirements()
requirements.organization = 'testOrg'
def restTemplate = Mock(OAuth2RestTemplate)
def clientContext = Mock(OAuth2ClientContext)
restTemplate.getOAuth2ClientContext() >> clientContext
clientContext.getAccessToken() >> Mock(OAuth2AccessToken)
@Subject tokenServices = new GithubProviderTokenServices(sso: sso, requirements: requirements)
tokenServices.restTemplate = restTemplate

when: 'the user orgs are contained in a multi-page API response'
HttpHeaders headers = new HttpHeaders();
headers.setContentType(MediaType.APPLICATION_JSON);
headers.add('Link', '<https://github.com/api/v3/users/1234/orgs?page=2>; rel="next", <https://github.com/api/v3/user/1234/orgs?page=3>; rel="last"')
def firstResponse = new ResponseEntity<List<Map<String, String>>>([['login': 'otherOrg']], headers, HttpStatus.OK);
restTemplate.getForEntity('https://github.com/api/v3/users/1234/orgs', List.class) >> firstResponse
def secondResponse = new ResponseEntity<List<Map<String, String>>>([['login': 'testOrg']], headers, HttpStatus.OK);
restTemplate.getForEntity('https://github.com/api/v3/users/1234/orgs?page=2', List.class) >> secondResponse

and: 'API requests are made'
boolean isMember = tokenServices.checkOrganization('testToken', 'https://github.com/api/v3/users/1234/orgs', 'testOrg')

then: 'the organization membership is found'
assert isMember
}

def 'should not find org membership for multi-page API response'() {
setup:
def sso = Mock(ResourceServerProperties)
sso.getClientId() >> 'testClientId'
def requirements = new GithubRequirements()
requirements.organization = 'testOrg'
def restTemplate = Mock(OAuth2RestTemplate)
def clientContext = Mock(OAuth2ClientContext)
restTemplate.getOAuth2ClientContext() >> clientContext
clientContext.getAccessToken() >> Mock(OAuth2AccessToken)
@Subject tokenServices = new GithubProviderTokenServices(sso: sso, requirements: requirements)
tokenServices.restTemplate = restTemplate

when: 'the user orgs are contained in a multi-page API response'
HttpHeaders headers = new HttpHeaders();
headers.setContentType(MediaType.APPLICATION_JSON);
headers.add('Link', '<https://github.com/api/v3/users/1234/orgs?page=2>; rel="next", <https://github.com/api/v3/users/1234/orgs?page=3>; rel="last"')
def firstResponse = new ResponseEntity<List<Map<String, String>>>([['login': 'otherOrg']], headers, HttpStatus.OK);
restTemplate.getForEntity('https://github.com/api/v3/users/1234/orgs', List.class) >> firstResponse
headers = new HttpHeaders();
headers.setContentType(MediaType.APPLICATION_JSON);
headers.add('Link', '<https://github.com/api/v3/users/1234/orgs?page=3>; rel="last", <https://github.com/api/v3/users/1234/orgs?page=1>; rel="first"')
def secondResponse = new ResponseEntity<List<Map<String, String>>>([['login': 'anotherOrg']], headers, HttpStatus.OK);
restTemplate.getForEntity('https://github.com/api/v3/users/1234/orgs?page=2', List.class) >> secondResponse

and: 'API requests are made'
boolean isMember = tokenServices.checkOrganization('testToken', 'https://github.com/api/v3/users/1234/orgs', 'testOrg')

then: 'the organization membership is found'
assert !isMember
}


}