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

Jump to bottom

Access-Control-Allow-Credentials default value #10660

Merged
merged 1 commit into from
Jul 15, 2020
Merged

Access-Control-Allow-Credentials default value #10660

merged 1 commit into from
Jul 15, 2020

Conversation

mcserra
Copy link
Contributor

@mcserra mcserra commented Jul 12, 2020

resolves: #10447

gsmet
gsmet requested changes Jul 12, 2020
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Thanks! I'll let @sberyozkin check that one but I spotted a small issue.

docs/src/main/asciidoc/http-reference.adoc Outdated Show resolved Hide resolved
@sberyozkin
Copy link
Member

@mcserra Hi, thanks for the PR, can you please update one of the existing unit CORS test, https://github.com/quarkusio/quarkus/tree/master/extensions/vertx-http/deployment/src/test/java/io/quarkus/vertx/http/cors

@mcserra
Copy link
Contributor Author

mcserra commented Jul 13, 2020

@sberyozkin so do you think we should keep the boolean, not allowing a user to force the false header?

@sberyozkin
Copy link
Member

@mcserra,

so do you think we should keep the boolean, not allowing a user to force the false header?

I actually resolved my earlier comment (thus agreeing to keeping Optional<Boolean>) so please restore it :-). IMHO it will be ready to go in afterwards, thanks

@sberyozkin
Copy link
Member

sberyozkin commented Jul 14, 2020 •
edited
Loading

@mcserra Thanks, IMHO it is looking good now.
There was a related open-api comment from @ia3andy here.
@ia3andy, as far as Access-Control-Allow-Credentials and the open-api is concerned, what is the risk, on that path it is only about accessing an open-doc document ? I don't mind it being tightened there as well, but would like to understand the situation better.
Would it make sense to resolve #10447 with this PR and for you to open a dedicated issue for the open api update ?
@ia3andy can you also look at this PR please ?

Thanks, Sergey

gsmet
gsmet previously requested changes Jul 15, 2020
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Please squash all the commits into one before merging.

Thanks!

@sberyozkin
Copy link
Member

@mcserra, @ia3andy I've opened #10740 so that we can merge this PR and resolve #10447

@sberyozkin sberyozkin self-requested a review July 15, 2020 08:25
@sberyozkin
Copy link
Member

@mcserra Please squash as requested by Guillaume and we'll be ready to merge this one, thanks

@mcserra
Access-Control-Allow-Credentials default value
1dceff5 
Update docs/src/main/asciidoc/http-reference.adoc

Co-authored-by: Guillaume Smet <[email protected]>

config type + tests

change origin match rules

change config to Optional<Boolean>
@mcserra
Copy link
Contributor Author

mcserra commented Jul 15, 2020

Squashed. Thanks!

@sberyozkin sberyozkin dismissed gsmet’s stale review July 15, 2020 12:25

@mcserra squashed as requested

@sberyozkin sberyozkin added this to the 1.7.0 - master milestone Jul 15, 2020
@sberyozkin sberyozkin self-requested a review July 15, 2020 12:25
@sberyozkin
Copy link
Member

@mcserra Thanks, please investigate #10740 if the time allows, cheers

@sberyozkin sberyozkin merged commit 475b2b4 into quarkusio:master Jul 15, 2020
@mcserra
Copy link
Contributor Author

mcserra commented Jul 15, 2020 •
edited
Loading

I will, sure!

@ia3andy
Copy link
Contributor

ia3andy commented Jul 20, 2020

@mcserra Thanks, IMHO it is looking good now.
There was a related open-api comment from @ia3andy here.
@ia3andy, as far as Access-Control-Allow-Credentials and the open-api is concerned, what is the risk, on that path it is only about accessing an open-doc document ? I don't mind it being tightened there as well, but would like to understand the situation better.
Would it make sense to resolve #10447 with this PR and for you to open a dedicated issue for the open api update ?
@ia3andy can you also look at this PR please ?

Thanks, Sergey

@sberyozkin sorry was in PTO last week.. this PR looks fine.

Regarding OpenApi, even if this is only about accessing an open-doc document, there is a risk for cookies.. I think we should open another issue for it..

@ia3andy
Copy link
Contributor

ia3andy commented Jul 20, 2020

ok I missed the issue.. thanks! :) #10740

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gsmet gsmet gsmet left review comments

@sberyozkin sberyozkin Awaiting requested review from sberyozkin

Assignees
No one assigned
Labels
area/documentation area/vertx
Projects
None yet
Milestone
1.7.0.CR1
4 participants
@mcserra @sberyozkin @ia3andy @gsmet