Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow cors spec to be defined as regexps #723

Merged
merged 4 commits into from Aug 11, 2016

Conversation

@matteosuppo
Copy link
Contributor

commented Aug 10, 2016

If the string passed into Origin() begins and ends with a "/"
it's considered a regular expression.

For example "/[api|swagger].goa.design/" can match both
api.goa.design and swagger.goa.design

Fix #718

Signed-off-by: Matteo Suppo matteo.suppo@gmail.com

Allow cors spec to be defined as regexps
If the string passed into Origin() begins and ends with a "/"
it's considered a regular expression.

For example "/[api|swagger].goa.design/" can match both
api.goa.design and swagger.goa.design

Signed-off-by: Matteo Suppo <matteo.suppo@gmail.com>
@raphael

This comment has been minimized.

Copy link
Member

commented Aug 10, 2016

Thank you, this is a great PR! I'm wondering though if there's a way we could avoid having to recompile the regexp on every request. Maybe we could introduce a MatchOriginRegexp function in the cors package that accepts a regular expression for the origin parameter. The generated code would compile the regexp once when mounting the action.

@matteosuppo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2016

Sounds reasonable

@matteosuppo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2016

It could generate something like:

func handleBoardsOrigin(h goa.Handler) goa.Handler {
    spec := regexp.MustCompile("[api|swagger].goa.design")
    return func(ctx context.Context, rw http.ResponseWriter, req *http.Request) error {
        origin := req.Header.Get("Origin")
        if origin == "" {
            // Not a CORS request
            return h(ctx, rw, req)
        }
        if cors.MatchOriginRegexp(origin, spec) {
            ctx = goa.WithLogContext(ctx, "origin", origin)
            rw.Header().Set("Access-Control-Allow-Origin", origin)
            rw.Header().Set("Access-Control-Allow-Credentials", "true")
            if acrm := req.Header.Get("Access-Control-Request-Method"); acrm != "" {
                // We are handling a preflight request
                rw.Header().Set("Access-Control-Allow-Methods", "GET, PUT, POST, DELETE")
                rw.Header().Set("Access-Control-Allow-Headers", "Authorization, Origin, X-Requested-With, Content-Type, Accept")
            }
            return h(ctx, rw, req)
        }

        return h(ctx, rw, req)
    }
}
Compile the cors regexp only once
The generated code compiles the cors spec when mounting the
controller, instead of compiling it on every request.

Requires a new Field on CORSDefinition, but it should remain
compatible. Adds a new function on the cors package, which may be
useless.

Signed-off-by: Matteo Suppo <matteo.suppo@gmail.com>
@matteosuppo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2016

Tomorrow I'll fix the tests

@raphael

This comment has been minimized.

Copy link
Member

commented Aug 10, 2016

Great, thanks. Generated code snippet looks good.

@raphael

This comment has been minimized.

Copy link
Member

commented Aug 10, 2016

Bonus points for adding a test for the generated code as well :)

matteosuppo added some commits Aug 11, 2016

Add test cases for regexp origin cors
Signed-off-by: Matteo Suppo <matteo.suppo@gmail.com>
Move validation of regex origin into CORSDefinition.Validate
Signed-off-by: Matteo Suppo <matteo.suppo@gmail.com>
@raphael

This comment has been minimized.

Copy link
Member

commented Aug 11, 2016

Awesome PR, thank you!

@raphael raphael merged commit ef5b1da into goadesign:master Aug 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.