You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2022/01/20 13:19:03 UTC
[GitHub] [drill] cgivre opened a new pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
cgivre opened a new pull request #2401:
URL: https://github.com/apache/drill/pull/2401
# [DRILL-8056](https://issues.apache.org/jira/browse/DRILL-8056): Add OAuth2 Support for HTTP Rest Plugin
## Description
This PR adds the ability to query APIs which use OAuth2.0. The PR handles obtaining all the tokens and refreshing them in API calls.
## Documentation
https://github.com/apache/drill/blob/d82e039c6af4d6996d83ce6619c599c90ced8637/contrib/storage-http/OAuth.md
See link: 👆
### Tasks Remaining
- [x] Complete Documentation
## Testing
Manually tested and added unit tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r795983922
##########
File path: logical/src/main/java/org/apache/drill/common/logical/security/CredentialsProvider.java
##########
@@ -33,6 +37,20 @@
* Returns map with authentication credentials. Key is the credential name, for example {@code "username"}
* and map value is corresponding credential value.
*/
+ Logger logger = LoggerFactory.getLogger(CredentialsProvider.class);
+
@JsonIgnore
Map<String, String> getCredentials();
+
+ /**
+ * Set an ephemeral credential. Implementations are not expected to write this
+ * value to persistent storage.
+ */
+ @JsonIgnore
+ default void setCredential(String key, String value) throws UserException {
Review comment:
Removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r795983829
##########
File path: logical/src/main/java/org/apache/drill/common/logical/security/PlainCredentialsProvider.java
##########
@@ -47,6 +51,18 @@ public PlainCredentialsProvider(@JsonProperty("credentials") Map<String, String>
return credentials;
}
+ @Override
+ @JsonIgnore
+ public void setCredential(String key, String value) {
Review comment:
Removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r788783715
##########
File path: exec/java-exec/src/main/resources/rest/storage/update.ftl
##########
@@ -43,6 +44,10 @@
<#else>
<a id="enabled" class="btn btn-success text-white">Enable</a>
</#if>
+ <#if model.getType() == "HttpStoragePluginConfig" >
+ <! -- TODO Also check to see whether the plugin uses OAuth or not -->
Review comment:
Fixed. The button now only appears when the plugin uses OAuth.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r782611662
##########
File path: exec/jdbc-all/pom.xml
##########
@@ -562,7 +562,7 @@
This is likely due to you adding new dependencies to a java-exec and not updating the excludes in this module. This is important as it minimizes the size of the dependency of Drill application users.
</message>
- <maxsize>46600000</maxsize>
+ <maxsize>50000000</maxsize>
Review comment:
I'm not sure why this happened. I did bump the version of `okhttp3` to 4.9.1 or whatever the latest version is, but there are no new dependencies.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/http/HttpOAuthConfig.java
##########
@@ -0,0 +1,173 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.store.http;
+
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;
+import lombok.Builder;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.experimental.Accessors;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.drill.common.PlanStringBuilder;
+
+import java.util.Map;
+
+@Slf4j
+@Builder
+@Getter
+@Setter
+@Accessors(fluent = true)
+@EqualsAndHashCode
+@JsonInclude(JsonInclude.Include.NON_DEFAULT)
+@JsonDeserialize(builder = HttpOAuthConfig.HttpOAuthConfigBuilder.class)
+public class HttpOAuthConfig {
+
+ @JsonProperty("baseURL")
+ private final String baseURL;
+
+ @JsonProperty("clientID")
+ private final String clientID;
+
+ @JsonProperty("clientSecret")
+ private final String clientSecret;
+
+ @JsonProperty("callbackURL")
+ private final String callbackURL;
+
+ @JsonProperty("authorizationURL")
+ private final String authorizationURL;
+
+ @JsonProperty("authorizationPath")
+ private final String authorizationPath;
+
+ @JsonProperty("authorizationParams")
+ private final Map<String, String> authorizationParams;
+
+ @JsonProperty("accessTokenPath")
+ private final String accessTokenPath;
+
+ @JsonProperty("generateCSRFToken")
+ private final boolean generateCSRFToken;
+
+ @JsonProperty("scope")
+ private final String scope;
+
+ @JsonProperty("tokens")
+ private final Map<String, String> tokens;
+
+ /**
+ * Clone constructor used for updating tokens
+ * @param that The original oAuth configs
+ * @param tokens The updated tokens
+ */
+ public HttpOAuthConfig(HttpOAuthConfig that, Map<String, String> tokens) {
+ this.baseURL = that.baseURL;
+ this.clientID = that.clientID;
+ this.clientSecret = that.clientSecret;
+ this.callbackURL = that.callbackURL;
+ this.authorizationURL = that.authorizationURL;
+ this.authorizationPath = that.authorizationPath;
+ this.authorizationParams = that.authorizationParams;
+ this.accessTokenPath = that.accessTokenPath;
+ this.generateCSRFToken = that.generateCSRFToken;
+ this.scope = that.scope;
+ this.tokens = tokens;
+ }
+
+ private HttpOAuthConfig(HttpOAuthConfig.HttpOAuthConfigBuilder builder) {
+ this.baseURL = builder.baseURL;
+ this.clientID = builder.clientID;
+ this.clientSecret = builder.clientSecret;
+ this.callbackURL = builder.callbackURL;
+ this.authorizationURL = builder.authorizationURL;
+ this.authorizationPath = builder.authorizationPath;
+ this.authorizationParams = builder.authorizationParams;
+ this.accessTokenPath = builder.accessTokenPath;
+ this.generateCSRFToken = builder.generateCSRFToken;
+ this.scope = builder.scope;
+ this.tokens = builder.tokens;
+ }
+
+ @Override
+ public String toString() {
+ return new PlanStringBuilder(this)
+ .field("baseURL", baseURL)
+ .field("clientID", clientID)
+ .maskedField("clientSecret", clientSecret)
+ .field("callbackURL", callbackURL)
+ .field("authorizationURL", authorizationURL)
+ .field("authorizationParams", authorizationParams)
+ .field("authorizationPath", authorizationPath)
+ .field("accessTokenPath", accessTokenPath)
+ .field("generateCSRFToken", generateCSRFToken)
+ .field("tokens", tokens.keySet())
+ .toString();
+ }
+
+ @JsonPOJOBuilder(withPrefix = "")
+ public static class HttpOAuthConfigBuilder {
+ @Getter
+ @Setter
Review comment:
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r786866169
##########
File path: exec/jdbc-all/pom.xml
##########
@@ -562,7 +562,7 @@
This is likely due to you adding new dependencies to a java-exec and not updating the excludes in this module. This is important as it minimizes the size of the dependency of Drill application users.
</message>
- <maxsize>46600000</maxsize>
+ <maxsize>50000000</maxsize>
Review comment:
The problem is that it is needed for `java-exec`. I tried this approach and unfortunately, it didn't work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r795927690
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/oauth/Tokens.java
##########
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.oauth;
+
+/**
+ * Aliases table. Provides API for managing and obtaining aliases.
Review comment:
Please fix javadoc
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/oauth/TokenRegistry.java
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.oauth;
+
+import java.util.Iterator;
+import java.util.Map;
+
+/**
+ * Registry for public and user-owned aliases.
Review comment:
Please fix javadoc
##########
File path: logical/src/main/java/org/apache/drill/common/logical/security/CredentialsProvider.java
##########
@@ -33,6 +37,20 @@
* Returns map with authentication credentials. Key is the credential name, for example {@code "username"}
* and map value is corresponding credential value.
*/
+ Logger logger = LoggerFactory.getLogger(CredentialsProvider.class);
+
@JsonIgnore
Map<String, String> getCredentials();
+
+ /**
+ * Set an ephemeral credential. Implementations are not expected to write this
+ * value to persistent storage.
+ */
+ @JsonIgnore
+ default void setCredential(String key, String value) throws UserException {
Review comment:
And please remove this one.
##########
File path: logical/src/main/java/org/apache/drill/common/logical/security/PlainCredentialsProvider.java
##########
@@ -47,6 +51,18 @@ public PlainCredentialsProvider(@JsonProperty("credentials") Map<String, String>
return credentials;
}
+ @Override
+ @JsonIgnore
+ public void setCredential(String key, String value) {
Review comment:
Please delete this method as it is not used anywhere.
##########
File path: pom.xml
##########
@@ -467,6 +467,7 @@
<exclude>**/*.dbf</exclude>
<exclude>**/*.cnf</exclude>
<exclude>**/*.access_log</exclude>
+ <exclude>**/success.ftl</exclude>
Review comment:
Please fix the license header instead of excluding the file.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/oauth/Tokens.java
##########
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.oauth;
+
+/**
+ * Aliases table. Provides API for managing and obtaining aliases.
+ */
+public interface Tokens {
+ /**
+ * Key of {@link this} tokens table.
+ */
+ String getKey();
+
+ /**
+ * Gets the current access token.
+ *
+ * @return The current access token
+ */
+ String getAccessToken();
+
+ /**
+ * Sets the access token.
+ *
+ * @param accessToken Sets the access token.
+ */
+ void setAccessToken(String accessToken);
+
+ String getRefreshToken();
+
+ void setRefreshToken(String refreshToken);
+
+ /**
+ * Returns value from tokens table that corresponds to provided plugin.
+ *
+ * @param token token of the value to obtain
+ * @return value from token table that corresponds to provided plugin
+ */
+ String get(String token);
+
+ /**
+ * Associates provided token with provided plugin in token table.
+ *
+ * @param token alias of the value to associate with
+ * @param value value that will be associated with provided alias
+ * @param replace whether existing value for the same alias should be replaced
+ * @return {@code true} if provided alias was associated with
+ * the provided value in aliases table
Review comment:
And here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r795987213
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/oauth/Tokens.java
##########
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.oauth;
+
+/**
+ * Aliases table. Provides API for managing and obtaining aliases.
+ */
+public interface Tokens {
+ /**
+ * Key of {@link this} tokens table.
+ */
+ String getKey();
+
+ /**
+ * Gets the current access token.
+ *
+ * @return The current access token
+ */
+ String getAccessToken();
+
+ /**
+ * Sets the access token.
+ *
+ * @param accessToken Sets the access token.
+ */
+ void setAccessToken(String accessToken);
+
+ String getRefreshToken();
+
+ void setRefreshToken(String refreshToken);
+
+ /**
+ * Returns value from tokens table that corresponds to provided plugin.
+ *
+ * @param token token of the value to obtain
+ * @return value from token table that corresponds to provided plugin
+ */
+ String get(String token);
+
+ /**
+ * Associates provided token with provided plugin in token table.
+ *
+ * @param token alias of the value to associate with
+ * @param value value that will be associated with provided alias
+ * @param replace whether existing value for the same alias should be replaced
+ * @return {@code true} if provided alias was associated with
+ * the provided value in aliases table
Review comment:
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] lgtm-com[bot] commented on pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #2401:
URL: https://github.com/apache/drill/pull/2401#issuecomment-1023246627
This pull request **introduces 1 alert** when merging 6ea7ecea23b41c03072611f4342ce61fdebead73 into 493ac43af92f165f31e6f6ca3182bd1f324823e3 - [view on LGTM.com](https://lgtm.com/projects/g/apache/drill/rev/pr-c5d9cb72d827b6171726fd5912d296edd9421e47)
**new alerts:**
* 1 for Potential input resource leak
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2401:
URL: https://github.com/apache/drill/pull/2401#issuecomment-1022857519
@vvysotskyi Thanks for your review comments. I believe I addressed all your concerns. The ephemeral tokens are now stored in a persistent store, with the `client_id` and `client_secret` being stored using the `credential provider`.
Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r782512496
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java
##########
@@ -180,6 +193,48 @@ public Response enablePlugin(@PathParam("name") String name, @PathParam("val") B
}
}
+ @GET
+ @Path("/storage/{name}/update_oath2_authtoken")
+ @Produces(MediaType.TEXT_HTML)
+ public Response updateAuthToken(@PathParam("name") String name, @QueryParam("code") String code) {
+ try {
+ if (storage.getPlugin(name).getConfig().getClass().getSimpleName().equalsIgnoreCase("HttpStoragePluginConfig")) {
+ HttpStoragePluginConfig config = (HttpStoragePluginConfig)storage.getPlugin(name).getConfig();
Review comment:
I refactored this so that it uses the `credentialProvider` to store the credentials.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r783226844
##########
File path: exec/jdbc-all/pom.xml
##########
@@ -562,7 +562,7 @@
This is likely due to you adding new dependencies to a java-exec and not updating the excludes in this module. This is important as it minimizes the size of the dependency of Drill application users.
</message>
- <maxsize>46600000</maxsize>
+ <maxsize>50000000</maxsize>
Review comment:
Before your change, okhttp3 was present only for test scope (and used only in some unit tests), and therefore it wasn't included in the JDBC driver. But you have changed its scope from the test to compile, so it is added to the driver. But the question here: is it required in JDBC?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r783536620
##########
File path: exec/jdbc-all/pom.xml
##########
@@ -562,7 +562,7 @@
This is likely due to you adding new dependencies to a java-exec and not updating the excludes in this module. This is important as it minimizes the size of the dependency of Drill application users.
</message>
- <maxsize>46600000</maxsize>
+ <maxsize>50000000</maxsize>
Review comment:
@vvysotskyi Unfortunately I tried excluding it from the JDBC and it does need to be there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r784050679
##########
File path: contrib/storage-http/OAuth.md
##########
@@ -0,0 +1,122 @@
+# OAuth2.0 Authentication in Drill
+Many APIs use OAuth2.0 as a means of authentication. Drill can connect to APIs that use OAuth2 for authentication but OAuth2 is significantly more complex than simple
+username/password authentication.
+
+The good news, and bottom line here is that you can configure Drill to handle all this automagically, but you do have to understand how to configure it so that it works. First,
+let's get a high level understanding of how OAuth works. Click here to [skip to the section on configuring Drill](#configure-drill).
+
+### Understanding the OAuth2 Process
+There are many tutorials as to how OAuth works which we will not repeat here. There are some slight variations but this is a good enough high level overview so you will understand the process works.
+Thus, we will summarize the process as four steps:
+
+#### Step 1: Obtain an Authorization Code
+For the first step, a user will have to log into the API's front end, and authorize the application to access the API. The API will issue you a `clientID` and a
+`client_secret`. We will use these tokens in later stages.
+
+You will also need to provide the API with a `callbackURL`. This URL is how the API sends your application the `authorizationCode` which we will use in step 2.
+Once you have the `clientID` and the `callbackURL`, your application will make a `GET` request to the API to obtain the `authorizationCode`.
+
+#### Step 2: Swap the Authorization Code for an Access Token
+At this point, we need to obtain the `accessToken`. We do so by sending a `POST` request to the API with the `clientID`, the `clientSecret` and the `authorizationCode` we
+obtained in step 1. Note that the `authorizationCode` is a short lived token, but the `accessToken` lasts for a longer period. When the access token expires, you may need to
+either re-authorize the application or use a refresh token to obtain a new one.
+
+#### Step 3: Call the Protected Resource with the Access Token
+Once you have the `accessToken` you are ready to make authenticated API calls. All you have to do here is add the `accessToken` to the API header and you can make API calls
+just like any other.
+
+#### Step 4: (Optional) Obtain a new Access Token using the Refresh Token
+Sometimes, the `accessToken` will expire. When this happens, the API will respond with a `401` not authorized response. When this happens, the application will make a `POST`
+request containing the `clientSecret`, the `clientID` and the `refreshToken` and will obtain new tokens.
+
+## The Artifacts
+Unlike simple username/password authentication, there are about 5 artifacts that you will need to authenticate using OAuth and it is also helpful to understand where they come
+from and to whom they "belong". Let's start with the artifacts that you will need to manually obtain from the API when you register your application: (These are not the Drill
+config variables, but the names are similar. More on that later.)
+* `clientID`: A token to uniquely identify your application with the API.
+* `clientSecret`: A sort of password token which will be used to obtain additional tokens.
+* `callbackURL`: The URL to which access and refresh tokens will be sent. You have to provide this URL when you register your application with the API. If this does not match
+ what you provide the API, the calls will fail.
+* `scope`: An optional parameter which defines the scope of access request for the given access token. The API will provide examples, but you have to pick what accesses you
+ are requesting.
+
+You will need to find two URLs in the API documentation:
+
+* `authorizationURL`: This is the URL from which you will request the `authorizationCode`. You should find this in the API documentation.
+* `tokenURL`: The URL from which you can request the `accessToken`.
+
+There are two other artifacts that you will need, but these artifacts are generated by the API. One thing to note is that while all the other artifacts are owned by the
+application, these two are unique (and "owned by") the user. These artifacts are:
+* `accessToken`: The token which is used to grant access to a protected resource
+* `refreshToken`: The token used to obtain a new `accessToken` without having to re-authorize the application.
+
+Currently, Drill does not allow per-user credentials. However, it is possible to configure Drill to use the Vault Credential Provider so that each individual user has their
Review comment:
You are correct... :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] lgtm-com[bot] commented on pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #2401:
URL: https://github.com/apache/drill/pull/2401#issuecomment-989548997
This pull request **introduces 1 alert** when merging d82e039c6af4d6996d83ce6619c599c90ced8637 into 7ddfb08523ea9fcf0f2f4e6d1307cc9ca9c4c851 - [view on LGTM.com](https://lgtm.com/projects/g/apache/drill/rev/pr-cb95004be65512fade1b932dfe219a2874824cba)
**new alerts:**
* 1 for Dereferenced variable may be null
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r782619735
##########
File path: contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/oauth/AccessTokenRepository.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.store.http.oauth;
+
+import okhttp3.OkHttpClient.Builder;
+import okhttp3.OkHttpClient;
+import okhttp3.Request;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.drill.common.exceptions.UserException;
+import org.apache.drill.exec.store.StoragePluginRegistry;
+import org.apache.drill.exec.store.http.HttpOAuthConfig;
+import org.apache.drill.exec.store.http.HttpStoragePluginConfig;
+import org.apache.drill.exec.store.http.util.HttpProxyConfig;
+import org.apache.drill.exec.store.http.util.SimpleHttp;
+import org.apache.parquet.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Map;
+
+
+public class AccessTokenRepository {
+
+ private static final Logger logger = LoggerFactory.getLogger(AccessTokenRepository.class);
+ private String accessToken;
+ private final OkHttpClient client;
+ private final HttpOAuthConfig oAuthConfig;
+ private final StoragePluginRegistry registry;
+ private final HttpStoragePluginConfig pluginConfig;
+
+ public AccessTokenRepository(HttpOAuthConfig oAuthConfig,
+ HttpProxyConfig proxyConfig,
+ HttpStoragePluginConfig pluginConfig,
+ StoragePluginRegistry registry) {
+ Builder builder = new OkHttpClient.Builder();
+ this.oAuthConfig = oAuthConfig;
+ this.registry = registry;
+ this.pluginConfig = pluginConfig;
+
+ if (oAuthConfig.tokens() != null && oAuthConfig.tokens().containsKey("accessToken")) {
+ accessToken = oAuthConfig.tokens().get("accessToken");
+ }
+
+ // Add proxy info
+ SimpleHttp.addProxyInfo(builder, proxyConfig);
+ client = builder.build();
+ }
+
+ /**
+ * Returns the current access token. Does not perform an HTTP request.
+ * @return The current access token.
+ */
+ public String getAccessToken() {
+ logger.debug("Getting Access token");
+ if (accessToken == null) {
+ return refreshAccessToken();
+ }
+ return accessToken;
+ }
+
+ /**
+ * Refreshes the access token using the code and other information from the HTTP OAuthConfig.
+ * This executes a POST request. This method will throw exceptions if any of the required fields
+ * are empty. This plugin also updates the configuration in the storage plugin registry.
+ *
+ * In the event that a user submits a request and the access token is expired, the API will
+ * return a 401 non-authorized response. In the event of a 401 response, the AccessTokenAuthenticator will
+ * create additional calls to obtain an updated token. This process should be transparent to the user.
+ *
+ * @return String of the new access token.
+ */
+ public String refreshAccessToken() {
+ Request request;
+ logger.debug("Refreshing Access Token.");
+ validateKeys();
+
+ // If the refresh token is present process with that
+ if (oAuthConfig.tokens().containsKey("refreshToken") &&
+ StringUtils.isNotEmpty(oAuthConfig.tokens().get("refreshToken"))) {
+ request = OAuthUtils.getAccessTokenRequestFromRefreshToken(oAuthConfig);
+ } else {
+ request = OAuthUtils.getAccessTokenRequest(oAuthConfig);
+ }
+
+ // Update/Refresh the tokens
+ Map<String, String> tokens = OAuthUtils.getOAuthTokens(client, request);
+ HttpOAuthConfig updatedConfig = new HttpOAuthConfig(oAuthConfig, tokens);
+
+ if (tokens.containsKey("accessToken")) {
+ accessToken = tokens.get("accessToken");
+ }
+
+ // This null check is here for testing only. In actual Drill, the registry will not be null.
+ if (registry != null) {
+ OAuthUtils.updateOAuthTokens(registry, updatedConfig, pluginConfig);
Review comment:
This is completely refactored to store (and update) the creds in the `credentialProvider`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] lgtm-com[bot] commented on pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #2401:
URL: https://github.com/apache/drill/pull/2401#issuecomment-1023352554
This pull request **introduces 1 alert** when merging 92b0e070ecf7d173e9b248e5818aeb8285efae8e into 493ac43af92f165f31e6f6ca3182bd1f324823e3 - [view on LGTM.com](https://lgtm.com/projects/g/apache/drill/rev/pr-285426c242f7dbf085187dbc9e2752f5676f9285)
**new alerts:**
* 1 for Potential input resource leak
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] lgtm-com[bot] commented on pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #2401:
URL: https://github.com/apache/drill/pull/2401#issuecomment-1022893297
This pull request **introduces 1 alert** when merging ee140b987d7f9a1b753d9f9a98aa84ba1310e3af into 239a8f8336936ac51707fbf4a73da86a1156cd59 - [view on LGTM.com](https://lgtm.com/projects/g/apache/drill/rev/pr-436d18816b7d5c6bc46c8113f1942e3032274467)
**new alerts:**
* 1 for Potential input resource leak
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2401:
URL: https://github.com/apache/drill/pull/2401#issuecomment-1016577142
@vvysotskyi Thanks for the review. I fixed the merge conflicts and addressed the comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r782516016
##########
File path: contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/oauth/AccessTokenAuthenticator.java
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.store.http.oauth;
+
+import lombok.NonNull;
+import okhttp3.Authenticator;
+import okhttp3.Request;
+import okhttp3.Response;
+import okhttp3.Route;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class AccessTokenAuthenticator implements Authenticator {
+ private final static Logger logger = LoggerFactory.getLogger(AccessTokenAuthenticator.class);
+
+ private final AccessTokenRepository accessTokenRepository;
+
+ public AccessTokenAuthenticator(AccessTokenRepository accessTokenRepository) {
+ this.accessTokenRepository = accessTokenRepository;
+ }
+
+ @Override
+ public Request authenticate(Route route, @NotNull Response response) {
Review comment:
The way it should work is that it first tries the request with the access token that is stored in Drill. If that request fails, it executes additional requests to obtain an updated token, and then re-executes the request.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
jnturton commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r766654445
##########
File path: contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/oauth/AccessTokenAuthenticator.java
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.store.http.oauth;
+
+import lombok.NonNull;
+import okhttp3.Authenticator;
+import okhttp3.Request;
+import okhttp3.Response;
+import okhttp3.Route;
+import org.jetbrains.annotations.NotNull;
Review comment:
Is `NotNull` something different to `NonNull` that came in intentionally, or did it creep in from a typo? There are occurrences in other files too.
##########
File path: exec/jdbc-all/pom.xml
##########
@@ -562,7 +562,7 @@
This is likely due to you adding new dependencies to a java-exec and not updating the excludes in this module. This is important as it minimizes the size of the dependency of Drill application users.
</message>
- <maxsize>46600000</maxsize>
+ <maxsize>50000000</maxsize>
Review comment:
I still don't completely understand this bit, but should the new dependencies rather be excluded from the jdbc-all module?
##########
File path: exec/java-exec/src/main/resources/rest/storage/update.ftl
##########
@@ -133,6 +138,71 @@
}
});
+ $("#getOauth").click(function() {
Review comment:
I guess this JS should also be inside a `<#if model.getType() == "HttpStoragePluginConfig" >` block if it accesses `#getOauth`?
##########
File path: logical/src/main/java/org/apache/drill/common/logical/security/CredentialsProvider.java
##########
@@ -33,6 +37,16 @@
* Returns map with authentication credentials. Key is the credential name, for example {@code "username"}
* and map value is corresponding credential value.
*/
+ static final Logger logger = LoggerFactory.getLogger(CredentialsProvider.class);
+
@JsonIgnore
Map<String, String> getCredentials();
+
+ @JsonIgnore
+ default void updateCredentials(String key, String value) throws UserException {
+ throw UserException.internalError()
+ .message("Update credential function not implemented for " + Id.NAME)
+ .build(logger);
+ }
Review comment:
```suggestion
/**
* Set an ephemeral credential. Implementations are not expected to write this
* value to persistent storage.
*/
@JsonIgnore
void setCredential(String name, String value);
```
##########
File path: contrib/storage-http/OAuth.md
##########
@@ -0,0 +1,122 @@
+# OAuth2.0 Authentication in Drill
+Many APIs use OAuth2.0 as a means of authentication. Drill can connect to APIs that use OAuth2 for authentication but OAuth2 is significantly more complex than simple
+username/password authentication.
+
+The good news, and bottom line here is that you can configure Drill to handle all this automagically, but you do have to understand how to configure it so that it works. First,
+let's get a high level understanding of how OAuth works. Click here to [skip to the section on configuring Drill](#configure-drill).
+
+### Understanding the OAuth2 Process
+There are many tutorials as to how OAuth works which we will not repeat here. There are some slight variations but this is a good enough high level overview so you will understand the process works.
+Thus, we will summarize the process as four steps:
+
+#### Step 1: Obtain an Authorization Code
+For the first step, a user will have to log into the API's front end, and authorize the application to access the API. The API will issue you a `clientID` and a
+`client_secret`. We will use these tokens in later stages.
+
+You will also need to provide the API with a `callbackURL`. This URL is how the API sends your application the `authorizationCode` which we will use in step 2.
+Once you have the `clientID` and the `callbackURL`, your application will make a `GET` request to the API to obtain the `authorizationCode`.
+
+#### Step 2: Swap the Authorization Code for an Access Token
+At this point, we need to obtain the `accessToken`. We do so by sending a `POST` request to the API with the `clientID`, the `clientSecret` and the `authorizationCode` we
+obtained in step 1. Note that the `authorizationCode` is a short lived token, but the `accessToken` lasts for a longer period. When the access token expires, you may need to
+either re-authorize the application or use a refresh token to obtain a new one.
+
+#### Step 3: Call the Protected Resource with the Access Token
+Once you have the `accessToken` you are ready to make authenticated API calls. All you have to do here is add the `accessToken` to the API header and you can make API calls
+just like any other.
+
+#### Step 4: (Optional) Obtain a new Access Token using the Refresh Token
+Sometimes, the `accessToken` will expire. When this happens, the API will respond with a `401` not authorized response. When this happens, the application will make a `POST`
+request containing the `clientSecret`, the `clientID` and the `refreshToken` and will obtain new tokens.
+
+## The Artifacts
+Unlike simple username/password authentication, there are about 5 artifacts that you will need to authenticate using OAuth and it is also helpful to understand where they come
+from and to whom they "belong". Let's start with the artifacts that you will need to manually obtain from the API when you register your application: (These are not the Drill
+config variables, but the names are similar. More on that later.)
+* `clientID`: A token to uniquely identify your application with the API.
+* `clientSecret`: A sort of password token which will be used to obtain additional tokens.
+* `callbackURL`: The URL to which access and refresh tokens will be sent. You have to provide this URL when you register your application with the API. If this does not match
+ what you provide the API, the calls will fail.
+* `scope`: An optional parameter which defines the scope of access request for the given access token. The API will provide examples, but you have to pick what accesses you
+ are requesting.
+
+You will need to find two URLs in the API documentation:
+
+* `authorizationURL`: This is the URL from which you will request the `authorizationCode`. You should find this in the API documentation.
+* `tokenURL`: The URL from which you can request the `accessToken`.
+
+There are two other artifacts that you will need, but these artifacts are generated by the API. One thing to note is that while all the other artifacts are owned by the
+application, these two are unique (and "owned by") the user. These artifacts are:
+* `accessToken`: The token which is used to grant access to a protected resource
+* `refreshToken`: The token used to obtain a new `accessToken` without having to re-authorize the application.
+
+Currently, Drill does not allow per-user credentials. However, it is possible to configure Drill to use the Vault Credential Provider so that each individual user has their
Review comment:
I don't think this is the case...
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/security/CredentialProviderUtils.java
##########
@@ -43,4 +43,59 @@ public static CredentialsProvider getCredentialsProvider(
}
return new PlainCredentialsProvider(mapBuilder.build());
}
+
+ /**
+ * Constructor for OAuth based authentication. Allows for tokens to be stored in whatever vault
Review comment:
I think we can avoid having all of this OAuth-specific code show up here like this, by a universally-supported `setCredential()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r767162796
##########
File path: contrib/storage-http/src/test/java/org/apache/drill/exec/store/http/oauth/TestOAuthAccessTokenRepository.java
##########
@@ -0,0 +1,168 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.store.http.oauth;
+
+import okhttp3.mockwebserver.MockResponse;
+import okhttp3.mockwebserver.MockWebServer;
+import org.apache.drill.common.exceptions.UserException;
+import org.apache.drill.common.util.DrillFileUtils;
+import org.apache.drill.exec.store.http.HttpOAuthConfig;
+import org.apache.drill.exec.store.http.TestHttpPlugin;
+import org.apache.drill.shaded.guava.com.google.common.base.Charsets;
+import org.apache.drill.shaded.guava.com.google.common.io.Files;
+
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.fail;
+
+public class TestOAuthAccessTokenRepository {
Review comment:
Please extend the test class from `BaseTest`.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/http/HttpOAuthConfig.java
##########
@@ -0,0 +1,173 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.store.http;
+
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;
+import lombok.Builder;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.experimental.Accessors;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.drill.common.PlanStringBuilder;
+
+import java.util.Map;
+
+@Slf4j
+@Builder
+@Getter
+@Setter
+@Accessors(fluent = true)
+@EqualsAndHashCode
+@JsonInclude(JsonInclude.Include.NON_DEFAULT)
+@JsonDeserialize(builder = HttpOAuthConfig.HttpOAuthConfigBuilder.class)
+public class HttpOAuthConfig {
+
+ @JsonProperty("baseURL")
+ private final String baseURL;
+
+ @JsonProperty("clientID")
+ private final String clientID;
+
+ @JsonProperty("clientSecret")
+ private final String clientSecret;
+
+ @JsonProperty("callbackURL")
+ private final String callbackURL;
+
+ @JsonProperty("authorizationURL")
+ private final String authorizationURL;
+
+ @JsonProperty("authorizationPath")
+ private final String authorizationPath;
+
+ @JsonProperty("authorizationParams")
+ private final Map<String, String> authorizationParams;
+
+ @JsonProperty("accessTokenPath")
+ private final String accessTokenPath;
+
+ @JsonProperty("generateCSRFToken")
+ private final boolean generateCSRFToken;
+
+ @JsonProperty("scope")
+ private final String scope;
+
+ @JsonProperty("tokens")
+ private final Map<String, String> tokens;
+
+ /**
+ * Clone constructor used for updating tokens
+ * @param that The original oAuth configs
+ * @param tokens The updated tokens
+ */
+ public HttpOAuthConfig(HttpOAuthConfig that, Map<String, String> tokens) {
+ this.baseURL = that.baseURL;
+ this.clientID = that.clientID;
+ this.clientSecret = that.clientSecret;
+ this.callbackURL = that.callbackURL;
+ this.authorizationURL = that.authorizationURL;
+ this.authorizationPath = that.authorizationPath;
+ this.authorizationParams = that.authorizationParams;
+ this.accessTokenPath = that.accessTokenPath;
+ this.generateCSRFToken = that.generateCSRFToken;
+ this.scope = that.scope;
+ this.tokens = tokens;
+ }
+
+ private HttpOAuthConfig(HttpOAuthConfig.HttpOAuthConfigBuilder builder) {
+ this.baseURL = builder.baseURL;
+ this.clientID = builder.clientID;
+ this.clientSecret = builder.clientSecret;
+ this.callbackURL = builder.callbackURL;
+ this.authorizationURL = builder.authorizationURL;
+ this.authorizationPath = builder.authorizationPath;
+ this.authorizationParams = builder.authorizationParams;
+ this.accessTokenPath = builder.accessTokenPath;
+ this.generateCSRFToken = builder.generateCSRFToken;
+ this.scope = builder.scope;
+ this.tokens = builder.tokens;
+ }
+
+ @Override
+ public String toString() {
+ return new PlanStringBuilder(this)
+ .field("baseURL", baseURL)
+ .field("clientID", clientID)
+ .maskedField("clientSecret", clientSecret)
+ .field("callbackURL", callbackURL)
+ .field("authorizationURL", authorizationURL)
+ .field("authorizationParams", authorizationParams)
+ .field("authorizationPath", authorizationPath)
+ .field("accessTokenPath", accessTokenPath)
+ .field("generateCSRFToken", generateCSRFToken)
+ .field("tokens", tokens.keySet())
+ .toString();
+ }
+
+ @JsonPOJOBuilder(withPrefix = "")
+ public static class HttpOAuthConfigBuilder {
+ @Getter
+ @Setter
Review comment:
Instead of adding annotations for every field, you can add them to the class and they will be applied to every field.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java
##########
@@ -180,6 +193,48 @@ public Response enablePlugin(@PathParam("name") String name, @PathParam("val") B
}
}
+ @GET
+ @Path("/storage/{name}/update_oath2_authtoken")
+ @Produces(MediaType.TEXT_HTML)
+ public Response updateAuthToken(@PathParam("name") String name, @QueryParam("code") String code) {
+ try {
+ if (storage.getPlugin(name).getConfig().getClass().getSimpleName().equalsIgnoreCase("HttpStoragePluginConfig")) {
+ HttpStoragePluginConfig config = (HttpStoragePluginConfig)storage.getPlugin(name).getConfig();
Review comment:
I don't think adding http plugin related code to the exec module is a good idea.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java
##########
@@ -180,6 +193,48 @@ public Response enablePlugin(@PathParam("name") String name, @PathParam("val") B
}
}
+ @GET
+ @Path("/storage/{name}/update_oath2_authtoken")
+ @Produces(MediaType.TEXT_HTML)
+ public Response updateAuthToken(@PathParam("name") String name, @QueryParam("code") String code) {
+ try {
+ if (storage.getPlugin(name).getConfig().getClass().getSimpleName().equalsIgnoreCase("HttpStoragePluginConfig")) {
+ HttpStoragePluginConfig config = (HttpStoragePluginConfig)storage.getPlugin(name).getConfig();
Review comment:
Can we do all these actions when creating the HTTP plugin?
##########
File path: contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/oauth/AccessTokenRepository.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.store.http.oauth;
+
+import okhttp3.OkHttpClient.Builder;
+import okhttp3.OkHttpClient;
+import okhttp3.Request;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.drill.common.exceptions.UserException;
+import org.apache.drill.exec.store.StoragePluginRegistry;
+import org.apache.drill.exec.store.http.HttpOAuthConfig;
+import org.apache.drill.exec.store.http.HttpStoragePluginConfig;
+import org.apache.drill.exec.store.http.util.HttpProxyConfig;
+import org.apache.drill.exec.store.http.util.SimpleHttp;
+import org.apache.parquet.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Map;
+
+
+public class AccessTokenRepository {
+
+ private static final Logger logger = LoggerFactory.getLogger(AccessTokenRepository.class);
+ private String accessToken;
+ private final OkHttpClient client;
+ private final HttpOAuthConfig oAuthConfig;
+ private final StoragePluginRegistry registry;
+ private final HttpStoragePluginConfig pluginConfig;
+
+ public AccessTokenRepository(HttpOAuthConfig oAuthConfig,
+ HttpProxyConfig proxyConfig,
+ HttpStoragePluginConfig pluginConfig,
+ StoragePluginRegistry registry) {
+ Builder builder = new OkHttpClient.Builder();
+ this.oAuthConfig = oAuthConfig;
+ this.registry = registry;
+ this.pluginConfig = pluginConfig;
+
+ if (oAuthConfig.tokens() != null && oAuthConfig.tokens().containsKey("accessToken")) {
+ accessToken = oAuthConfig.tokens().get("accessToken");
+ }
+
+ // Add proxy info
+ SimpleHttp.addProxyInfo(builder, proxyConfig);
+ client = builder.build();
+ }
+
+ /**
+ * Returns the current access token. Does not perform an HTTP request.
+ * @return The current access token.
+ */
+ public String getAccessToken() {
+ logger.debug("Getting Access token");
+ if (accessToken == null) {
+ return refreshAccessToken();
+ }
+ return accessToken;
+ }
+
+ /**
+ * Refreshes the access token using the code and other information from the HTTP OAuthConfig.
+ * This executes a POST request. This method will throw exceptions if any of the required fields
+ * are empty. This plugin also updates the configuration in the storage plugin registry.
+ *
+ * In the event that a user submits a request and the access token is expired, the API will
+ * return a 401 non-authorized response. In the event of a 401 response, the AccessTokenAuthenticator will
+ * create additional calls to obtain an updated token. This process should be transparent to the user.
+ *
+ * @return String of the new access token.
+ */
+ public String refreshAccessToken() {
+ Request request;
+ logger.debug("Refreshing Access Token.");
+ validateKeys();
+
+ // If the refresh token is present process with that
+ if (oAuthConfig.tokens().containsKey("refreshToken") &&
+ StringUtils.isNotEmpty(oAuthConfig.tokens().get("refreshToken"))) {
+ request = OAuthUtils.getAccessTokenRequestFromRefreshToken(oAuthConfig);
+ } else {
+ request = OAuthUtils.getAccessTokenRequest(oAuthConfig);
+ }
+
+ // Update/Refresh the tokens
+ Map<String, String> tokens = OAuthUtils.getOAuthTokens(client, request);
+ HttpOAuthConfig updatedConfig = new HttpOAuthConfig(oAuthConfig, tokens);
+
+ if (tokens.containsKey("accessToken")) {
+ accessToken = tokens.get("accessToken");
+ }
+
+ // This null check is here for testing only. In actual Drill, the registry will not be null.
+ if (registry != null) {
+ OAuthUtils.updateOAuthTokens(registry, updatedConfig, pluginConfig);
Review comment:
Please see the previous comment regarding storing access tokens in ZK...
##########
File path: exec/jdbc-all/pom.xml
##########
@@ -562,7 +562,7 @@
This is likely due to you adding new dependencies to a java-exec and not updating the excludes in this module. This is important as it minimizes the size of the dependency of Drill application users.
</message>
- <maxsize>46600000</maxsize>
+ <maxsize>50000000</maxsize>
Review comment:
Is there any reason for updating this limit? The functionality is only for HTTP plugin, so it shouldn't affect jdbc driver size.
##########
File path: contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/oauth/AccessTokenAuthenticator.java
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.store.http.oauth;
+
+import lombok.NonNull;
+import okhttp3.Authenticator;
+import okhttp3.Request;
+import okhttp3.Response;
+import okhttp3.Route;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class AccessTokenAuthenticator implements Authenticator {
+ private final static Logger logger = LoggerFactory.getLogger(AccessTokenAuthenticator.class);
+
+ private final AccessTokenRepository accessTokenRepository;
+
+ public AccessTokenAuthenticator(AccessTokenRepository accessTokenRepository) {
+ this.accessTokenRepository = accessTokenRepository;
+ }
+
+ @Override
+ public Request authenticate(Route route, @NotNull Response response) {
Review comment:
According to this logic, the access token will be regenerated for every `authenticate` call. So do we actually need to store it in the storage config and persist it to the zookeeper instead of holding it for a specific HTTP client?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on pull request #2401:
URL: https://github.com/apache/drill/pull/2401#issuecomment-990055478
Looks like there are some issues with added tests:
```
Error: org.apache.drill.exec.store.http.oauth.TestOAuthAccessTokenRepository.testGetAccessToken Time elapsed: 0.042 s <<< ERROR!
java.net.BindException: Address already in use (Bind failed)
at org.apache.drill.exec.store.http.oauth.TestOAuthAccessTokenRepository.testGetAccessToken(TestOAuthAccessTokenRepository.java:56)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on pull request #2401:
URL: https://github.com/apache/drill/pull/2401#issuecomment-1017659175
@cgivre, for now, we can query Dropbox using the file storage plugin, but Dropbox also supports OAuth (in their docs I didn't find differences with OAuth), so extracting that logic to the OAuth creds provider and removing dependency to HTTP plugin will automatically enable this and other similar cases. If we have some specific logic for Google Sheets, in the future we can extend that implementation of the OAuth creds provider to handle additional cases.
Regarding concurrent updates of the token, I think it is a major issue because we could have high concurrent queries (even for a single user) that will be slowed down because of unnecessary updating of the access_token. Please take a look at the versioned persistent store, I think it can be used to solve that issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r783252405
##########
File path: exec/jdbc-all/pom.xml
##########
@@ -562,7 +562,7 @@
This is likely due to you adding new dependencies to a java-exec and not updating the excludes in this module. This is important as it minimizes the size of the dependency of Drill application users.
</message>
- <maxsize>46600000</maxsize>
+ <maxsize>50000000</maxsize>
Review comment:
@vvysotskyi Thanks for looking at this. It actually is used in one of the files for Drill's internal rest API. (See below). Does that need to be included in the JDBC driver?
https://github.com/apache/drill/blob/d6c54936129f8e2a8eda83be7b1ab69998e893f8/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java#L206-L12
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r783258557
##########
File path: exec/jdbc-all/pom.xml
##########
@@ -562,7 +562,7 @@
This is likely due to you adding new dependencies to a java-exec and not updating the excludes in this module. This is important as it minimizes the size of the dependency of Drill application users.
</message>
- <maxsize>46600000</maxsize>
+ <maxsize>50000000</maxsize>
Review comment:
I don't think it should be included, so please exclude it from the driver.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
jnturton commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r784146363
##########
File path: exec/jdbc-all/pom.xml
##########
@@ -562,7 +562,7 @@
This is likely due to you adding new dependencies to a java-exec and not updating the excludes in this module. This is important as it minimizes the size of the dependency of Drill application users.
</message>
- <maxsize>46600000</maxsize>
+ <maxsize>50000000</maxsize>
Review comment:
@cgivre I _think_ you include in java-exec's pom and also explcitly _exclude_ in the java-exec dependency section in jdbc-all's pom.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2401:
URL: https://github.com/apache/drill/pull/2401#issuecomment-990378477
> Looks like there are some issues with added tests:
>
> ```
> Error: org.apache.drill.exec.store.http.oauth.TestOAuthAccessTokenRepository.testGetAccessToken Time elapsed: 0.042 s <<< ERROR!
> java.net.BindException: Address already in use (Bind failed)
> at org.apache.drill.exec.store.http.oauth.TestOAuthAccessTokenRepository.testGetAccessToken(TestOAuthAccessTokenRepository.java:56)
> ```
That was weird. The test ran fine in my IDE. I think I fixed it now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r786152757
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java
##########
@@ -62,6 +72,10 @@
@RolesAllowed(ADMIN_ROLE)
public class StorageResources {
private static final Logger logger = LoggerFactory.getLogger(StorageResources.class);
+ private static final String OAUTH_SUCCESS = "<html>\n" + "<head>\n" + " <title>Success!</title>\n" + " <link rel=\"shortcut icon\" href=\"/static/img/drill.ico\">\n" + " " +
Review comment:
Please move to the file and use ftl template as it is done for all other UI stuff.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java
##########
@@ -180,6 +194,47 @@ public Response enablePlugin(@PathParam("name") String name, @PathParam("val") B
}
}
+ @GET
+ @Path("/storage/{name}/update_oath2_authtoken")
+ @Produces(MediaType.TEXT_HTML)
+ public Response updateAuthToken(@PathParam("name") String name, @QueryParam("code") String code) {
+ try {
+ if (storage.getPlugin(name).getConfig().getClass().getSimpleName().equalsIgnoreCase("HttpStoragePluginConfig")) {
Review comment:
Let's remove this check for `HttpStoragePluginConfig`, and add only check that plugin config is an instance of `AbstractSecuredStoragePluginConfig`. Theoretically, there might be other plugins that might want to use this functionality.
##########
File path: exec/java-exec/src/main/resources/rest/storage/update.ftl
##########
@@ -43,6 +44,10 @@
<#else>
<a id="enabled" class="btn btn-success text-white">Enable</a>
</#if>
+ <#if model.getType() == "HttpStoragePluginConfig" >
+ <! -- TODO Also check to see whether the plugin uses OAuth or not -->
Review comment:
I think it makes sense to add some method to `StoragePlugin` or `StoragePluginConfig` to check whether it supports OAuth.
##########
File path: contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpOAuthConfig.java
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.store.http;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;
+import lombok.Builder;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.ToString;
+import lombok.extern.slf4j.Slf4j;
+
+import java.util.Map;
+
+@Slf4j
+@ToString
+@Getter
+@Setter
+@Builder
+@EqualsAndHashCode
+@JsonInclude(JsonInclude.Include.NON_DEFAULT)
+@JsonDeserialize(builder = HttpOAuthConfig.HttpOAuthConfigBuilder.class)
+public class HttpOAuthConfig {
Review comment:
I don't see the place where this class is actually used...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r788757688
##########
File path: contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpOAuthConfig.java
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.store.http;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;
+import lombok.Builder;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.ToString;
+import lombok.extern.slf4j.Slf4j;
+
+import java.util.Map;
+
+@Slf4j
+@ToString
+@Getter
+@Setter
+@Builder
+@EqualsAndHashCode
+@JsonInclude(JsonInclude.Include.NON_DEFAULT)
+@JsonDeserialize(builder = HttpOAuthConfig.HttpOAuthConfigBuilder.class)
+public class HttpOAuthConfig {
Review comment:
The `OAuthConfig` class is used to hold several variables that don't end up in the credential provider. Specifically, these are config parameters like `accessTokenInHeader`, `scope`, `tokenType` etc. These didn't really seem like they belong in the `credentialsProvider`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre closed pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre closed pull request #2401:
URL: https://github.com/apache/drill/pull/2401
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r783477833
##########
File path: exec/jdbc-all/pom.xml
##########
@@ -562,7 +562,7 @@
This is likely due to you adding new dependencies to a java-exec and not updating the excludes in this module. This is important as it minimizes the size of the dependency of Drill application users.
</message>
- <maxsize>46600000</maxsize>
+ <maxsize>50000000</maxsize>
Review comment:
@vvysotskyi Fixed!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r784050447
##########
File path: exec/jdbc-all/pom.xml
##########
@@ -562,7 +562,7 @@
This is likely due to you adding new dependencies to a java-exec and not updating the excludes in this module. This is important as it minimizes the size of the dependency of Drill application users.
</message>
- <maxsize>46600000</maxsize>
+ <maxsize>50000000</maxsize>
Review comment:
Unfortunately, I had to include them because there is an `OAuthUtils` class which uses OkHttp which is part of java-exec. If you have some advice as to where else I could put that, I'm all ears.
The issue is that both the rest server and the http storage plugin needs to be able to access the same body of code for obtaining and refreshing tokens.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r784053795
##########
File path: logical/src/main/java/org/apache/drill/common/logical/security/CredentialsProvider.java
##########
@@ -33,6 +37,16 @@
* Returns map with authentication credentials. Key is the credential name, for example {@code "username"}
* and map value is corresponding credential value.
*/
+ static final Logger logger = LoggerFactory.getLogger(CredentialsProvider.class);
+
@JsonIgnore
Map<String, String> getCredentials();
+
+ @JsonIgnore
+ default void updateCredentials(String key, String value) throws UserException {
+ throw UserException.internalError()
+ .message("Update credential function not implemented for " + Id.NAME)
+ .build(logger);
+ }
Review comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r788788385
##########
File path: contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/oauth/AccessTokenAuthenticator.java
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.store.http.oauth;
+
+import lombok.NonNull;
+import okhttp3.Authenticator;
+import okhttp3.Request;
+import okhttp3.Response;
+import okhttp3.Route;
+import org.jetbrains.annotations.NotNull;
Review comment:
Fixed, I think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r795982198
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/oauth/TokenRegistry.java
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.oauth;
+
+import java.util.Iterator;
+import java.util.Map;
+
+/**
+ * Registry for public and user-owned aliases.
Review comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre merged pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre merged pull request #2401:
URL: https://github.com/apache/drill/pull/2401
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r767162796
##########
File path: contrib/storage-http/src/test/java/org/apache/drill/exec/store/http/oauth/TestOAuthAccessTokenRepository.java
##########
@@ -0,0 +1,168 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.store.http.oauth;
+
+import okhttp3.mockwebserver.MockResponse;
+import okhttp3.mockwebserver.MockWebServer;
+import org.apache.drill.common.exceptions.UserException;
+import org.apache.drill.common.util.DrillFileUtils;
+import org.apache.drill.exec.store.http.HttpOAuthConfig;
+import org.apache.drill.exec.store.http.TestHttpPlugin;
+import org.apache.drill.shaded.guava.com.google.common.base.Charsets;
+import org.apache.drill.shaded.guava.com.google.common.io.Files;
+
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.fail;
+
+public class TestOAuthAccessTokenRepository {
Review comment:
Please extend the test class from `BaseTest`.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/http/HttpOAuthConfig.java
##########
@@ -0,0 +1,173 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.store.http;
+
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;
+import lombok.Builder;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.experimental.Accessors;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.drill.common.PlanStringBuilder;
+
+import java.util.Map;
+
+@Slf4j
+@Builder
+@Getter
+@Setter
+@Accessors(fluent = true)
+@EqualsAndHashCode
+@JsonInclude(JsonInclude.Include.NON_DEFAULT)
+@JsonDeserialize(builder = HttpOAuthConfig.HttpOAuthConfigBuilder.class)
+public class HttpOAuthConfig {
+
+ @JsonProperty("baseURL")
+ private final String baseURL;
+
+ @JsonProperty("clientID")
+ private final String clientID;
+
+ @JsonProperty("clientSecret")
+ private final String clientSecret;
+
+ @JsonProperty("callbackURL")
+ private final String callbackURL;
+
+ @JsonProperty("authorizationURL")
+ private final String authorizationURL;
+
+ @JsonProperty("authorizationPath")
+ private final String authorizationPath;
+
+ @JsonProperty("authorizationParams")
+ private final Map<String, String> authorizationParams;
+
+ @JsonProperty("accessTokenPath")
+ private final String accessTokenPath;
+
+ @JsonProperty("generateCSRFToken")
+ private final boolean generateCSRFToken;
+
+ @JsonProperty("scope")
+ private final String scope;
+
+ @JsonProperty("tokens")
+ private final Map<String, String> tokens;
+
+ /**
+ * Clone constructor used for updating tokens
+ * @param that The original oAuth configs
+ * @param tokens The updated tokens
+ */
+ public HttpOAuthConfig(HttpOAuthConfig that, Map<String, String> tokens) {
+ this.baseURL = that.baseURL;
+ this.clientID = that.clientID;
+ this.clientSecret = that.clientSecret;
+ this.callbackURL = that.callbackURL;
+ this.authorizationURL = that.authorizationURL;
+ this.authorizationPath = that.authorizationPath;
+ this.authorizationParams = that.authorizationParams;
+ this.accessTokenPath = that.accessTokenPath;
+ this.generateCSRFToken = that.generateCSRFToken;
+ this.scope = that.scope;
+ this.tokens = tokens;
+ }
+
+ private HttpOAuthConfig(HttpOAuthConfig.HttpOAuthConfigBuilder builder) {
+ this.baseURL = builder.baseURL;
+ this.clientID = builder.clientID;
+ this.clientSecret = builder.clientSecret;
+ this.callbackURL = builder.callbackURL;
+ this.authorizationURL = builder.authorizationURL;
+ this.authorizationPath = builder.authorizationPath;
+ this.authorizationParams = builder.authorizationParams;
+ this.accessTokenPath = builder.accessTokenPath;
+ this.generateCSRFToken = builder.generateCSRFToken;
+ this.scope = builder.scope;
+ this.tokens = builder.tokens;
+ }
+
+ @Override
+ public String toString() {
+ return new PlanStringBuilder(this)
+ .field("baseURL", baseURL)
+ .field("clientID", clientID)
+ .maskedField("clientSecret", clientSecret)
+ .field("callbackURL", callbackURL)
+ .field("authorizationURL", authorizationURL)
+ .field("authorizationParams", authorizationParams)
+ .field("authorizationPath", authorizationPath)
+ .field("accessTokenPath", accessTokenPath)
+ .field("generateCSRFToken", generateCSRFToken)
+ .field("tokens", tokens.keySet())
+ .toString();
+ }
+
+ @JsonPOJOBuilder(withPrefix = "")
+ public static class HttpOAuthConfigBuilder {
+ @Getter
+ @Setter
Review comment:
Instead of adding annotations for every field, you can add them to the class and they will be applied to every field.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java
##########
@@ -180,6 +193,48 @@ public Response enablePlugin(@PathParam("name") String name, @PathParam("val") B
}
}
+ @GET
+ @Path("/storage/{name}/update_oath2_authtoken")
+ @Produces(MediaType.TEXT_HTML)
+ public Response updateAuthToken(@PathParam("name") String name, @QueryParam("code") String code) {
+ try {
+ if (storage.getPlugin(name).getConfig().getClass().getSimpleName().equalsIgnoreCase("HttpStoragePluginConfig")) {
+ HttpStoragePluginConfig config = (HttpStoragePluginConfig)storage.getPlugin(name).getConfig();
Review comment:
I don't think adding http plugin related code to the exec module is a good idea.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java
##########
@@ -180,6 +193,48 @@ public Response enablePlugin(@PathParam("name") String name, @PathParam("val") B
}
}
+ @GET
+ @Path("/storage/{name}/update_oath2_authtoken")
+ @Produces(MediaType.TEXT_HTML)
+ public Response updateAuthToken(@PathParam("name") String name, @QueryParam("code") String code) {
+ try {
+ if (storage.getPlugin(name).getConfig().getClass().getSimpleName().equalsIgnoreCase("HttpStoragePluginConfig")) {
+ HttpStoragePluginConfig config = (HttpStoragePluginConfig)storage.getPlugin(name).getConfig();
Review comment:
Can we do all these actions when creating the HTTP plugin?
##########
File path: contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/oauth/AccessTokenRepository.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.store.http.oauth;
+
+import okhttp3.OkHttpClient.Builder;
+import okhttp3.OkHttpClient;
+import okhttp3.Request;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.drill.common.exceptions.UserException;
+import org.apache.drill.exec.store.StoragePluginRegistry;
+import org.apache.drill.exec.store.http.HttpOAuthConfig;
+import org.apache.drill.exec.store.http.HttpStoragePluginConfig;
+import org.apache.drill.exec.store.http.util.HttpProxyConfig;
+import org.apache.drill.exec.store.http.util.SimpleHttp;
+import org.apache.parquet.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Map;
+
+
+public class AccessTokenRepository {
+
+ private static final Logger logger = LoggerFactory.getLogger(AccessTokenRepository.class);
+ private String accessToken;
+ private final OkHttpClient client;
+ private final HttpOAuthConfig oAuthConfig;
+ private final StoragePluginRegistry registry;
+ private final HttpStoragePluginConfig pluginConfig;
+
+ public AccessTokenRepository(HttpOAuthConfig oAuthConfig,
+ HttpProxyConfig proxyConfig,
+ HttpStoragePluginConfig pluginConfig,
+ StoragePluginRegistry registry) {
+ Builder builder = new OkHttpClient.Builder();
+ this.oAuthConfig = oAuthConfig;
+ this.registry = registry;
+ this.pluginConfig = pluginConfig;
+
+ if (oAuthConfig.tokens() != null && oAuthConfig.tokens().containsKey("accessToken")) {
+ accessToken = oAuthConfig.tokens().get("accessToken");
+ }
+
+ // Add proxy info
+ SimpleHttp.addProxyInfo(builder, proxyConfig);
+ client = builder.build();
+ }
+
+ /**
+ * Returns the current access token. Does not perform an HTTP request.
+ * @return The current access token.
+ */
+ public String getAccessToken() {
+ logger.debug("Getting Access token");
+ if (accessToken == null) {
+ return refreshAccessToken();
+ }
+ return accessToken;
+ }
+
+ /**
+ * Refreshes the access token using the code and other information from the HTTP OAuthConfig.
+ * This executes a POST request. This method will throw exceptions if any of the required fields
+ * are empty. This plugin also updates the configuration in the storage plugin registry.
+ *
+ * In the event that a user submits a request and the access token is expired, the API will
+ * return a 401 non-authorized response. In the event of a 401 response, the AccessTokenAuthenticator will
+ * create additional calls to obtain an updated token. This process should be transparent to the user.
+ *
+ * @return String of the new access token.
+ */
+ public String refreshAccessToken() {
+ Request request;
+ logger.debug("Refreshing Access Token.");
+ validateKeys();
+
+ // If the refresh token is present process with that
+ if (oAuthConfig.tokens().containsKey("refreshToken") &&
+ StringUtils.isNotEmpty(oAuthConfig.tokens().get("refreshToken"))) {
+ request = OAuthUtils.getAccessTokenRequestFromRefreshToken(oAuthConfig);
+ } else {
+ request = OAuthUtils.getAccessTokenRequest(oAuthConfig);
+ }
+
+ // Update/Refresh the tokens
+ Map<String, String> tokens = OAuthUtils.getOAuthTokens(client, request);
+ HttpOAuthConfig updatedConfig = new HttpOAuthConfig(oAuthConfig, tokens);
+
+ if (tokens.containsKey("accessToken")) {
+ accessToken = tokens.get("accessToken");
+ }
+
+ // This null check is here for testing only. In actual Drill, the registry will not be null.
+ if (registry != null) {
+ OAuthUtils.updateOAuthTokens(registry, updatedConfig, pluginConfig);
Review comment:
Please see the previous comment regarding storing access tokens in ZK...
##########
File path: exec/jdbc-all/pom.xml
##########
@@ -562,7 +562,7 @@
This is likely due to you adding new dependencies to a java-exec and not updating the excludes in this module. This is important as it minimizes the size of the dependency of Drill application users.
</message>
- <maxsize>46600000</maxsize>
+ <maxsize>50000000</maxsize>
Review comment:
Is there any reason for updating this limit? The functionality is only for HTTP plugin, so it shouldn't affect jdbc driver size.
##########
File path: contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/oauth/AccessTokenAuthenticator.java
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.store.http.oauth;
+
+import lombok.NonNull;
+import okhttp3.Authenticator;
+import okhttp3.Request;
+import okhttp3.Response;
+import okhttp3.Route;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class AccessTokenAuthenticator implements Authenticator {
+ private final static Logger logger = LoggerFactory.getLogger(AccessTokenAuthenticator.class);
+
+ private final AccessTokenRepository accessTokenRepository;
+
+ public AccessTokenAuthenticator(AccessTokenRepository accessTokenRepository) {
+ this.accessTokenRepository = accessTokenRepository;
+ }
+
+ @Override
+ public Request authenticate(Route route, @NotNull Response response) {
Review comment:
According to this logic, the access token will be regenerated for every `authenticate` call. So do we actually need to store it in the storage config and persist it to the zookeeper instead of holding it for a specific HTTP client?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r796000232
##########
File path: pom.xml
##########
@@ -467,6 +467,7 @@
<exclude>**/*.dbf</exclude>
<exclude>**/*.cnf</exclude>
<exclude>**/*.access_log</exclude>
+ <exclude>**/success.ftl</exclude>
Review comment:
Done. I renamed the file `success.html`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r795985634
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/oauth/Tokens.java
##########
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.oauth;
+
+/**
+ * Aliases table. Provides API for managing and obtaining aliases.
Review comment:
Fixed. I removed the Javadoc for this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r793259456
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java
##########
@@ -62,6 +72,10 @@
@RolesAllowed(ADMIN_ROLE)
public class StorageResources {
private static final Logger logger = LoggerFactory.getLogger(StorageResources.class);
+ private static final String OAUTH_SUCCESS = "<html>\n" + "<head>\n" + " <title>Success!</title>\n" + " <link rel=\"shortcut icon\" href=\"/static/img/drill.ico\">\n" + " " +
Review comment:
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r782512912
##########
File path: contrib/storage-http/src/test/java/org/apache/drill/exec/store/http/oauth/TestOAuthAccessTokenRepository.java
##########
@@ -0,0 +1,168 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.drill.exec.store.http.oauth;
+
+import okhttp3.mockwebserver.MockResponse;
+import okhttp3.mockwebserver.MockWebServer;
+import org.apache.drill.common.exceptions.UserException;
+import org.apache.drill.common.util.DrillFileUtils;
+import org.apache.drill.exec.store.http.HttpOAuthConfig;
+import org.apache.drill.exec.store.http.TestHttpPlugin;
+import org.apache.drill.shaded.guava.com.google.common.base.Charsets;
+import org.apache.drill.shaded.guava.com.google.common.io.Files;
+
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.fail;
+
+public class TestOAuthAccessTokenRepository {
Review comment:
I actually removed this entire class. Added different tests that better test the whole process.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r786914543
##########
File path: exec/java-exec/src/main/resources/rest/storage/update.ftl
##########
@@ -133,6 +138,71 @@
}
});
+ $("#getOauth").click(function() {
Review comment:
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2401:
URL: https://github.com/apache/drill/pull/2401#issuecomment-1015618627
@vvysotskyi I added a parameter to put the access token in the URL instead of the header. This is technically not part of the OAuth spec....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2401:
URL: https://github.com/apache/drill/pull/2401#issuecomment-1017072684
@vvysotskyi
To answer your questions, I actually first attempted this with a dedicated `OAuthCredentialProvider` class. The issue I ran into was that it needed the `SimpleHttp` class within the `HttpStoragePlugin` to actually do all the REST calls. For instance, if a user has a certificate store or web proxy configured, that would all be done in the config for the HTTP plugin and thus, you'd need that when making the token refreshes.
I was thinking about trying to make this reusable for other plugins that may use OAuth. The issue there is that while the flow is the same, there are a lot of pieces which are different. For example, I've been poking at a storage plugin for Google Sheets, which uses OAuth. While both use OAuth, the configuration is completely different and it unfortunately isn't really possible to reuse much in that situation.
The other thing to consider is that right now, the OAuth work only uses the `PlainCredentialProvider`, however users might want to store these tokens in Vault. I was thinking as a v2, to extend the functionality so that the tokens can be stored using the `VaultCredentialProvider`, but that will have to wait for another PR. Since some of these tokens are sensitive, it actually would make a lot of sense to store them more securely than in plain text in Zookeeper. ;-)
Regarding concurrency, let's say that two users execute a query at virtually the same time. Let's say that `access_token_A` is expired. What should happen is that the first query to hit that server will get a 401 error. Drill would then send a new request with the `refresh_token` and update the access token to `access_token_B`. My understanding of OAuth is that the tokens time out, but during that time, if userB requested a new access token using the same refresh_token, userB would get the same updated access token (`access_token_b`). In this situation, I think the worst case scenario would be that a few extra refresh requests are sent.
I think I may have mentioned this, but I'm also working on extending this a bit to allow for per-user tokens and credentials, so when that gets implemented, the chances of collisions happening is even less.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r788785459
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java
##########
@@ -180,6 +194,47 @@ public Response enablePlugin(@PathParam("name") String name, @PathParam("val") B
}
}
+ @GET
+ @Path("/storage/{name}/update_oath2_authtoken")
+ @Produces(MediaType.TEXT_HTML)
+ public Response updateAuthToken(@PathParam("name") String name, @QueryParam("code") String code) {
+ try {
+ if (storage.getPlugin(name).getConfig().getClass().getSimpleName().equalsIgnoreCase("HttpStoragePluginConfig")) {
Review comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2401: DRILL-8056: Add OAuth2 Support for HTTP Rest Plugin
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2401:
URL: https://github.com/apache/drill/pull/2401#discussion_r783252405
##########
File path: exec/jdbc-all/pom.xml
##########
@@ -562,7 +562,7 @@
This is likely due to you adding new dependencies to a java-exec and not updating the excludes in this module. This is important as it minimizes the size of the dependency of Drill application users.
</message>
- <maxsize>46600000</maxsize>
+ <maxsize>50000000</maxsize>
Review comment:
@vvysotskyi Thanks for looking at this. It actually is used in one of the files for Drill's internal rest API. (See below). Does that need to be included in the JDBC driver?
https://github.com/apache/drill/blob/d6c54936129f8e2a8eda83be7b1ab69998e893f8/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java#L206-L210
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org