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.

Sign up for GitHub

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

feat: Add allowed origins config #1408

Merged
merged 5 commits into from
Apr 27, 2023
Merged

feat: Add allowed origins config #1408

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
329a86c
add AllowedOrigins to API congif
fredcarle Apr 23, 2023
bedeea6
add config file and CLI option
fredcarle Apr 26, 2023
b7079b9
change to retrictive default
fredcarle Apr 26, 2023
11fbf72
add readme doc for CORS
fredcarle Apr 26, 2023
78e3de1
change to allowed-origins
fredcarle Apr 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,19 @@ Note: `sudo` is needed above for the redirection server (to bind port 80).

A valid email address is necessary for the creation of the certificate, and is important to get notifications from the Certificate Authority - in case the certificate is about to expire, etc.

## Supporting CORS

When accessing DefraDB through a frontend interface, you may be confronted with a CORS error. That is because, by default, DefraDB will not have any allowed origins set. To specify which origins should be allowed to access your DefraDB endpoint, you can specify them when starting the database:
```shell
defradb start --allowedorigins=https://yourdomain.com
Copy link
Member

@shahzadlone shahzadlone Apr 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion:

How about just origins. Mostly because we don't have a blockedorigins or any other option like that. Also allowedorigins is less readable, perhaps allowedOrigins if you want to stick with the "allowed" word.

Suggested change
defradb start --allowedorigins=https://yourdomain.com
defradb start --origins=https://yourdomain.com

Copy link
Collaborator Author

@fredcarle fredcarle Apr 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing with --origins is that it's a little vague and the http header is called With-Allowed-Origins. I feel like it makes more sense to have almost the full representation here as well. allowedOrigins could be nicer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: I have a very minor preference for allowed-origins, but only if it we dont already camel case any other existing multi-word params/flags

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like allowed-origins, would prefer that over allowedOrigins

Copy link
Contributor

@orpheuslummis orpheuslummis Apr 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one idea is corsdomain, and perhaps alternatively using --api.corsdomain or http.corsdomain depending on which top-level key we want to represent the http system.

when determining defradb's parameters, we have to keep in mind that they have three points of determination: environment variables, config file, CLI command.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather we stay closer to what it represents. allowed-origins is quite good.

Copy link
Contributor

@orpheuslummis orpheuslummis Apr 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the fully qualified key from the config's perspective would be api.allowed-origins, so perhaps it can be --allowed-origins in the CLI but as an env. variable it would be DEFRA_API_ALLOWED_ORIGINS perhaps. i'm not sure yet about if it's necessary to quote such a key with a - in Yaml - it might have to be

[api]
"allowed-origins": ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is according to what I see here https://www.cloudbees.com/blog/yaml-tutorial-everything-you-need-get-started

```

If running a frontend app locally on localhost, allowed origins must be set with the port of the app:
```shell
defradb start --allowedorigins=http://localhost:3000
```

The catch-all `*` is also a valid origin.

## Community

Expand Down
10 changes: 10 additions & 0 deletions cli/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,15 @@ func MakeStartCommand(cfg *config.Config) *cobra.Command {
log.FeedbackFatalE(context.Background(), "Could not bind api.tls", err)
}

cmd.Flags().StringArray(
"allowedorigins", cfg.API.AllowedOrigins,
"List of origins to allow for CORS requests",
)
err = cfg.BindFlag("api.allowedorigins", cmd.Flags().Lookup("allowedorigins"))
if err != nil {
log.FeedbackFatalE(context.Background(), "Could not bind api.allowedorigins", err)
}

cmd.Flags().String(
"pubkeypath", cfg.API.PubKeyPath,
"Path to the public key for tls",
Expand Down Expand Up @@ -319,6 +328,7 @@ func start(ctx context.Context, cfg *config.Config) (*defraInstance, error) {
sOpt := []func(*httpapi.Server){
httpapi.WithAddress(cfg.API.Address),
httpapi.WithRootDir(cfg.Rootdir),
httpapi.WithAllowedOrigins(cfg.API.AllowedOrigins...),
}

if n != nil {
Expand Down
22 changes: 12 additions & 10 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,20 +287,22 @@ func (dbcfg DatastoreConfig) validate() error {

// APIConfig configures the API endpoints.
type APIConfig struct {
Address string
TLS bool
PubKeyPath string
PrivKeyPath string
Email string
Address string
TLS bool
AllowedOrigins []string
PubKeyPath string
PrivKeyPath string
Email string
}

func defaultAPIConfig() *APIConfig {
return &APIConfig{
Address: "localhost:9181",
TLS: false,
PubKeyPath: "certs/server.key",
PrivKeyPath: "certs/server.crt",
Email: DefaultAPIEmail,
Address: "localhost:9181",
TLS: false,
AllowedOrigins: []string{},
PubKeyPath: "certs/server.key",
PrivKeyPath: "certs/server.crt",
Email: DefaultAPIEmail,
}
}

Expand Down
2 changes: 2 additions & 0 deletions config/configfile_yaml.gotmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ api:
address: {{ .API.Address }}
# Whether the API server should listen over HTTPS
tls: {{ .API.TLS }}
# The list of origins a cross-domain request can be executed from.
# allowedorigins: {{ .API.AllowedOrigins }}
# The path to the public key file. Ignored if domains is set.
pubkeypath: {{ .API.PubKeyPath }}
# The path to the private key file. Ignored if domains is set.
Expand Down