Add HTTP Proxy support #233

VitaliiTytarenko-okta · 2020-12-23T21:57:32Z

bdemers

IMHO, both the “standard” Java proxy props should work, and the ones used by the Okta SDK.

that way a user only has to specify the props once 🤔

arvindkrishnakumar-okta · 2020-12-28T17:58:53Z

IMHO, both the “standard” Java proxy props should work, and the ones used by the Okta SDK.

that way a user only has to specify the props once 🤔

@VitaliiTytarenko-okta You might want to check out https://github.com/okta/okta-spring-boot#proxy to see if it helps. In theory, setting these JVM properties should work.

VitaliiTytarenko-okta · 2021-01-12T20:20:40Z

IMHO, both the “standard” Java proxy props should work, and the ones used by the Okta SDK.

that way a user only has to specify the props once 🤔

As I mentioned here #57, I think, it's a bad idea to mix these properties.

bdemers · 2021-01-12T21:17:36Z

We should standardize on okta.oauth2.proxy.* That makes it mirror what is used for the SDKs

Re: the property names, other libraries use the Java proxy properties or directly use the ProxySelector (IIRC OK HTTP uses the ProxySelector).

HttpClient also has this implementation: https://hc.apache.org/httpcomponents-client-ga/httpclient/apidocs/org/apache/http/impl/client/SystemDefaultHttpClient.html

VitaliiTytarenko-okta · 2021-01-15T16:46:59Z

@bdemers
Could you take a look, please?
I changed property names. Have I missed something else?

oauth2/pom.xml

bdemers · 2021-01-15T20:39:54Z

oauth2/src/main/java/com/okta/spring/boot/oauth/OktaOAuth2AutoConfig.java

+                Authenticator.setDefault(new ProxyPasswordAuthentication(proxyProperties.getUsername(),
+                    proxyProperties.getPassword().toCharArray()));


Don't set the default authenticator, this needs to be scoped to this RestTemplate instance

bdemers · 2021-01-15T20:46:20Z

oauth2/src/main/java/com/okta/spring/boot/oauth/config/OktaOAuth2Properties.java

+        public void setHost(String host) {
+            this.host = host;
+        }
+
+        public int getPort() {
+            return port;
+        }
+
+        public void setPort(int port) {
+            this.port = port;
+        }


nit: consider validating the Proxy object above you have an if statement that both host and port either both required or both should be missing, but there is no validation, if one of them is missing

bdemers · 2021-01-15T20:46:51Z

oauth2/src/test/groovy/com/okta/spring/boot/oauth/OktaOAuth2AutoConfigRestTemplateTest.groovy

@@ -0,0 +1,108 @@
+/*
+ * Copyright 2020-Present Okta, Inc.


Suggested change

* Copyright 2020-Present Okta, Inc.

* Copyright 2021-Present Okta, Inc.

Check other files added in this commit

bdemers · 2021-01-15T20:48:04Z

oauth2/src/test/groovy/com/okta/spring/boot/oauth/OktaOAuth2AutoConfigRestTemplateTest.groovy

+        assertThat "Wrong user", Authenticator.theAuthenticator.getAt("proxyUser"), is("foo")
+        assertThat "Wrong password", Authenticator.theAuthenticator.getAt("proxyPassword").toString(), is("bar")


nit: add the word proxy here

bdemers · 2021-01-15T20:49:35Z

oauth2/src/test/groovy/com/okta/spring/boot/oauth/OktaOAuth2AutoConfigRestTemplateTest.groovy

+
+class OktaOAuth2AutoConfigRestTemplateTest {
+    private static final String LOCALHOST = "localhost"
+    private static final int PORT = 7000


Don't use a static port for testing, use a random free port, this mockserver should support this out of the box, but if not, just lookup a free port first

bdemers · 2021-01-15T20:52:06Z

oauth2/src/test/groovy/com/okta/spring/boot/oauth/OktaOAuth2AutoConfigRestTemplateTest.groovy

+        RestTemplate restTemplate = new OktaOAuth2AutoConfig().restTemplate(properties)
+        def headers = new HttpHeaders(singletonMap("Cookie", "sessionId=" + sessionId))
+        ResponseEntity<OAuth2AccessTokenResponse> response = restTemplate
+            .exchange("http://base_url.com", HttpMethod.GET, new HttpEntity<String>(headers), OAuth2AccessTokenResponse)


use example.com

arvindkrishnakumar-okta · 2021-01-19T18:05:54Z

README.md

-Dhttps.proxyPort=port
-Dhttps.proxyUser="user"          # optional
-Dhttps.proxyPassword="password". # optional 
+-Dokta.oauth2.proxy.host=host


Please add username and password properties here.

arvindkrishnakumar-okta · 2021-01-19T18:06:20Z

README.md

 ```

 or, you could set it programmatically like:

 ```java
-System.setProperty("https.proxyHost", "https://example-proxy.com");
-System.setProperty("https.proxyPort", "443");
+System.setProperty("okta.oauth2.proxy.host", "example-proxy.com");


same as above comment.

bdemers · 2021-01-19T21:00:30Z

README.md

+okta:
+  oauth2:
+    proxy:
+      host: "example-proxy.com"


use proxy.example.com (when possible use example.com as this is the intended use for this domain)

README.md

bdemers · 2021-01-20T04:40:10Z

README.md

+-Dokta.oauth2.proxy.username=username
+-Dokta.oauth2.proxy.password=password


Suggested change

-Dokta.oauth2.proxy.username=username

-Dokta.oauth2.proxy.password=password

-Dokta.oauth2.proxy.username=your-username # optional

-Dokta.oauth2.proxy.password=your-secret-password # optional

bdemers · 2021-01-20T04:40:19Z

README.md

+System.setProperty("okta.oauth2.proxy.host", "proxy.example.com");
+System.setProperty("okta.oauth2.proxy.port", "7000");
+System.setProperty("okta.oauth2.proxy.username", "username");
+System.setProperty("okta.oauth2.proxy.password", "password");


see above suggestion

VitaliiTytarenko-okta · 2021-01-21T16:12:18Z

@bdemers @arvindkrishnakumar-okta
Done

arvindkrishnakumar-okta

lgtm

bdemers · 2021-01-29T22:40:35Z

oauth2/src/main/java/com/okta/spring/boot/oauth/OktaOAuth2AutoConfig.java

+
+        OktaOAuth2Properties.Proxy proxyProperties = oktaOAuth2Properties.getProxy();
+        Optional<BasicAuthenticationInterceptor> basicAuthenticationInterceptor = Optional.empty();
+        if (proxyProperties != null && Strings.hasText(proxyProperties.getHost()) && proxyProperties.getPort() != 0) {


nit

Suggested change

if (proxyProperties != null && Strings.hasText(proxyProperties.getHost()) && proxyProperties.getPort() != 0) {

if (proxyProperties != null && Strings.hasText(proxyProperties.getHost()) && proxyProperties.getPort() > 0) {

bdemers · 2021-01-29T22:43:20Z

oauth2/src/main/java/com/okta/spring/boot/oauth/OktaOAuth2ResourceServerAutoConfig.java

        NimbusJwtDecoder decoder = builder.build();
        decoder.setJwtValidator(TokenUtil.jwtValidator(oktaOAuth2Properties.getIssuer(), oktaOAuth2Properties.getAudience()));
        return decoder;
    }

-    private RestTemplate restTemplate() {
+    private RestTemplate restTemplate(OktaOAuth2Properties oktaOAuth2Properties) {


Remove the duplicate code from: OktaOidcUserService

bdemers

After removing the duplicate code, it looks like it's ready to go

Looks good

VitaliiTytarenko-okta requested review from arvindkrishnakumar-okta, sergiishamrai-okta and bdemers December 23, 2020 21:57

bdemers reviewed Dec 24, 2020

View reviewed changes

VitaliiTytarenko-okta mentioned this pull request Jan 12, 2021

Add doc example for setting HTTP proxy config #57

Closed

VitaliiTytarenko-okta force-pushed the add-proxy_support branch from 217f87a to 53d736b Compare January 13, 2021 16:19

bdemers suggested changes Jan 15, 2021

View reviewed changes

arvindkrishnakumar-okta reviewed Jan 19, 2021

View reviewed changes

bdemers reviewed Jan 19, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

bdemers reviewed Jan 20, 2021

View reviewed changes

VitaliiTytarenko-okta force-pushed the add-proxy_support branch from 9472f33 to ba8f17c Compare January 21, 2021 15:53

arvindkrishnakumar-okta approved these changes Jan 29, 2021

View reviewed changes

arvindkrishnakumar-okta self-requested a review January 29, 2021 21:41

arvindkrishnakumar-okta approved these changes Jan 29, 2021

View reviewed changes

bdemers reviewed Jan 29, 2021

View reviewed changes

bdemers previously requested changes Jan 29, 2021

View reviewed changes

arvindkrishnakumar-okta added 5 commits February 1, 2021 21:40

Allow proxy configuration via okta properties

0f912ad

cleanup unused imports

b058126

add username and password to proxy config

716c60b

add conditional on resttemplate bean

b684857

refactored to address findbug issue

ad4644e

arvindkrishnakumar-okta and others added 13 commits February 1, 2021 21:40

refactoring done

886c088

add test for RestTemplate with Proxy

17e9a60

update README.md

4f73ad0

add license header

11e9920

Allow proxy configuration via okta properties

2bfd0ff

add username and password to proxy config

67f3c97

add conditional on resttemplate bean

52368ce

refactored to address findbug issue

521e196

refactoring

4ef1fbc

Avoid unused import

b87cecd

changes as per code review

c2750be

update README.md according to code review

9cf77c0

Remove the duplicate code

683050c

VitaliiTytarenko-okta force-pushed the add-proxy_support branch 2 times, most recently from 143d403 to 9742f52 Compare February 4, 2021 01:50

not expose RestTemplate bean

aa2b8a0

VitaliiTytarenko-okta force-pushed the add-proxy_support branch from 9742f52 to aa2b8a0 Compare February 4, 2021 02:37

VitaliiTytarenko-okta merged commit 060c14b into master Feb 4, 2021

VitaliiTytarenko-okta deleted the add-proxy_support branch February 4, 2021 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HTTP Proxy support #233

Add HTTP Proxy support #233

VitaliiTytarenko-okta commented Dec 23, 2020 •

edited by arvindkrishnakumar-okta

Loading

bdemers left a comment

arvindkrishnakumar-okta commented Dec 28, 2020 •

edited

Loading

VitaliiTytarenko-okta commented Jan 12, 2021

bdemers commented Jan 12, 2021 •

edited

Loading

VitaliiTytarenko-okta commented Jan 15, 2021

bdemers Jan 15, 2021

VitaliiTytarenko-okta Jan 19, 2021

bdemers Jan 15, 2021

bdemers Jan 15, 2021

VitaliiTytarenko-okta Jan 19, 2021

bdemers Jan 15, 2021

bdemers Jan 15, 2021

VitaliiTytarenko-okta Jan 19, 2021

bdemers Jan 15, 2021

VitaliiTytarenko-okta Jan 19, 2021

arvindkrishnakumar-okta Jan 19, 2021

VitaliiTytarenko-okta Jan 19, 2021

arvindkrishnakumar-okta Jan 19, 2021

VitaliiTytarenko-okta Jan 19, 2021

bdemers Jan 19, 2021

bdemers Jan 20, 2021

bdemers Jan 20, 2021

VitaliiTytarenko-okta commented Jan 21, 2021

arvindkrishnakumar-okta left a comment

bdemers Jan 29, 2021

bdemers Jan 29, 2021

VitaliiTytarenko-okta Feb 1, 2021

bdemers left a comment

		Authenticator.setDefault(new ProxyPasswordAuthentication(proxyProperties.getUsername(),
		proxyProperties.getPassword().toCharArray()));

	* Copyright 2020-Present Okta, Inc.
	* Copyright 2021-Present Okta, Inc.

		assertThat "Wrong user", Authenticator.theAuthenticator.getAt("proxyUser"), is("foo")
		assertThat "Wrong password", Authenticator.theAuthenticator.getAt("proxyPassword").toString(), is("bar")

		-Dokta.oauth2.proxy.username=username
		-Dokta.oauth2.proxy.password=password

	if (proxyProperties != null && Strings.hasText(proxyProperties.getHost()) && proxyProperties.getPort() != 0) {
	if (proxyProperties != null && Strings.hasText(proxyProperties.getHost()) && proxyProperties.getPort() > 0) {

Add HTTP Proxy support #233

Add HTTP Proxy support #233

Conversation

VitaliiTytarenko-okta commented Dec 23, 2020 • edited by arvindkrishnakumar-okta Loading

bdemers left a comment

Choose a reason for hiding this comment

arvindkrishnakumar-okta commented Dec 28, 2020 • edited Loading

VitaliiTytarenko-okta commented Jan 12, 2021

bdemers commented Jan 12, 2021 • edited Loading

VitaliiTytarenko-okta commented Jan 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VitaliiTytarenko-okta commented Jan 21, 2021

arvindkrishnakumar-okta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdemers left a comment

Choose a reason for hiding this comment

VitaliiTytarenko-okta commented Dec 23, 2020 •

edited by arvindkrishnakumar-okta

Loading

arvindkrishnakumar-okta commented Dec 28, 2020 •

edited

Loading

bdemers commented Jan 12, 2021 •

edited

Loading