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

filters/auth: add -oauth2-token-cookie-remove-subdomains flag #2203

Merged
merged 1 commit into from
Jan 20, 2023

Conversation

AlexanderYastrebov
Copy link
Member

See related #1909

Signed-off-by: Alexander Yastrebov [email protected]

Comment on lines +95 to +98
// TokenCookieRemoveSubdomains sets the number of subdomains to remove from
// the callback request hostname to obtain token cookie domain.
// Init converts default nil to 1.
TokenCookieRemoveSubdomains *int
Copy link
Member Author

Choose a reason for hiding this comment

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

To stay backwards-compatible I had to use pointer here to tell zero and default value (one) apart.
The proper config initialization requires Init() call.

If we can sacrifice a bit of compatibility for library users then we could use just int and rely on the default skipper flag value of 1.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are not many options here https://stackoverflow.com/questions/68800319/how-to-differentiate-int-null-and-defaulted-to-zero-from-int-actually-equal-to-z

We could also create a wrapper type but that feels too much.

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 need this 3 state: 0 value, 0, N, then ptr is the way to go

@AlexanderYastrebov AlexanderYastrebov force-pushed the filters/auth/grant-cookie-subdomains branch from 0b35759 to c2d64ab Compare January 18, 2023 17:25
@AlexanderYastrebov AlexanderYastrebov marked this pull request as ready for review January 19, 2023 16:52
@@ -214,14 +217,22 @@ func checkStatus(t *testing.T, rsp *http.Response, expectedStatus int) {
}
}

func checkRedirect(t *testing.T, rsp *http.Response, expectedURL string) {
func checkRedirect(t *testing.T, rsp *http.Response, expectedLocationWithoutQuery string) {
Copy link
Member

Choose a reason for hiding this comment

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

expectedURI maybe?

@szuecs
Copy link
Member

szuecs commented Jan 19, 2023

👍

1 similar comment
@AlexanderYastrebov
Copy link
Member Author

👍

@AlexanderYastrebov AlexanderYastrebov merged commit 35e0df7 into master Jan 20, 2023
@AlexanderYastrebov AlexanderYastrebov deleted the filters/auth/grant-cookie-subdomains branch January 20, 2023 10:09
AlexanderYastrebov added a commit that referenced this pull request Feb 1, 2024
Followup on #2203
Followup on #2244
Followup on #2457

Updates #2898

Signed-off-by: Alexander Yastrebov <[email protected]>
AlexanderYastrebov added a commit that referenced this pull request Feb 5, 2024
Followup on #2203
Followup on #2244
Followup on #2457

Updates #2898

Signed-off-by: Alexander Yastrebov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants