feat(cors): Added cross origin support #221

msohailhussain · 2020-04-30T17:06:13Z

Summary

Cross origin support added specifically when running from js.

TestPlan

Unit tests added that checks headers for preflight requests.

Issues

No Issues

msohailhussain · 2020-04-30T17:06:25Z

cmd/optimizely/main_test.go

@@ -94,6 +94,15 @@ func assertAPIAuth(t *testing.T, actual config.ServiceAuthConfig) {
 	assert.Equal(t, 25*time.Second, actual.JwksUpdateInterval)
 }

+func assertAPICORS(t *testing.T, actual config.CORSConfig) {
+	assert.Equal(t, []string{"http://test.com"}, actual.AllowedOrigins)


add one more domain.

codecov · 2020-04-30T17:10:33Z

Codecov Report

Merging #221 into master will increase coverage by 0.17%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #221      +/-   ##
==========================================
+ Coverage   82.38%   82.55%   +0.17%     
==========================================
  Files          27       27              
  Lines        1243     1261      +18     
==========================================
+ Hits         1024     1041      +17     
- Misses        164      165       +1     
  Partials       55       55

Impacted Files	Coverage Δ
pkg/handlers/notification.go	`82.81% <ø> (-0.27%)`	⬇️
config/config.go	`100.00% <100.00%> (ø)`
pkg/routers/api.go	`97.14% <100.00%> (+0.53%)`	⬆️
pkg/server/server.go	`90.00% <0.00%> (-1.43%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73c375d...ce89726. Read the comment docs.

pkg/routers/api_test.go

mikecdavis · 2020-05-01T18:53:38Z

pkg/routers/api_test.go

@@ -66,6 +78,17 @@ var testAuthMiddleware = func(next http.Handler) http.Handler {
 	return http.HandlerFunc(fn)
 }

+var testCorsHandler = cors.Handler(


Looks like we're testing the cors.Handler more than we're testing api.go. I think what we really need to test is that some handler was properly added where appropriate in the request path.

@mikecdavis handler is added for all requests. The only way to verify is to check things are coming back in the response or not.
Please suggest if you want unit tests other than this approach.

mikecdavis · 2020-05-01T19:00:56Z

config/config.go

+}
+
+// ToCorsOptions converts CORSConfig to cors middleware options
+func (c *CORSConfig) ToCorsOptions() cors.Options {


Nit, but I'd prefer we didn't add this here as it feels like we're leaking a dependency. I think having a method in api.go would be a cleaner abstraction.

func CreateCorsHandler(c conf.CORSConfig) func(next http.Handler) http.Handler { options := cors.Options{ AllowedOrigins: c.AllowedOrigins, AllowedMethods: c.AllowedMethods, AllowedHeaders: c.AllowedHeaders, ExposedHeaders: c.ExposedHeaders, AllowCredentials: c.AllowedCredentials, MaxAge: c.MaxAge, } return cors.Handler(options) }

mikecdavis · 2020-05-01T19:03:24Z

config/config.go

+				// If allowedOrigins is nil or empty, value is set to ["*"].
+				AllowedOrigins: nil,


Looks like this is defaulting to wide-open CORS support. We need to default to disabled then enable only if explicitly configured.

agree with this comment

Discussed offline, it's OK to comment it out and give an idea to the user, CORS is not enabled. It will work as expected without cors before implementing it how it was working.

mjc1283 · 2020-05-04T16:27:13Z

@msohailhussain @mikecdavis I suggest this setting be used in the Notifications handler, which is currently hard-coded to Access-Control-Allow-Origin: *.

msohailhussain · 2020-05-04T22:08:03Z

config.yaml

 ##
 ## admin service configuration
 ##
 admin:
    ## http listener port
    port: "8088"
-
+    


remove this line.

msohailhussain · 2020-05-04T22:26:31Z

#221 (comment)
I will take care of it @mjc1283

msohailhussain · 2020-05-05T17:04:46Z

@mjc1283 I added this commit, cca1309
AccessControl header is set in parent handler. And it's only useful for OPTION requests, not for all.

mikecdavis

One minor nit, else LGTM

mikecdavis · 2020-05-05T17:27:36Z

config/config.go

+				// If AllowedOrigins is nil or empty, value is set to ["*"].
+				AllowedOrigins: nil,
+				// If AllowedMethods is nil or empty, value is set to (HEAD, GET and POST).
+				AllowedMethods: []string{"HEAD", "GET", "PUT", "POST", "DELETE"},


Might as well default to nil or empty here rather than to extend the default.

Sure, will do.

msohailhussain and others added 4 commits April 29, 2020 14:45

cors config support

4f5cbdc

Merge branch 'master' into sohail/cors

94d86d7

additional code removed

eda4670

fixes and unit tests.

186139f

msohailhussain commented Apr 30, 2020

View reviewed changes

pkg/routers/api_test.go Show resolved Hide resolved

msohailhussain added 4 commits April 30, 2020 12:47

slightly changed

149b427

config tests

613b1a0

config tests

c991979

config test corrected

a479991

msohailhussain changed the title ~~Sohail/cors~~ feat(cors): Added cross origin support Apr 30, 2020

msohailhussain marked this pull request as ready for review April 30, 2020 22:07

msohailhussain requested review from mikecdavis and mikeproeng37 May 1, 2020 00:09

mikeproeng37 requested a review from a team May 1, 2020 00:37

mikecdavis suggested changes May 1, 2020

View reviewed changes

Recommended changes made.

59717fc

yasirfolio3 assigned yasirfolio3 and unassigned yasirfolio3 May 4, 2020

feedback addressed

8e5afb1

msohailhussain commented May 4, 2020

View reviewed changes

msohailhussain requested review from mikecdavis and pawels-optimizely May 4, 2020 22:27

yasirfolio3 and others added 2 commits May 5, 2020 10:48

nit fixed.

d0d7655

notification handler cors set outside

cca1309

mikecdavis approved these changes May 5, 2020

View reviewed changes

defaulted to allowed methods

ce89726

mjc1283 approved these changes May 5, 2020

View reviewed changes

mikeproeng37 merged commit c9751ee into master May 5, 2020

mikeproeng37 deleted the sohail/cors branch May 5, 2020 19:07

		// If allowedOrigins is nil or empty, value is set to ["*"].
		AllowedOrigins: nil,

feat(cors): Added cross origin support #221

feat(cors): Added cross origin support #221

Uh oh!

Conversation

msohailhussain commented Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

TestPlan

Issues

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjc1283 commented May 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msohailhussain commented May 4, 2020

Uh oh!

msohailhussain commented May 5, 2020

Uh oh!

mikecdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

msohailhussain commented Apr 30, 2020 •

edited

Loading

codecov bot commented Apr 30, 2020 •

edited

Loading