diff --git a/modules/api/src/main/scala/vinyldns/api/route/StatusRouting.scala b/modules/api/src/main/scala/vinyldns/api/route/StatusRouting.scala index 2cd5c29da..0b3ef1cad 100644 --- a/modules/api/src/main/scala/vinyldns/api/route/StatusRouting.scala +++ b/modules/api/src/main/scala/vinyldns/api/route/StatusRouting.scala @@ -17,10 +17,15 @@ package vinyldns.api.route import akka.http.scaladsl.model.StatusCodes -import akka.http.scaladsl.server.Directives +import akka.http.scaladsl.server.Route import akka.util.Timeout import cats.effect.IO import fs2.concurrent.SignallingRef +import org.slf4j.{Logger, LoggerFactory} +import vinyldns.api.Interfaces.{EitherImprovements, Result, ensuring} +import vinyldns.api.config.ServerConfig +import vinyldns.api.domain.zone.NotAuthorizedError +import vinyldns.core.domain.auth.AuthPrincipal import scala.concurrent.duration._ @@ -31,39 +36,63 @@ final case class CurrentStatus( version: String ) -trait StatusRoute extends Directives { - this: VinylDNSJsonProtocol => +class StatusRoute( + serverConfig: ServerConfig, + val vinylDNSAuthenticator: VinylDNSAuthenticator, + val processingDisabled: SignallingRef[IO, Boolean] +) extends VinylDNSJsonProtocol + with VinylDNSDirectives[Throwable] { - implicit val timeout = Timeout(10.seconds) + def getRoutes: Route = statusRoute - def processingDisabled: SignallingRef[IO, Boolean] + implicit val timeout: Timeout = Timeout(10.seconds) - def statusRoute(color: String, version: String, keyName: String) = + def logger: Logger = LoggerFactory.getLogger(classOf[StatusRoute]) + + def handleErrors(e: Throwable): PartialFunction[Throwable, Route] = { + case NotAuthorizedError(msg) => complete(StatusCodes.Forbidden, msg) + } + + def postStatus(isProcessingDisabled: Boolean, authPrincipal: AuthPrincipal): Result[Boolean] = { + for { + _ <- isAdmin(authPrincipal).toResult + isDisabled = isProcessingDisabled + } yield isDisabled + } + + def isAdmin(authPrincipal: AuthPrincipal): Either[Throwable, Unit] = + ensuring(NotAuthorizedError(s"Not authorized. User '${authPrincipal.signedInUser.userName}' cannot make the requested change.")) { + authPrincipal.isSystemAdmin + } + + val statusRoute: Route = (get & path("status")) { onSuccess(processingDisabled.get.unsafeToFuture()) { isProcessingDisabled => complete( StatusCodes.OK, CurrentStatus( isProcessingDisabled, - color, - keyName, - version + serverConfig.color, + serverConfig.keyName, + serverConfig.version ) ) } } ~ (post & path("status")) { parameters("processingDisabled".as[Boolean]) { isProcessingDisabled => - onSuccess(processingDisabled.set(isProcessingDisabled).unsafeToFuture()) { - complete( - StatusCodes.OK, - CurrentStatus( - isProcessingDisabled, - color, - keyName, - version + authenticateAndExecute(postStatus(isProcessingDisabled, _)){ isProcessingDisabled => + onSuccess(processingDisabled.set(isProcessingDisabled).unsafeToFuture()) { + complete( + StatusCodes.OK, + CurrentStatus( + isProcessingDisabled, + serverConfig.color, + serverConfig.keyName, + serverConfig.version + ) ) - ) + } } } } diff --git a/modules/api/src/main/scala/vinyldns/api/route/VinylDNSService.scala b/modules/api/src/main/scala/vinyldns/api/route/VinylDNSService.scala index 1ef2a2f82..f9957bb2d 100644 --- a/modules/api/src/main/scala/vinyldns/api/route/VinylDNSService.scala +++ b/modules/api/src/main/scala/vinyldns/api/route/VinylDNSService.scala @@ -69,7 +69,6 @@ class VinylDNSService( ) extends PingRoute with HealthCheckRoute with BlueGreenRoute - with StatusRoute with PrometheusRoute with VinylDNSJsonProtocol with RequestLogging { @@ -97,27 +96,29 @@ class VinylDNSService( vinylDNSAuthenticator, vinyldnsConfig.manualReviewConfig ).getRoutes + val statusRoute: Route = + new StatusRoute( + vinyldnsConfig.serverConfig, + vinylDNSAuthenticator, + processingDisabled + ).getRoutes val unloggedUris = Seq( Uri.Path("/health"), Uri.Path("/color"), Uri.Path("/ping"), - Uri.Path("/status"), Uri.Path("/metrics/prometheus") ) val unloggedRoutes: Route = healthCheckRoute ~ pingRoute ~ colorRoute( vinyldnsConfig.serverConfig.color - ) ~ statusRoute( - vinyldnsConfig.serverConfig.color, - vinyldnsConfig.serverConfig.version, - vinyldnsConfig.serverConfig.keyName ) ~ prometheusRoute val allRoutes: Route = unloggedRoutes ~ batchChangeRoute ~ zoneRoute ~ recordSetRoute ~ - membershipRoute + membershipRoute ~ + statusRoute val vinyldnsRoutes: Route = logRequestResult(requestLogger(unloggedUris))(allRoutes) diff --git a/modules/api/src/test/functional/tests/internal/status_test.py b/modules/api/src/test/functional/tests/internal/status_test.py index c8f87acbc..c6cfccc5a 100644 --- a/modules/api/src/test/functional/tests/internal/status_test.py +++ b/modules/api/src/test/functional/tests/internal/status_test.py @@ -18,6 +18,43 @@ def test_get_status_success(shared_zone_test_context): assert_that(result["version"], not_none()) +def test_post_status_fails_for_non_admin_users(shared_zone_test_context): + """ + Tests that the post request to status endpoint fails for non-admin users + """ + client = shared_zone_test_context.ok_vinyldns_client + result = client.post_status(True) + + assert_that(result, is_("Not authorized. User 'ok' cannot make the requested change.")) + + +def test_post_status_fails_for_non_users(shared_zone_test_context): + """ + Tests that the post request to status endpoint fails for non-users with fake access and secret key + """ + client = shared_zone_test_context.non_user_client + result = client.post_status(True) + + assert_that(result, is_("Authentication Failed: Account with accessKey not-exist-key specified was not found")) + + +def test_post_status_pass_for_admin_users(shared_zone_test_context): + """ + Tests that the post request to status endpoint pass for admin users + """ + client = shared_zone_test_context.support_user_client + + client.post_status(True) + + status = client.get_status() + assert_that(status["processingDisabled"], is_(True)) + + client.post_status(False) + + status = client.get_status() + assert_that(status["processingDisabled"], is_(False)) + + @pytest.mark.serial @pytest.mark.skip_production def test_toggle_processing(shared_zone_test_context): @@ -25,15 +62,16 @@ def test_toggle_processing(shared_zone_test_context): Test that updating a zone when processing is disabled does not happen """ client = shared_zone_test_context.ok_vinyldns_client + admin_client = shared_zone_test_context.support_user_client ok_zone = copy.deepcopy(shared_zone_test_context.ok_zone) # disable processing - client.post_status(True) + admin_client.post_status(True) status = client.get_status() assert_that(status["processingDisabled"], is_(True)) - client.post_status(False) + admin_client.post_status(False) status = client.get_status() assert_that(status["processingDisabled"], is_(False)) diff --git a/modules/api/src/test/functional/tests/shared_zone_test_context.py b/modules/api/src/test/functional/tests/shared_zone_test_context.py index 1206d608c..3bef09088 100644 --- a/modules/api/src/test/functional/tests/shared_zone_test_context.py +++ b/modules/api/src/test/functional/tests/shared_zone_test_context.py @@ -31,8 +31,9 @@ class SharedZoneTestContext(object): self.unassociated_client = VinylDNSClient(VinylDNSTestContext.vinyldns_url, "listGroupAccessKey", "listGroupSecretKey") self.test_user_client = VinylDNSClient(VinylDNSTestContext.vinyldns_url, "testUserAccessKey", "testUserSecretKey") self.history_client = VinylDNSClient(VinylDNSTestContext.vinyldns_url, "history-key", "history-secret") + self.non_user_client = VinylDNSClient(VinylDNSTestContext.vinyldns_url, "not-exist-key", "not-exist-secret") self.clients = [self.ok_vinyldns_client, self.dummy_vinyldns_client, self.shared_zone_vinyldns_client, self.support_user_client, - self.unassociated_client, self.test_user_client, self.history_client] + self.unassociated_client, self.test_user_client, self.history_client, self.non_user_client] self.list_zones = ListZonesTestContext(partition_id) self.list_zones_client = self.list_zones.client self.list_records_context = ListRecordSetsTestContext(partition_id) diff --git a/modules/api/src/test/scala/vinyldns/api/VinylDNSTestHelpers.scala b/modules/api/src/test/scala/vinyldns/api/VinylDNSTestHelpers.scala index ffc6054f8..d4fc56ba5 100644 --- a/modules/api/src/test/scala/vinyldns/api/VinylDNSTestHelpers.scala +++ b/modules/api/src/test/scala/vinyldns/api/VinylDNSTestHelpers.scala @@ -16,10 +16,12 @@ package vinyldns.api +import cats.effect.{ContextShift, IO} import com.comcast.ip4s.IpAddress +import fs2.concurrent.SignallingRef import java.time.{Instant, LocalDateTime, Month, ZoneOffset} import vinyldns.api.config.{BatchChangeConfig, HighValueDomainConfig, LimitsConfig, ManualReviewConfig, ScheduledChangesConfig} -import vinyldns.api.config.{ZoneAuthConfigs, DottedHostsConfig, BatchChangeConfig, HighValueDomainConfig, LimitsConfig, ManualReviewConfig, ScheduledChangesConfig} +import vinyldns.api.config.{ZoneAuthConfigs, DottedHostsConfig, BatchChangeConfig, HighValueDomainConfig, LimitsConfig, ManualReviewConfig, ScheduledChangesConfig, ServerConfig} import vinyldns.api.domain.batch.V6DiscoveryNibbleBoundaries import vinyldns.core.domain.record._ import vinyldns.core.domain.zone._ @@ -28,6 +30,12 @@ import scala.util.matching.Regex trait VinylDNSTestHelpers { + private implicit val cs: ContextShift[IO] = + IO.contextShift(scala.concurrent.ExecutionContext.global) + + val processingDisabled: SignallingRef[IO, Boolean] = + fs2.concurrent.SignallingRef[IO, Boolean](false).unsafeRunSync() + val highValueDomainRegexList: List[Regex] = List(new Regex("high-value-domain.*")) val highValueDomainIpList: List[IpAddress] = (IpAddress("192.0.2.252") ++ IpAddress("192.0.2.253") ++ IpAddress( @@ -76,6 +84,9 @@ trait VinylDNSTestHelpers { val testLimitConfig: LimitsConfig = LimitsConfig(100,100,1000,1500,100,100,100) + val testServerConfig: ServerConfig = + ServerConfig(100, 100, 100, 100, true, approvedNameServers, "blue", "unset", "vinyldns.", false, true, true) + val batchChangeConfig: BatchChangeConfig = BatchChangeConfig(batchChangeLimit, sharedApprovedTypes, v6DiscoveryNibbleBoundaries) diff --git a/modules/api/src/test/scala/vinyldns/api/domain/membership/MembershipValidationsSpec.scala b/modules/api/src/test/scala/vinyldns/api/domain/membership/MembershipValidationsSpec.scala index c59eb2fa0..83977ba73 100644 --- a/modules/api/src/test/scala/vinyldns/api/domain/membership/MembershipValidationsSpec.scala +++ b/modules/api/src/test/scala/vinyldns/api/domain/membership/MembershipValidationsSpec.scala @@ -94,7 +94,13 @@ class MembershipValidationsSpec val nonSuperAuth = AuthPrincipal(user, Seq()) canSeeGroup(okGroup.id, nonSuperAuth) should be(right) } + } + "User toString" should { + "not display access and secret keys" in { + val userString = s"""User: [id="ok"; userName="ok"; firstName="Some(ok)"; lastName="Some(ok)"; email="Some(test@test.com)"; created="${okUser.created}"; isSuper="false"; isSupport="false"; isTest="false"; lockStatus="Unlocked"; ]""" + okUser.toString shouldBe userString + } } "isGroupChangePresent" should { diff --git a/modules/api/src/test/scala/vinyldns/api/route/StatusRoutingSpec.scala b/modules/api/src/test/scala/vinyldns/api/route/StatusRoutingSpec.scala index adec84c2a..ceab8eb0c 100644 --- a/modules/api/src/test/scala/vinyldns/api/route/StatusRoutingSpec.scala +++ b/modules/api/src/test/scala/vinyldns/api/route/StatusRoutingSpec.scala @@ -18,35 +18,49 @@ package vinyldns.api.route import akka.actor.ActorSystem import akka.http.scaladsl.model.StatusCodes +import akka.http.scaladsl.server.Route import akka.http.scaladsl.testkit.ScalatestRouteTest -import cats.effect.{ContextShift, IO} -import fs2.concurrent.SignallingRef import org.scalatest._ import org.scalatest.matchers.should.Matchers import org.scalatest.wordspec.AnyWordSpec import org.scalatestplus.mockito.MockitoSugar +import vinyldns.api.VinylDNSTestHelpers +import vinyldns.api.VinylDNSTestHelpers.processingDisabled +import vinyldns.core.TestMembershipData.{notAuth, okAuth, superUserAuth} class StatusRoutingSpec extends AnyWordSpec with ScalatestRouteTest - with StatusRoute with OneInstancePerTest with VinylDNSJsonProtocol with BeforeAndAfterEach with MockitoSugar with Matchers { + val statusRoute: Route = + new StatusRoute( + VinylDNSTestHelpers.testServerConfig, + new TestVinylDNSAuthenticator(okAuth), + VinylDNSTestHelpers.processingDisabled + ).getRoutes + val notAuthRoute: Route = + new StatusRoute( + VinylDNSTestHelpers.testServerConfig, + new TestVinylDNSAuthenticator(notAuth), + VinylDNSTestHelpers.processingDisabled + ).getRoutes + val adminUserRoute: Route = + new StatusRoute( + VinylDNSTestHelpers.testServerConfig, + new TestVinylDNSAuthenticator(superUserAuth), + VinylDNSTestHelpers.processingDisabled + ).getRoutes + def actorRefFactory: ActorSystem = system - private implicit val cs: ContextShift[IO] = - IO.contextShift(scala.concurrent.ExecutionContext.global) - - val processingDisabled: SignallingRef[IO, Boolean] = - fs2.concurrent.SignallingRef[IO, Boolean](false).unsafeRunSync() - "GET /status" should { "return the current status of true" in { - Get("/status") ~> statusRoute("blue", "unset", "vinyldns.") ~> check { + Get("/status") ~> statusRoute ~> check { response.status shouldBe StatusCodes.OK val resultStatus = responseAs[CurrentStatus] resultStatus.processingDisabled shouldBe false @@ -58,16 +72,16 @@ class StatusRoutingSpec } "POST /status" should { - "disable processing" in { - Post("/status?processingDisabled=true") ~> statusRoute("blue", "unset", "vinyldns.") ~> check { + "disable processing when it's requested by admin user" in { + Post("/status?processingDisabled=true") ~> adminUserRoute ~> check { response.status shouldBe StatusCodes.OK val resultStatus = responseAs[CurrentStatus] resultStatus.processingDisabled shouldBe true } } - "enable processing" in { - Post("/status?processingDisabled=false") ~> statusRoute("blue", "unset", "vinyldns.") ~> check { + "enable processing when it's requested by admin user" in { + Post("/status?processingDisabled=false") ~> adminUserRoute ~> check { response.status shouldBe StatusCodes.OK val resultStatus = responseAs[CurrentStatus] resultStatus.processingDisabled shouldBe false @@ -76,5 +90,35 @@ class StatusRoutingSpec processingDisabled.get.unsafeRunSync() shouldBe false } } + + "not disable processing when it's requested by non-admin user" in { + Post("/status?processingDisabled=true") ~> statusRoute ~> check { + response.status shouldBe StatusCodes.Forbidden + } + } + + "not enable processing when it's requested by non-admin user" in { + Post("/status?processingDisabled=false") ~> statusRoute ~> check { + response.status shouldBe StatusCodes.Forbidden + + // remember, the signal is the opposite of intent + processingDisabled.get.unsafeRunSync() shouldBe false + } + } + + "not disable processing when it's requested by a non-user" in { + Post("/status?processingDisabled=true") ~> notAuthRoute ~> check { + response.status shouldBe StatusCodes.Forbidden + } + } + + "not enable processing when it's requested by a non-user" in { + Post("/status?processingDisabled=false") ~> notAuthRoute ~> check { + response.status shouldBe StatusCodes.Forbidden + + // remember, the signal is the opposite of intent + processingDisabled.get.unsafeRunSync() shouldBe false + } + } } } diff --git a/modules/core/src/main/scala/vinyldns/core/domain/membership/User.scala b/modules/core/src/main/scala/vinyldns/core/domain/membership/User.scala index 546d0a070..552358a2a 100644 --- a/modules/core/src/main/scala/vinyldns/core/domain/membership/User.scala +++ b/modules/core/src/main/scala/vinyldns/core/domain/membership/User.scala @@ -52,6 +52,23 @@ final case class User( def withEncryptedSecretKey(cryptoAlgebra: CryptoAlgebra): User = copy(secretKey = cryptoAlgebra.encrypt(secretKey)) + + override def toString: String = { + val sb = new StringBuilder + sb.append("User: [") + sb.append("id=\"").append(id).append("\"; ") + sb.append("userName=\"").append(userName).append("\"; ") + sb.append("firstName=\"").append(firstName.toString).append("\"; ") + sb.append("lastName=\"").append(lastName.toString).append("\"; ") + sb.append("email=\"").append(email.toString).append("\"; ") + sb.append("created=\"").append(created).append("\"; ") + sb.append("isSuper=\"").append(isSuper).append("\"; ") + sb.append("isSupport=\"").append(isSupport).append("\"; ") + sb.append("isTest=\"").append(isTest).append("\"; ") + sb.append("lockStatus=\"").append(lockStatus.toString).append("\"; ") + sb.append("]") + sb.toString + } } object User { diff --git a/modules/portal/test/controllers/TestApplicationData.scala b/modules/portal/test/controllers/TestApplicationData.scala index bf9de5004..5330bad4f 100644 --- a/modules/portal/test/controllers/TestApplicationData.scala +++ b/modules/portal/test/controllers/TestApplicationData.scala @@ -161,7 +161,7 @@ trait TestApplicationData { this: Mockito => | "oldGroup": {}, | "id": "b6018a9b-c893-40e9-aa25-4ccfee460c18", | "created": "2022-07-22T08:19:22Z", - | "userName": "$frodoUser", + | "userName": "${frodoUser.userName}", | "groupChangeMessage": "" | } """.stripMargin) @@ -181,7 +181,7 @@ trait TestApplicationData { this: Mockito => | "oldGroup": {}, | "id": "b6018a9b-c893-40e9-aa25-4ccfee460c18", | "created": "2022-07-22T08:19:22Z", - | "userName": "$frodoUser", + | "userName": "${frodoUser.userName}", | "groupChangeMessage": "" | }], | "maxItems": 100