From 9364762b781b194f2cf4e4c3268f8ecd56fff9c0 Mon Sep 17 00:00:00 2001 From: Travis Tomsu Date: Wed, 25 Jan 2017 13:39:24 -0500 Subject: [PATCH] Refactors and cleans up dead code in GitHub module (#137) * Refactors and cleans up dead code in GitHub module * Added rate limit header info --- .../spinnaker/fiat/config/GitHubConfig.java | 30 +-- .../github/GithubTeamsUserRolesProvider.java | 199 +++++++++--------- .../roles/github/client/GitHubClient.java | 14 +- .../roles/github/client/GitHubMaster.java | 79 ------- .../fiat/roles/github/model/Team.java | 28 +++ .../roles/github/model/TeamMembership.java | 27 +++ 6 files changed, 174 insertions(+), 203 deletions(-) delete mode 100644 fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/client/GitHubMaster.java create mode 100644 fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/model/Team.java create mode 100644 fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/model/TeamMembership.java diff --git a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/config/GitHubConfig.java b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/config/GitHubConfig.java index bb1144511..c3446d364 100644 --- a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/config/GitHubConfig.java +++ b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/config/GitHubConfig.java @@ -2,8 +2,6 @@ import com.netflix.spinnaker.fiat.roles.github.GitHubProperties; import com.netflix.spinnaker.fiat.roles.github.client.GitHubClient; -import com.netflix.spinnaker.fiat.roles.github.client.GitHubMaster; -import lombok.Getter; import lombok.Setter; import lombok.extern.slf4j.Slf4j; import org.slf4j.Logger; @@ -18,8 +16,6 @@ import retrofit.client.OkClient; import retrofit.converter.JacksonConverter; -import javax.validation.Valid; - /** * Converts the list of GitHub Configuration properties a collection of clients to access the GitHub hosts */ @@ -36,22 +32,18 @@ public class GitHubConfig { @Setter private RestAdapter.LogLevel retrofitLogLevel; - @Bean - public GitHubMaster gitHubMasters(@Valid final GitHubProperties gitHubProperties) { - log.info("bootstrapping " + gitHubProperties.getBaseUrl() + " as github"); - - return new GitHubMaster() - .setGitHubClient(gitHubClient(gitHubProperties.getBaseUrl(), - gitHubProperties.getAccessToken())) - .setBaseUrl(gitHubProperties.getBaseUrl()); - } + @Autowired + @Setter + private GitHubProperties gitHubProperties; - private GitHubClient gitHubClient(String address, String accessToken) { - BasicAuthRequestInterceptor interceptor = new BasicAuthRequestInterceptor(); + @Bean + public GitHubClient gitHubClient() { + BasicAuthRequestInterceptor interceptor = new BasicAuthRequestInterceptor() + .setAccessToken(gitHubProperties.getAccessToken()); return new RestAdapter.Builder() - .setEndpoint(Endpoints.newFixedEndpoint(address)) - .setRequestInterceptor(interceptor.setAccessToken(accessToken)) + .setEndpoint(Endpoints.newFixedEndpoint(gitHubProperties.getBaseUrl())) + .setRequestInterceptor(interceptor) .setClient(okClient) .setConverter(new JacksonConverter()) .setLogLevel(retrofitLogLevel) @@ -79,13 +71,13 @@ public void log(String message) { private static class BasicAuthRequestInterceptor implements RequestInterceptor { - @Getter @Setter private String accessToken; @Override public void intercept(RequestFacade request) { - request.addQueryParam("access_token", accessToken); + // See docs at https://developer.github.com/v3/#authentication + request.addHeader("Authorization", "token " + accessToken); } } } diff --git a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GithubTeamsUserRolesProvider.java b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GithubTeamsUserRolesProvider.java index 5ce5e8a2b..056497125 100644 --- a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GithubTeamsUserRolesProvider.java +++ b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GithubTeamsUserRolesProvider.java @@ -18,7 +18,9 @@ import com.netflix.spinnaker.fiat.model.resources.Role; import com.netflix.spinnaker.fiat.roles.UserRolesProvider; -import com.netflix.spinnaker.fiat.roles.github.client.GitHubMaster; +import com.netflix.spinnaker.fiat.roles.github.client.GitHubClient; +import com.netflix.spinnaker.fiat.roles.github.model.Team; +import com.netflix.spinnaker.fiat.roles.github.model.TeamMembership; import lombok.Setter; import lombok.extern.slf4j.Slf4j; import lombok.val; @@ -29,154 +31,155 @@ import org.springframework.stereotype.Component; import org.springframework.util.Assert; import retrofit.RetrofitError; +import retrofit.client.Header; import retrofit.client.Response; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; @Slf4j @Component @ConditionalOnProperty(value = "auth.groupMembership.service", havingValue = "github") public class GithubTeamsUserRolesProvider implements UserRolesProvider, InitializingBean { + private static List RATE_LIMITING_HEADERS = Arrays.asList( + "X-RateLimit-Limit", + "X-RateLimit-Remaining", + "X-RateLimit-Reset" + ); + @Autowired @Setter - GitHubMaster master; + private GitHubClient gitHubClient; @Autowired @Setter - GitHubProperties gitHubProperties; + private GitHubProperties gitHubProperties; + + @Override + public void afterPropertiesSet() throws Exception { + Assert.state(gitHubProperties.getOrganization() != null, "Supply an organization"); + } @Override - public List loadRoles(String userName) { - log.debug("loadRoles for user " + userName); - if (StringUtils.isEmpty(userName)|| StringUtils.isEmpty(gitHubProperties.getOrganization())) { + public List loadRoles(String username) { + log.debug("loadRoles for user " + username); + if (StringUtils.isEmpty(username) || StringUtils.isEmpty(gitHubProperties.getOrganization())) { + return new ArrayList<>(); + } + + if (!isMemberOfOrg(username)) { + log.debug(username + "is not a member of organization " + gitHubProperties.getOrganization()); return new ArrayList<>(); } - // check organization if set. - // If organization is unset, all GitHub users can login and have full access - // If an organization is set, add it to roles to restrict users to this organization - // If organization is set AND requiredGroupMembership set, organization members will have RO access - // and requiredGroupMembership members RW access - Boolean isMemberOfOrg = false; + log.debug(username + "is a member of organization " + gitHubProperties.getOrganization()); + + List result = new ArrayList<>(); + result.add(toRole(gitHubProperties.getOrganization())); + // Get teams of the org + List teams = getTeams(); + log.debug("Found " + teams.size() + " teams in org."); + + teams.forEach(t -> { + String debugMsg = username + " is a member of team " + t.getName(); + if (isMemberOfTeam(t, username)) { + result.add(toRole(t.getSlug())); + debugMsg += ": true"; + } else { + debugMsg += ": false"; + } + log.debug(debugMsg); + }); + + return result; + } + + private boolean isMemberOfOrg(String username) { + boolean isMemberOfOrg = false; try { - Response response = master.getGitHubClient() - .isMemberOfOrganization(gitHubProperties.getOrganization(), - userName); + Response response = gitHubClient.isMemberOfOrganization(gitHubProperties.getOrganization(), + username); isMemberOfOrg = (response.getStatus() == 204); - if(log.isDebugEnabled()) { - StringBuilder sb = new StringBuilder(userName).append(" is "); - if (!isMemberOfOrg) { - sb.append("not "); - } - sb.append("a member of ") - .append(gitHubProperties.getOrganization()) - .append(" organization."); - log.debug(sb.toString()); - } } catch (RetrofitError e) { - if (e.getKind() == RetrofitError.Kind.NETWORK) { - log.error(String.format("Could not find the server %s", master.getBaseUrl()), e); - return new ArrayList<>(); - } else if (e.getResponse().getStatus() == 404) { - log.error(String.format("%s is not a member of GitHub organization %s", - userName, - gitHubProperties.getOrganization()), - e); - return new ArrayList<>(); - } else if (e.getResponse().getStatus() == 401) { - log.error(String.format("Cannot get GitHub organization %s information: Not authorized.", - gitHubProperties.getOrganization()), - e); - return new ArrayList<>(); + if (e.getResponse().getStatus() != 404) { + handleNon404s(e); } } - if (!isMemberOfOrg) { - return new ArrayList<>(); - } - - List result = new ArrayList<>(); - result.add(toRole(gitHubProperties.getOrganization())); + return isMemberOfOrg; + } - // Get teams of the current user - List teams = new ArrayList<>(); + private List getTeams() { + List teams = new ArrayList<>(); int page = 1; boolean hasMorePages = true; do { - try { - log.debug("Requesting page " + page + " of teams."); - List teamsPage = master.getGitHubClient() - .getOrgTeams(gitHubProperties.getOrganization(), - page++, - gitHubProperties.paginationValue); - teams.addAll(teamsPage); - if (teamsPage.size() != gitHubProperties.paginationValue) { - hasMorePages = false; - } - log.debug("Got " + teamsPage.size() + " teams back. hasMorePages: " + hasMorePages); - } catch (RetrofitError e) { + List teamsPage = getTeamsInOrgPaginated(page++); + teams.addAll(teamsPage); + if (teamsPage.size() != gitHubProperties.paginationValue) { hasMorePages = false; - log.error(String.format("RetrofitError %s %s ", - e.getResponse().getStatus(), - e.getResponse().getReason()), - e); - if (e.getKind() == RetrofitError.Kind.NETWORK) { - log.error(String.format("Could not find the server %s", master.getBaseUrl()), e); - } else if (e.getResponse().getStatus() == 404) { - log.error("404 when getting teams"); - return result; - } else if (e.getResponse().getStatus() == 401) { - log.error(String.format("Cannot get GitHub organization %s teams: Not authorized.", - gitHubProperties.getOrganization()), - e); - return result; - } } + log.debug("Got " + teamsPage.size() + " teams back. hasMorePages: " + hasMorePages); } while (hasMorePages); - log.debug("Found " + teams.size() + " teams in org."); - teams.forEach(t -> { - StringBuilder sb = new StringBuilder(userName).append(" is member of team ").append(t.getName()); - if (isMemberOfTeam(t, userName)) { - sb.append(": true"); - result.add(toRole(t.getSlug())); + return teams; + } + + private List getTeamsInOrgPaginated(int page) { + List teams = new ArrayList<>(); + try { + log.debug("Requesting page " + page + " of teams."); + teams = gitHubClient.getOrgTeams(gitHubProperties.getOrganization(), + page, + gitHubProperties.paginationValue); + } catch (RetrofitError e) { + if (e.getResponse().getStatus() != 404) { + handleNon404s(e); } else { - sb.append(": false"); + log.error("404 when getting teams", e); } - log.debug(sb.toString()); - }); + } - return result; + return teams; } - - private boolean isMemberOfTeam(GitHubMaster.Team t, String userName) { + private boolean isMemberOfTeam(Team t, String username) { String ACTIVE = "active"; try { - GitHubMaster.TeamMembership response = master.getGitHubClient() - .isMemberOfTeam(t.getId(), userName); + TeamMembership response = gitHubClient.isMemberOfTeam(t.getId(), username); return (response.getState().equals(ACTIVE)); } catch (RetrofitError e) { - if (e.getKind() == RetrofitError.Kind.NETWORK) { - log.error(String.format("Could not find the server %s", master.getBaseUrl()), e); - } else if (e.getResponse().getStatus() == 401) { - log.error(String.format("Cannot check if $userName is member of %s teams: Not authorized.", - t.getName()), - e); + if (e.getResponse().getStatus() != 404) { + handleNon404s(e); } } return false; } - @Override - public void afterPropertiesSet() throws Exception { - Assert.state(gitHubProperties.getOrganization ()!= null, "Supply an organization"); + private void handleNon404s(RetrofitError e) { + String msg = ""; + if (e.getKind() == RetrofitError.Kind.NETWORK) { + msg = String.format("Could not find the server %s", gitHubProperties.getBaseUrl()); + } else if (e.getResponse().getStatus() == 401) { + msg = "HTTP 401 Unauthorized."; + } else if (e.getResponse().getStatus() == 403) { + val rateHeaders = e.getResponse() + .getHeaders() + .stream() + .filter(header -> RATE_LIMITING_HEADERS.contains(header.getName())) + .map(Header::toString) + .collect(Collectors.toList()); + + msg = "HTTP 403 Forbidden. Rate limit info: " + StringUtils.join(rateHeaders, ", "); + } + log.error(msg, e); } private static Role toRole(String name) { @@ -194,6 +197,4 @@ public Map> multiLoadRoles(Collection userEmail return emailGroupsMap; } - - } diff --git a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/client/GitHubClient.java b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/client/GitHubClient.java index f64885994..93f019483 100644 --- a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/client/GitHubClient.java +++ b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/client/GitHubClient.java @@ -17,6 +17,8 @@ package com.netflix.spinnaker.fiat.roles.github.client; +import com.netflix.spinnaker.fiat.roles.github.model.Team; +import com.netflix.spinnaker.fiat.roles.github.model.TeamMembership; import retrofit.client.Response; import retrofit.http.GET; import retrofit.http.Path; @@ -37,15 +39,15 @@ Response isMemberOfOrganization(@Path("org") String org, * This one should use the Current User credentials */ @GET("/user/teams") - List getUserTeams(); + List getUserTeams(); @GET("/orgs/{org}/teams") - List getOrgTeams(@Path("org") String org, - @Query("page") int page, - @Query("per_page") int paginationValue); + List getOrgTeams(@Path("org") String org, + @Query("page") int page, + @Query("per_page") int paginationValue); @GET("/teams/{idTeam}/memberships/{username}") - GitHubMaster.TeamMembership isMemberOfTeam(@Path("idTeam") Long idTeam, - @Path("username") String username); + TeamMembership isMemberOfTeam(@Path("idTeam") Long idTeam, + @Path("username") String username); } diff --git a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/client/GitHubMaster.java b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/client/GitHubMaster.java deleted file mode 100644 index 4adbd5644..000000000 --- a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/client/GitHubMaster.java +++ /dev/null @@ -1,79 +0,0 @@ -package com.netflix.spinnaker.fiat.roles.github.client; - -import com.fasterxml.jackson.annotation.JsonIgnoreProperties; -import com.netflix.spinnaker.fiat.roles.github.GitHubProperties; -import lombok.Data; -import lombok.Setter; -import lombok.val; -import org.springframework.context.annotation.Bean; -import retrofit.Endpoints; -import retrofit.RequestInterceptor; -import retrofit.RestAdapter; -import retrofit.client.OkClient; -import retrofit.converter.SimpleXMLConverter; - -import javax.validation.Valid; - -/** - * Wrapper class for a collection of GitHub clients - */ -@Data -public class GitHubMaster { - - @Setter - private GitHubClient gitHubClient; - - @Setter - private String baseUrl; - - @Bean - public GitHubMaster gitHubMasters(@Valid GitHubProperties gitHubProperties) { - val client = gitHubClient(gitHubProperties.getBaseUrl(), - gitHubProperties.getAccessToken()); - - GitHubMaster master = new GitHubMaster(); - master.setGitHubClient(client); - master.setBaseUrl(gitHubProperties.getBaseUrl()); - - return master; - } - - public static GitHubClient gitHubClient(String address, String accessToken) { - return new RestAdapter.Builder() - .setEndpoint(Endpoints.newFixedEndpoint(address)) - .setRequestInterceptor(new BasicAuthRequestInterceptor(accessToken)) - .setClient(new OkClient()) - .setConverter(new SimpleXMLConverter()) - .build() - .create(GitHubClient.class); - } - - @Data - public static class BasicAuthRequestInterceptor implements RequestInterceptor { - private final String accessToken; - - public BasicAuthRequestInterceptor(String accessToken) { - this.accessToken = accessToken; - } - - @Override - public void intercept(RequestFacade request) { - request.addQueryParam("access_token", accessToken); - } - } - - @Data - @JsonIgnoreProperties(ignoreUnknown = true) - public static class TeamMembership { - private String role; - private String state; - } - - @Data - @JsonIgnoreProperties(ignoreUnknown = true) - public static class Team { - private Long id; - private String name; - private String slug; - } -} diff --git a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/model/Team.java b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/model/Team.java new file mode 100644 index 000000000..45ebaca63 --- /dev/null +++ b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/model/Team.java @@ -0,0 +1,28 @@ +/* + * Copyright 2017 Google, 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.fiat.roles.github.model; + +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import lombok.Data; + +@Data +@JsonIgnoreProperties(ignoreUnknown = true) +public class Team { + private Long id; + private String name; + private String slug; +} diff --git a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/model/TeamMembership.java b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/model/TeamMembership.java new file mode 100644 index 000000000..c941c48de --- /dev/null +++ b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/model/TeamMembership.java @@ -0,0 +1,27 @@ +/* + * Copyright 2017 Google, 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.fiat.roles.github.model; + +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import lombok.Data; + +@Data +@JsonIgnoreProperties(ignoreUnknown = true) +public class TeamMembership { + private String role; + private String state; +}