Skip to content

Commit a33b999

Browse files
authored
Deprecate camelCase parameters used by SAML APIs (#73984) (#74464)
Both SAML complete logout and SAML invalidate session APIs use a camelCase request parameter, queryString, while the convention is to use snake_case parameters. This PR deprecates queryString and replaces it with query_string. It is an error to if a request specifies both of them.
1 parent 2de1e7b commit a33b999

File tree

8 files changed

+61
-14
lines changed

8 files changed

+61
-14
lines changed

x-pack/docs/en/rest-api/security/saml-complete-logout-api.asciidoc

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,10 +46,14 @@ clients. See also <<security-api-saml-authenticate,SAML authenticate API>>,
4646
(Required, array) A json array with all the valid SAML Request Ids that the caller of
4747
the API has for the current user.
4848

49-
`queryString`::
49+
`query_string`::
5050
(Optional, string) If the SAML IdP sends the logout response with the HTTP-Redirect
5151
binding, this field must be set to the query string of the redirect URI.
5252

53+
`queryString`::
54+
deprecated:[7.14.0, "Use query_string instead"]
55+
See `query_string`
56+
5357
`content`::
5458
(Optional, string) If the SAML IdP sends the logout response with the HTTP-Post
5559
binding, this field must be set to the value of the `SAMLResponse` form parameter
@@ -67,7 +71,7 @@ POST /_security/saml/complete_logout
6771
{
6872
"realm": "saml1",
6973
"ids": [ "_1c368075e0b3..." ],
70-
"queryString": "SAMLResponse=fZHLasMwEEVbfb1bf...&SigAlg=http%3A%2F%2Fwww.w3.org%2F2000%2F09%2Fxmldsig%23rsa-sha1&Signature=CuCmFn%2BLqnaZGZJqK..."
74+
"query_string": "SAMLResponse=fZHLasMwEEVbfb1bf...&SigAlg=http%3A%2F%2Fwww.w3.org%2F2000%2F09%2Fxmldsig%23rsa-sha1&Signature=CuCmFn%2BLqnaZGZJqK..."
7175
}
7276
--------------------------------------------------
7377
// TEST[skip:can't test this without a valid SAML Logout Response]

x-pack/docs/en/rest-api/security/saml-invalidate-api.asciidoc

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,16 +40,20 @@ clients. See also <<security-api-saml-authenticate,SAML authenticate API>>,
4040
(Optional, string) The Assertion Consumer Service URL that matches the one of the SAML
4141
realm in {es} that should be used. You must specify either this parameter or the `realm` parameter.
4242

43-
`queryString`::
43+
`query_string`::
4444
(Required, string) The query part of the URL that the user was redirected to by the SAML
4545
IdP to initiate the Single Logout. This query should include a single
4646
parameter named `SAMLRequest` that contains a SAML logout request that is
4747
deflated and Base64 encoded. If the SAML IdP has signed the logout request,
4848
the URL should include two extra parameters named `SigAlg` and `Signature`
4949
that contain the algorithm used for the signature and the signature value itself.
50-
In order for {es} to be able to verify the IdP's signature, the value of the queryString field must be an exact match to the string provided by the browser.
50+
In order for {es} to be able to verify the IdP's signature, the value of the query_string field must be an exact match to the string provided by the browser.
5151
The client application must not attempt to parse or process the string in any way.
5252

53+
`queryString`::
54+
deprecated:[7.14.0, "Use query_string instead"]
55+
See `query_string`.
56+
5357
`realm`::
5458
(Optional, string) The name of the SAML realm in {es} the configuration. You must specify
5559
either this parameter or the `acs` parameter.
@@ -78,7 +82,7 @@ the user that is identified in the SAML Logout Request:
7882
--------------------------------------------------
7983
POST /_security/saml/invalidate
8084
{
81-
"queryString" : "SAMLRequest=nZFda4MwFIb%2FiuS%2BmviRpqFaClKQdbvo2g12M2KMraCJ9cRR9utnW4Wyi13sMie873MeznJ1aWrnS3VQGR0j4mLkKC1NUeljjA77zYyhVbIE0dR%2By7fmaHq7U%2BdegXWGpAZ%2B%2F4pR32luBFTAtWgUcCv56%2Fp5y30X87Yz1khTIycdgpUW9kY7WdsC9zxoXTvMvWuVV98YyMnSGH2SYE5pwALBIr9QKiwDGpW0oGVUznGeMyJZKFkQ4jBf5HnhUymjIhzCAL3KNFihbYx8TBYzzGaY7EnIyZwHzCWMfiDnbRIftkSjJr%2BFu0e9v%2B0EgOquRiiZjKpiVFp6j50T4WXoyNJ%2FEWC9fdqc1t%2F1%2B2F3aUpjzhPiXpqMz1%2FHSn4A&SigAlg=http%3A%2F%2Fwww.w3.org%2F2001%2F04%2Fxmldsig-more%23rsa-sha256&Signature=MsAYz2NFdovMG2mXf6TSpu5vlQQyEJAg%2B4KCwBqJTmrb3yGXKUtIgvjqf88eCAK32v3eN8vupjPC8LglYmke1ZnjK0%2FKxzkvSjTVA7mMQe2AQdKbkyC038zzRq%2FYHcjFDE%2Bz0qISwSHZY2NyLePmwU7SexEXnIz37jKC6NMEhus%3D",
85+
"query_string" : "SAMLRequest=nZFda4MwFIb%2FiuS%2BmviRpqFaClKQdbvo2g12M2KMraCJ9cRR9utnW4Wyi13sMie873MeznJ1aWrnS3VQGR0j4mLkKC1NUeljjA77zYyhVbIE0dR%2By7fmaHq7U%2BdegXWGpAZ%2B%2F4pR32luBFTAtWgUcCv56%2Fp5y30X87Yz1khTIycdgpUW9kY7WdsC9zxoXTvMvWuVV98YyMnSGH2SYE5pwALBIr9QKiwDGpW0oGVUznGeMyJZKFkQ4jBf5HnhUymjIhzCAL3KNFihbYx8TBYzzGaY7EnIyZwHzCWMfiDnbRIftkSjJr%2BFu0e9v%2B0EgOquRiiZjKpiVFp6j50T4WXoyNJ%2FEWC9fdqc1t%2F1%2B2F3aUpjzhPiXpqMz1%2FHSn4A&SigAlg=http%3A%2F%2Fwww.w3.org%2F2001%2F04%2Fxmldsig-more%23rsa-sha256&Signature=MsAYz2NFdovMG2mXf6TSpu5vlQQyEJAg%2B4KCwBqJTmrb3yGXKUtIgvjqf88eCAK32v3eN8vupjPC8LglYmke1ZnjK0%2FKxzkvSjTVA7mMQe2AQdKbkyC038zzRq%2FYHcjFDE%2Bz0qISwSHZY2NyLePmwU7SexEXnIz37jKC6NMEhus%3D",
8286
"realm" : "saml1"
8387
}
8488
--------------------------------------------------

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequest.java

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,10 +43,10 @@ public ActionRequestValidationException validate() {
4343
validationException = addValidationError("realm may not be empty", validationException);
4444
}
4545
if (Strings.hasText(queryString) == false && Strings.hasText(content) == false) {
46-
validationException = addValidationError("queryString and content may not both be empty", validationException);
46+
validationException = addValidationError("query_string and content may not both be empty", validationException);
4747
}
4848
if (Strings.hasText(queryString) && Strings.hasText(content)) {
49-
validationException = addValidationError("queryString and content may not both present", validationException);
49+
validationException = addValidationError("query_string and content may not both present", validationException);
5050
}
5151
return validationException;
5252
}
@@ -56,7 +56,11 @@ public String getQueryString() {
5656
}
5757

5858
public void setQueryString(String queryString) {
59-
this.queryString = queryString;
59+
if (this.queryString == null) {
60+
this.queryString = queryString;
61+
} else {
62+
throw new IllegalArgumentException("Must use either [query_string] or [queryString], not both at the same time");
63+
}
6064
}
6165

6266
public String getContent() {

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlInvalidateSessionRequest.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ public SamlInvalidateSessionRequest() {
4040
public ActionRequestValidationException validate() {
4141
ActionRequestValidationException validationException = null;
4242
if (Strings.isNullOrEmpty(queryString)) {
43-
validationException = addValidationError("queryString is missing", validationException);
43+
validationException = addValidationError("query_string is missing", validationException);
4444
}
4545
return validationException;
4646
}
@@ -50,7 +50,11 @@ public String getQueryString() {
5050
}
5151

5252
public void setQueryString(String queryString) {
53-
this.queryString = queryString;
53+
if (this.queryString == null) {
54+
this.queryString = queryString;
55+
} else {
56+
throw new IllegalArgumentException("Must use either [query_string] or [queryString], not both at the same time");
57+
}
5458
}
5559

5660
public String getRealmName() {

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequestTests.java

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ public void testValidateFailsWhenQueryAndBodyBothNotExist() {
1818
final SamlCompleteLogoutRequest samlCompleteLogoutRequest = new SamlCompleteLogoutRequest();
1919
samlCompleteLogoutRequest.setRealm("realm");
2020
final ActionRequestValidationException validationException = samlCompleteLogoutRequest.validate();
21-
assertThat(validationException.getMessage(), containsString("queryString and content may not both be empty"));
21+
assertThat(validationException.getMessage(), containsString("query_string and content may not both be empty"));
2222
}
2323

2424
public void testValidateFailsWhenQueryAndBodyBothSet() {
@@ -27,7 +27,7 @@ public void testValidateFailsWhenQueryAndBodyBothSet() {
2727
samlCompleteLogoutRequest.setQueryString("queryString");
2828
samlCompleteLogoutRequest.setContent("content");
2929
final ActionRequestValidationException validationException = samlCompleteLogoutRequest.validate();
30-
assertThat(validationException.getMessage(), containsString("queryString and content may not both present"));
30+
assertThat(validationException.getMessage(), containsString("query_string and content may not both present"));
3131
}
3232

3333
public void testValidateFailsWhenRealmIsNotSet() {
@@ -36,4 +36,12 @@ public void testValidateFailsWhenRealmIsNotSet() {
3636
final ActionRequestValidationException validationException = samlCompleteLogoutRequest.validate();
3737
assertThat(validationException.getMessage(), containsString("realm may not be empty"));
3838
}
39+
40+
public void testCannotSetQueryStringTwice() {
41+
final SamlCompleteLogoutRequest samlCompleteLogoutRequest = new SamlCompleteLogoutRequest();
42+
samlCompleteLogoutRequest.setQueryString("query_string");
43+
final IllegalArgumentException e =
44+
expectThrows(IllegalArgumentException.class, () -> samlCompleteLogoutRequest.setQueryString("queryString"));
45+
assertThat(e.getMessage(), containsString("Must use either [query_string] or [queryString], not both at the same time"));
46+
}
3947
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
8+
package org.elasticsearch.xpack.core.security.action.saml;
9+
10+
import org.elasticsearch.test.ESTestCase;
11+
12+
import static org.hamcrest.Matchers.containsString;
13+
14+
public class SamlInvalidateSessionRequestTests extends ESTestCase {
15+
16+
public void testCannotSetQueryStringTwice() {
17+
final SamlInvalidateSessionRequest samlInvalidateSessionRequest = new SamlInvalidateSessionRequest();
18+
samlInvalidateSessionRequest.setQueryString("query_string");
19+
final IllegalArgumentException e =
20+
expectThrows(IllegalArgumentException.class, () -> samlInvalidateSessionRequest.setQueryString("queryString"));
21+
assertThat(e.getMessage(), containsString("Must use either [query_string] or [queryString], not both at the same time"));
22+
}
23+
}

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlCompleteLogoutAction.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ public class RestSamlCompleteLogoutAction extends SamlBaseRestHandler{
4848
PARSER = new ObjectParser<>("saml_complete_logout", SamlCompleteLogoutRequest::new);
4949

5050
static {
51-
PARSER.declareStringOrNull(SamlCompleteLogoutRequest::setQueryString, new ParseField("queryString"));
51+
PARSER.declareStringOrNull(SamlCompleteLogoutRequest::setQueryString, new ParseField("query_string", "queryString"));
5252
PARSER.declareStringOrNull(SamlCompleteLogoutRequest::setContent, new ParseField("content"));
5353
PARSER.declareStringArray(SamlCompleteLogoutRequest::setValidRequestIds, new ParseField("ids"));
5454
PARSER.declareString(SamlCompleteLogoutRequest::setRealm, new ParseField("realm"));

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlInvalidateSessionAction.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ public class RestSamlInvalidateSessionAction extends SamlBaseRestHandler {
3838
new ObjectParser<>("saml_invalidate_session", SamlInvalidateSessionRequest::new);
3939

4040
static {
41-
PARSER.declareString(SamlInvalidateSessionRequest::setQueryString, new ParseField("queryString"));
41+
PARSER.declareString(SamlInvalidateSessionRequest::setQueryString, new ParseField("query_string", "queryString"));
4242
PARSER.declareString(SamlInvalidateSessionRequest::setAssertionConsumerServiceURL, new ParseField("acs"));
4343
PARSER.declareString(SamlInvalidateSessionRequest::setRealmName, new ParseField("realm"));
4444
}

0 commit comments

Comments
 (0)