✨ feat(cors): Added new 'AllowOriginsFunc' function.

[Jamess-Lucass]

Description

Adds AllowOriginsFunc function to cors config which adds the ability to dynamically chose whether to allow the origin via CORS or not.

Fixes #2390

Type of change

• New feature (non-breaking change which adds functionality)
• This change requires a documentation update

Checklist:

• For new functionalities I follow the inspiration of the express js framework and built them similar in usage
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation - /docs/ directory for https://docs.gofiber.io/
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• If new dependencies exist, I have checked that they are really necessary and agreed with the maintainers/community (we want to have as few dependencies as possible)
• I tried to make my code as fast as possible with as few allocations as possible

Commit formatting:

Use emojis on commit messages so it provides an easy way of identifying the purpose or intention of a commit. Check out the emoji cheatsheet here: https://gitmoji.carloscuesta.me/

[welcome]

Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

[leonklingele]

We might want to issue a warning to stderr when both config.AllowOrigins and the new config. AllowOriginsFunc are set.

[ReneWerner87]

@Jamess-Lucass can you do the suggested change ? (use the log package https://pkg.go.dev/log with "[Warning] ")
https://github.com/search?q=repo%3Agofiber%2Ffiber+%5BWarning%5D&type=code

[ReneWerner87]

https://github.com/search?q=repo%3Agofiber%2Ffiber%20log.Printf(%22%5B&type=code

maybe with the middelware name in uppercase
"[CORS] - [Warning] ...."

[Jamess-Lucass]

Yeah I can add a warning log when both are set. I will add that I think it's a valid scenario for both to be set, eg.

app.Use(cors.New(cors.Config{
    AllowCredentials: true,
    AllowOrigins: "https://my-domain.com", // for production
    AllowOriginsFunc: func(origin string) bool {
        return os.Getenv("ENVIRONMENT") == "development" // in development allow CORS from any origin
    }
}))

or even

cfg := cors.Config{
    AllowCredentials: true,
    AllowOrigins: "https://my-domain.com", // for production
}

if os.Getenv("ENVIRONMENT") == "development" {
    cfg.AllowOriginsFunc = func(origin string) bool {
        // some logic
    },
}
  
app.Use(cors.New(cfg))

As long as people are aware this log does not indicate there is a miss configuration on their part and they do not need to resolve anything then I am happy.

[ReneWerner87]

@Jamess-Lucass the idea was to allow both but to give the user the information to choose one or the other

with express only one setting is possible at the same time, because it is on the same property
https://expressjs.com/en/resources/middleware/cors.html#configuration-options

do you think it makes sense to use both at the same time ?
know that it is also possible with gin, but the question is whether it is useful

[Jamess-Lucass]

@Jamess-Lucass the idea was to allow both but to give the user the information to choose one or the other

with express only one setting is possible at the same time, because it is on the same property https://expressjs.com/en/resources/middleware/cors.html#configuration-options

do you think it makes sense to use both at the same time ? know that it is also possible with gin, but the question is whether it is useful

Ahh okay I see, I personally would be setting both values but if we encourage users to just set one of them the above code block example would then look like.

cfg := cors.Config{
    AllowCredentials: true    
}

if os.Getenv("ENVIRONMENT") == "development" {
    cfg.AllowOriginsFunc = func(origin string) bool {
        // some logic
    },
}

if os.Getenv("ENVIRONMENT") == "production" {
    cfg.AllowOrigins = "https://my-domain.com", // for production
}
  
app.Use(cors.New(cfg))

I think either approach is fine. At the moment if it finds a match for AllowOrigins it won't then run AllowOriginsFunc.

Whatever you think is best for developer experience

[ReneWerner87]

since we are oriented to express and one would achieve both with the functionality of the function

I think that the current warning message is okay

what do you think @leonklingele @efectn

[jub0bs]

See my comment on issue #2390. I really think you should reconsider the addition of this feature...

[efectn]

I'm OK to the PR. But i'd recommend to add a description to avoid using this feature in production. It can be added to config and docs

[Jamess-Lucass]

I'm OK to the PR. But i'd recommend to add a description to avoid using this feature in production. It can be added to config and docs

See most recent commit, I updated docs with a notice about discouraging the use of this in production. Just let me know if any of the wording wants to be changed at all 👍

[welcome]

Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord