Clean up Parameter Map usage

[joshfischer1108]

We pass request parameters as a map through various service methods. This causes duplicated, hard-to-read code. We should remove the use of a map of parameters and explicitly call them out in the function definition.

An example of what needs to be removed is : here

This should not break or enhance any feature at this point. It should only clean up the code to make it more readable and maintainable.

[github-actions[bot]]

Stale issue message

[rwithers]

So just to be sure I understand what you are suggesting is instead of this:

final var paramMap =
    addToMap(responseType, state, redirectUri, clientId, codeChallengeMethod, codeChallenge);

final var response = authenticationService.authenticate(request, paramMap);

You would take and make authenticationService.authenticate take each map value as an input parameter?

Depending on the type of the fields I might recommend creating an object vs parameters. An object keeps the parameters from potentially getting transposed if you have a lot of repeating string data.

Let me know if I'm interpreting this issue correcting and I'll probably take it on then.

[joshfischer1108]

I think you are on point. Currently we use a map to hold all the values, it's loaded like so in the controller

private HashMap<String, String> addToMap(
      final String responseType,
      final String state,
      final String redirectUri,
      final String clientId,
      final String codeChallengeMethod,
      final String codeChallenge) {
    final var paramMap = new HashMap<String, String>();
    paramMap.put("response_type", responseType);
    paramMap.put("state", state);
    paramMap.put("redirect_uri", redirectUri);
    paramMap.put("client_id", clientId);
    paramMap.put("code_challenge_method", codeChallengeMethod);
    paramMap.put("code_challenge", codeChallenge);
    return paramMap;
  }

The problem with this is later on down the line we have to do a bunch of redundant calls to default an empty string like this.

checkArgument(
        attributeValidator.isNotBlank(
            params.getOrDefault(AuthorizationCodeFlowConstants.CLIENT_ID.getValue(), "")),
        "clientId must not be blank");

If we could keep the signature like the below for the service call, I'd be fine with that. If you have an idea that you think is better, I'm good with that too.

(responseType, state, redirectUri, clientId, codeChallengeMethod, codeChallenge)

[rwithers]

Okay maybe I'm making this too difficult but I'm trying to take the key/value pairs and assign them to objects.

I've introduced a package software.iridium.api.model and I want to take the key/values and make them the fields of an object then I'm going to have a factory that creates the object because many of the values will be hard coded for our implementation of the OAuth spec. I've got the initial object created.

I've been reading the spec: https://datatracker.ietf.org/doc/html/rfc6749#section-1.2

The Identity controller looks like it takes fields that are very similar to those described here: https://www.oauth.com/oauth2-servers/pkce/authorization-request/

I want to tie this to the IdentityController it seems equivalent to the Authorization Request from the actual speck. Am I reading this correctly?

[rwithers]

You will still have to check for the "" values because none of the fields are specified as required in the controller code as shown here:

https://github.com/IridiumIdentity/iridium/blob/f2b6dc18c9a602b44de23eae95ffe1365dae9d66/iridium-core-server/src/main/java/software/iridium/api/controller/AuthenticationController.java#L38

[rwithers]

So I've created an AuthenticationRequestHolder to replace the parameters that are being passed around from place to place. This has caused a lot of change in many places from params to the holder. The holder should be seen as essentially a DTO, as an aside I'm open to renaming. Should have a PR soon. Rather than doing it in two strokes I am planning on submitting everything now.

[github-actions[bot]]

Stale issue message

[erzohrakhan]

@rwithers by default all the request Params are required = true; In case of false, we have to mention @RequestParam(value = "response_type", required = false) final String responseType,

[rwithers]

So I'm on the fence actually about how much value turning the parameter map into an object will bring. I appreciate your question because as I think about it the controllers are a place where you might be able to introduce an AuthenticationObjectFactory that takes the parameter map and produces an AuthenticationObject then use that everywhere. Doing it that way will allow you to keep the required pieces the way you have called them out above.

[joshfischer1108]

Closing this out as it's been resolved by a few other PRs.