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

feat:add opt CORS #92

Merged
merged 2 commits into from
Mar 6, 2024
Merged

feat:add opt CORS #92

merged 2 commits into from
Mar 6, 2024

Conversation

Marsyew
Copy link
Contributor

@Marsyew Marsyew commented Mar 3, 2024

No description provided.

@a-wing
Copy link
Member

a-wing commented Mar 4, 2024

Maybe need add a example in config-dist.toml

@a-wing
Copy link
Member

a-wing commented Mar 5, 2024

I have a good idea

We can design look like this, Because CORS belong http, So:

[http]
listen = "[::]:7777"

# I think default should disable
core = false

@a-wing
Copy link
Member

a-wing commented Mar 5, 2024

#78

Copy link
Member

@a-wing a-wing left a comment

Choose a reason for hiding this comment

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

I think, last version is more good

config-dist.toml Outdated
@@ -30,3 +30,16 @@ urls = [
# Default: info
# Values: off, error, warn, info, debug, trace
# level = "warn"

[http]
Copy link
Member

Choose a reason for hiding this comment

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

Should only one [http] block

Copy link
Member

Choose a reason for hiding this comment

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

This config begin is a listen right?

Actually, this listen and cors belong [http]

We need changed config, add a [http] block, and move listen to [http], after add core to http

config-dist.toml Outdated

[http]
listen = "[::]:7777"
cors = false
Copy link
Member

Choose a reason for hiding this comment

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

If you want to describe core item

You can describe:

# Cross-Origin Resource Sharing (CORS)
# reference: https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS
cors = false

config-dist.toml Outdated
listen = "[::]:7777"
cors = false

#[http]
Copy link
Member

Choose a reason for hiding this comment

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

Remove this

src/config.rs Outdated
@@ -16,6 +18,7 @@ pub struct Config {
pub auth: Auth,
#[serde(default = "default_log")]
pub log: Log,
pub enable_cors: bool,
Copy link
Member

Choose a reason for hiding this comment

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

you move to core in http block, this field name is changed

src/config.rs Outdated
@@ -157,6 +160,7 @@ impl Config {
listen: default_listen(),
auth: Default::default(),
log: default_log(),
enable_cors: true,
Copy link
Member

Choose a reason for hiding this comment

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

default false

Copy link
Member

Choose a reason for hiding this comment

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

field name is changed

src/config.rs Outdated


pub fn cors_layer() -> CorsLayer {
let config_cors = fs::read_to_string("config-dist.toml")
Copy link
Member

Choose a reason for hiding this comment

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

What this? read config-dist.toml should only read one time

Copy link
Member

Choose a reason for hiding this comment

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

cors_layer() should a params, enable: boolean

src/config.rs Outdated
eprintln!("Unable to read config-dist.toml");
String::from(r#"
[http]
listen = "[::]:7777"
Copy link
Member

Choose a reason for hiding this comment

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

Why together display listen error and cors

Copy link
Member

@a-wing a-wing left a comment

Choose a reason for hiding this comment

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

Config struct should changed

look like:

pub struct Http {
    pub listen: String,
    pub core: Boolean,
}

config-dist.toml Outdated
@@ -30,3 +30,16 @@ urls = [
# Default: info
# Values: off, error, warn, info, debug, trace
# level = "warn"

[http]
Copy link
Member

Choose a reason for hiding this comment

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

This config begin is a listen right?

Actually, this listen and cors belong [http]

We need changed config, add a [http] block, and move listen to [http], after add core to http

src/config.rs Outdated


pub fn cors_layer() -> CorsLayer {
let config_cors = fs::read_to_string("config-dist.toml")
Copy link
Member

Choose a reason for hiding this comment

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

cors_layer() should a params, enable: boolean

src/main.rs Outdated
@@ -79,6 +79,7 @@ async fn main() {
config: cfg.clone(),
};
let auth_layer = ValidateRequestHeaderLayer::custom(ManyValidate::new(cfg.auth));
let cors = cors_layer();
Copy link
Member

Choose a reason for hiding this comment

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

You should cors enable in here, For example cfg.http.core

@a-wing a-wing merged commit 2850d64 into binbat:main Mar 6, 2024
4 checks passed
@a-wing a-wing added enhancement New feature or request mod::live777 Live777 server labels Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request mod::live777 Live777 server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants