Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

HIPP-1777: Stop creating hidden production credentials #220

Merged
merged 2 commits into from
Dec 12, 2024
Merged

HIPP-1777: Stop creating hidden production credentials #220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a10c966
HIPP-1777: Stop creating hidden production credentials
victorarbuesmallada Dec 11, 2024
5a05fb1
HIPP-1777: Address PR comments
victorarbuesmallada Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions app/uk/gov/hmrc/apihubapplications/models/idms/ClientResponse.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,6 @@ case class ClientResponse(clientId: String, secret: String) {
secretFragment = Some(secret.takeRight(4))
)
}

def asNewHiddenCredential(clock: Clock): Credential = {
Credential(
clientId = clientId,
created = LocalDateTime.now(clock),
clientSecret = None,
secretFragment = None
)
}

}

object ClientResponse {
Expand Down
5 changes: 4 additions & 1 deletion app/uk/gov/hmrc/apihubapplications/services/ApplicationsLifecycleService.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,10 @@ class ApplicationsLifecycleServiceImpl @ Inject()(

ApplicationEnrichers.process(
application,
hipEnvironments.environments.map(ApplicationEnrichers.credentialCreatingApplicationEnricher(_, application, idmsConnector, clock))
hipEnvironments.environments.filterNot(_.isProductionLike)
.map(
ApplicationEnrichers.credentialCreatingApplicationEnricher(_, application, idmsConnector, clock)
)
).flatMap {
case Right(enriched) =>
repository.insert(enriched).flatMap {
Expand Down
5 changes: 1 addition & 4 deletions app/uk/gov/hmrc/apihubapplications/services/helpers/ApplicationEnricher.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,15 +182,12 @@ object ApplicationEnrichers {
original: Application,
idmsConnector: IdmsConnector,
clock: Clock,
hiddenPrimary: Boolean = true
)(implicit ec: ExecutionContext, hc: HeaderCarrier): Future[Either[IdmsException, ApplicationEnricher]] = {
idmsConnector.createClient(hipEnvironment.environmentName, Client(original)).map {
case Right(clientResponse) =>
Right(
(application: Application) => {
if hipEnvironment.isProductionLike && hiddenPrimary then
application.addCredential(hipEnvironment.environmentName, clientResponse.asNewHiddenCredential(clock))
else application.addCredential(hipEnvironment.environmentName, clientResponse.asNewCredential(clock))
application.addCredential(hipEnvironment.environmentName, clientResponse.asNewCredential(clock))
}
)
case Left(e) => Left(e)
Expand Down
31 changes: 1 addition & 30 deletions test/uk/gov/hmrc/apihubapplications/services/ApplicationsLifecycleServiceSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ class ApplicationsLifecycleServiceSpec extends AsyncFreeSpec with Matchers with
.thenReturn(Future.successful(Right(secondaryClientResponse)))

val applicationWithCreds = application
.setCredentials(Primary, Seq(primaryClientResponse.asNewHiddenCredential(clock)))
.setCredentials(Primary, Seq.empty)
.setCredentials(Secondary, Seq(secondaryClientResponse.asNewCredential(clock)))

val saved = applicationWithCreds.copy(id = Some("test-id"))
Expand All @@ -97,31 +97,6 @@ class ApplicationsLifecycleServiceSpec extends AsyncFreeSpec with Matchers with
}
}

"must return IdmsException and not persist in MongoDb if the primary credentials fail" in {
val fixture = buildFixture
import fixture._

val newApplication = NewApplication(
"test-name",
Creator(email = "test-email"),
Seq.empty
)

when(idmsConnector.createClient(eqTo(Primary), eqTo(Client(newApplication)))(any))
.thenReturn(Future.successful(Left(IdmsException("test-message", CallError))))

val secondaryClientResponse = ClientResponse("secondary-client-id", "test-secret-5678")
when(idmsConnector.createClient(eqTo(Secondary), eqTo(Client(newApplication)))(any))
.thenReturn(Future.successful(Right(secondaryClientResponse)))

service.registerApplication(newApplication)(HeaderCarrier()) map {
actual =>
actual.left.value mustBe a[IdmsException]
verify(repository, times(0)).insert(any)
succeed
}
}

"must return IdmsException and not persist in MongoDb if the secondary credentials fail" in {
val fixture = buildFixture
import fixture._
Expand Down Expand Up @@ -211,7 +186,6 @@ class ApplicationsLifecycleServiceSpec extends AsyncFreeSpec with Matchers with
val saved = Application(newApplication, clock)
.copy(id = Some("id"))
.copy(teamId = None)
.addCredential(Primary, clientResponse.asNewHiddenCredential(clock))
.addCredential(Secondary, clientResponse.asNewCredential(clock))

when(repository.insert(any))
Expand Down Expand Up @@ -248,7 +222,6 @@ class ApplicationsLifecycleServiceSpec extends AsyncFreeSpec with Matchers with
val saved = Application(newApplication, clock)
.copy(id = Some("id"))
.copy(teamId = None)
.addCredential(Primary, clientResponse.asNewHiddenCredential(clock))
.addCredential(Secondary, clientResponse.asNewCredential(clock))

when(repository.insert(any))
Expand Down Expand Up @@ -285,7 +258,6 @@ class ApplicationsLifecycleServiceSpec extends AsyncFreeSpec with Matchers with
val saved = Application(newApplication, clock)
.copy(id = Some("id"))
.copy(teamId = Some("team-id"))
.addCredential(Primary, clientResponse.asNewHiddenCredential(clock))
.addCredential(Secondary, clientResponse.asNewCredential(clock))

when(repository.insert(any)).thenReturn(Future.successful(saved))
Expand Down Expand Up @@ -315,7 +287,6 @@ class ApplicationsLifecycleServiceSpec extends AsyncFreeSpec with Matchers with

val saved = Application(newApplication, clock)
.copy(id = Some("id"))
.addCredential(Primary, clientResponse.asNewHiddenCredential(clock))
.addCredential(Secondary, clientResponse.asNewCredential(clock))

when(repository.insert(any))
Expand Down
2 changes: 1 addition & 1 deletion test/uk/gov/hmrc/apihubapplications/services/helpers/ApplicationEnricherSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ class ApplicationEnricherSpec extends AsyncFreeSpec
"credentialCreatingApplicationEnricher" - {
"must create a credential in the hip environments and enrich the application with it" in {
val expected = Seq(
testApplication.setCredentials(FakeHipEnvironments.primaryEnvironment, Seq(testClientResponse1.asNewHiddenCredential(clock))),
Copy link
Contributor

Choose a reason for hiding this comment

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

We still need to test creating credentials in the primary environment - why remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the reason was because I was filtering the credentials creation for production/primary environments on credentialCreatingApplicationEnricher, that's now done on the ApplicationsLifeCycleService.
Although as of now, credentialCreatingApplicationEnricher will never create primary environments credentials.

testApplication.setCredentials(FakeHipEnvironments.primaryEnvironment, Seq(testClientResponse1.asNewCredential(clock))),
testApplication.setCredentials(FakeHipEnvironments.secondaryEnvironment, Seq(testClientResponse2.asNewCredential(clock))),
)

Expand Down