jenkinsci / azure-ad-plugin

Authentication and Authorization with Azure AD
https://plugins.jenkins.io/azure-ad/
MIT License
27 stars 56 forks source link

Wrong group name validation if name provided with spaces [source code] #559

Open bostonaqua opened 2 months ago

bostonaqua commented 2 months ago

Jenkins and plugins versions report

Environment ```text azure-ad:442.v355cca_6b_c169 role-strategy:689.v731678c3e0eb_ ```

What Operating System are you using (both controller, and any agents involved in the problem)?

Jenkins deployed with official helm chart on Kubernetes cluster

Reproduction steps

Reproducing logic of this function via Jenkins Script Console:

import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
import com.microsoft.graph.options.QueryOption;

def groupName = 'Group Name With Spaces';  // group that exists in EntraID
def encodedGroupName = URLEncoder.encode(groupName, StandardCharsets.UTF_8.name()); 

def query = String.format("displayName eq '%s'", encodedGroupName);
def queryNoEncode = String.format("displayName eq '%s'", groupName);
println("Query with enc: $query\nQuery without enc: $queryNoEncode");

def requestOptions = new LinkedList<>();
def requestOptionsNoEncode = new LinkedList<>();
requestOptions.add(new QueryOption('$filter', query));
        requestOptionsNoEncode.add(new QueryOption('$filter', queryNoEncode));

def secRealm = Jenkins.get().getSecurityRealm();

groupCollectionPage = secRealm.getAzureClient().groups()
         .buildRequest(requestOptions)
         .select("id,displayName")
         .get();
println groupCollectionPage.getCurrentPage().size();  // 0. group will be null.

groupCollectionPageNoEncode = secRealm.getAzureClient().groups()
         .buildRequest(requestOptionsNoEncode)
         .select("id,displayName")
         .get();
println groupCollectionPageNoEncode.getCurrentPage().size();  // 1
println groupCollectionPageNoEncode.getCurrentPage().get(0).displayName;  // Group Name With Spaces

So this validation wont work with groups names which contain spaces.

Expected Results

validateGroup function validates group with spaces in its name

Actual Results

validateGroup function does not validate group with spaces in its name

Anything else?

As a solution need to replace all '+' to '%20' after encoding (did not test it, maybe need to replace it back to space ' ') I think encoding may affect other special characters

Are you interested in contributing a fix?

No response

dsrowell commented 2 months ago

Actually I think just encoding the group name is the bug. The implicitly-invoked GroupCollectionRequest inside getAzureClient().group().buildRequest().select().get() is encoding the entire URL, so what you end up with is "Group%2B%Name%2BWith%2BSpaces" in the actual URL. If you switch out '+' with '%20', then you'd end up with "Group%2520Name%2520With%2520Spaces", also not what you want. The solution, I believe, is to leave the group name alone (i.e. not attempt to encode it) and let the request encode it instead. But I sense a Chesterton's fence here and I'm reluctant to remove it without knowing why it's there in the first place.

timja commented 2 months ago

check the git blame on that line it was added for a reason.