(#154) Set Groups and Case Sensitivity to optional Parameters

[TheLydonKing]

Release Notes:

• Removed overloaded methods and introduced AuthMethod trait to indicate authentication method. This removes ambiguity with the default parameters and avoids compilation errors.

Closes: #154

[github-actions]

Report: Report: clientLibrary - scala:2.12.17

Metric (instruction)	Coverage	Threshold	Status
Overall	62.51%	43.0%	✅
Changed Files	78.28%	70.0%	✅

File Path	Coverage	Threshold	Status
AuthMethod.scala	100.0%	0.0%	✅
TokenRetrievalClient.scala	56.56%	0.0%	✅

[dk1844]

The code is nice, but with a library we should meet additional standards (see comments below)

[dk1844]

Neither the new method nor the existing (which should be present for some time as described above) seem to be covered with unit tests.

That is unfortunate, because the appropriate class TokenRetrievalClientTest already exists and it would be nice to have it covered. I see two options:

1. You may need to mock the creation of RestTemplate & KerberosRestTemplate (could be done by moving the creation to a method that you override in the test) to write the unit test properly.
2. A half-measure would be just to mock the fetchToken methods in the class to check if those are receiving expected parameters. Not a complete unit test, but even this would be better than nothing if the option 1 cannot be reasonably done (I believe it should be).

[dk1844]

Since you are already providing the mocked exchange templates, I assume you want the full solution. Good choice.

However, you do not need to override all the fetchToken and refreshAccessToken (also these should be rather tested, not overridden). I think all you need is this:

Suggested change

	class MockableTokenRetrievalClient(
	host: String,
	restTemplate: RestTemplate,
	kerberosRestTemplate: KerberosRestTemplate) extends TokenRetrievalClient(host) {
	override private[client] def fetchToken(issuerUri: String, username: String, password: String): String = {
	val response: ResponseEntity[String] = restTemplate.exchange(
	issuerUri,
	HttpMethod.POST,
	null,
	classOf[String])
	response.getBody
	}
	override private[client] def fetchToken(
	issuerUri: String,
	keyTabLocation: Option[String],
	userPrincipal: Option[String]): String = {
	val response: ResponseEntity[String] = kerberosRestTemplate.exchange(
	issuerUri,
	HttpMethod.POST,
	null,
	classOf[String])
	response.getBody
	}
	override def refreshAccessToken(
	accessToken: AccessToken,
	refreshToken: RefreshToken): (AccessToken, RefreshToken) = {
	val issuerUri = s"$host/token/refresh"
	val jsonPayload: JsonObject = new JsonObject()
	jsonPayload.addProperty("token", accessToken.token)
	jsonPayload.addProperty("refresh", refreshToken.token)
	val headers = new HttpHeaders()
	headers.setContentType(MediaType.APPLICATION_JSON)
	headers.setAccept(Collections.singletonList(MediaType.APPLICATION_JSON))
	val requestEntity = new HttpEntity[String](jsonPayload.toString, headers)
	val response: ResponseEntity[String] = restTemplate.exchange(
	issuerUri,
	HttpMethod.POST,
	requestEntity,
	classOf[String]
	)
	val jsonObject = JsonParser.parseString(response.getBody).getAsJsonObject
	(
	AccessToken(jsonObject.get("token").getAsString),
	RefreshToken(jsonObject.get("refresh").getAsString)
	)
	}
	class MockableTokenRetrievalClient(
	host: String,
	restTemplate: RestTemplate,
	kerberosRestTemplate: KerberosRestTemplate) extends TokenRetrievalClient(host) {
	override def createRestTemplate(): RestTemplate = restTemplate
	override def createKerberosRestTemplate( /* opt params or two methods? */ ): KerberosRestTemplate = kerberosRestTempl

while updating the with

case class TokenRetrievalClient(host: String) {

  private[client] def createKerberosRestTemplate( /* opt params or two methods? */ ): KerberosRestTemplate = {
    new KerberosRestTemplate( /* opt params?*/)
  }

  private[client] def createRestTemplate(): RestTemplate = {
    new RestTemplate()
  }

// and wherever the `new RestTemplate()` or `new KerberosRestTemplate(...)` is used within `TokenRetrievalClient`, you replace it with the methods above.

This way, you only override the very minimal to get your exchange mocks in -- while the rest of the TokenRetrievalClient stays original as is correctly subjected to unit testing.

[dk1844]

Good advancement, just one more important thing in the tests:

when(mockRestTemplate.exchange(
      anyString(),
      any[HttpMethod],
      any[HttpEntity[String]],
      any[Class[String]]
    ))

is just too broad for a meaningful check. Please consider doing a better arguments check.

[dk1844]

Suggested change

	case (Some(_), Some(_)) =>
	logger.info(s"Fetching token with user $userPrincipal")
	new KerberosRestTemplate(keyTabLocation.get, userPrincipal.get)
	case (Some(definedKeyTabLocation), Some(definedUserPrincipal)) =>
	logger.info(s"Fetching token with user $definedUserPrincipal")
	new KerberosRestTemplate(definedKeyTabLocation, definedUserPrincipal)

You do not have to resort to optionValueName.get this way 😄

[dk1844]

Unfortunately, this is not good. This is the place where we express our expectations of what will be sent to the endpoint -- and the expectations must hold for the successful test (also for the test to make sense). If we allow anything, what is the point of the test -- it would just check that there was something sent (that is not enough).

import org.mockito.ArgumentMatchers.{eq => eqTo} // not to clash with `Matchers.eq`
// this may be common for the whole test or even with hardcoded values
val base64Credentials = new String(Base64.getEncoder.encodeToString((s"$dummyUser:$dummyPassword").getBytes()))

Suggested change

	when(mockRestTemplate.exchange(
	anyString(),
	any[HttpMethod],
	any[HttpEntity[String]],
	any[Class[String]]
	)).thenReturn(new ResponseEntity(tokenResponse, HttpStatus.OK))
	val expectedBasicAuthHeader = new HttpHeaders()
	expectedBasicAuthHeader.set("Authorization", s"Basic $base64Credentials")
	when(mockRestTemplate.exchange(
	eqTo(s"$dummyUri/token/generate"),
	eqTo(HttpMethod.POST),
	eqTo(new HttpEntity[String](null, expectedBasicAuthHeader)),
	eqTo(classOf[String])
	)).thenReturn(new ResponseEntity(tokenResponse, HttpStatus.OK))

... and I believe this is needed for every when(mockRestTemplate.exchange(...)) in this test -- of course the content will vary if it is a generate request, refresh request`, basic auth vs kerberos etc.

[TheLydonKing]

So, my assumption here is that the Template in this case is mimicking the actual API response and the API is a separate entity from testing the Client entity itself, so irrelevant to testing the client.

I will change it accordingly though.