From 2c20740a43b95e27e7245905e88fe6367ac68ce5 Mon Sep 17 00:00:00 2001 From: Bruce Potter Date: Wed, 29 Jan 2020 09:32:36 -0500 Subject: [PATCH 1/3] Fixed another case for issue 264 - 401 incorrectly returned when it should return 502 --- README.md | 5 ++ src/main/resources/version.txt | 2 +- .../com/horizon/exchangeapi/AuthCache.scala | 48 ++++++++++--------- .../exchangeapi/AuthorizationSupport.scala | 12 +++-- .../com/horizon/exchangeapi/OrgsRoutes.scala | 1 + src/test/bash/primedb.sh | 14 +++--- 6 files changed, 48 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index 6c9123db..6d6eb9d7 100644 --- a/README.md +++ b/README.md @@ -224,6 +224,11 @@ Now you can disable root by setting `api.root.enabled` to `false` in `/etc/horiz - detect if a pattern is updated with service that has userInput w/o default values, and give warning - Consider changing all creates to POST, and update (via put/patch) return codes to 200 +## Changes in 2.10.0 + +- Fixed another case for issue 264 +- Moved the sort of `/changes` data to exchange scala code (from the postgresql db), and simplified the query filters a little + ## Changes in 2.9.0 - Issue 284: Notification Framework no longer throws an error for empty db responses diff --git a/src/main/resources/version.txt b/src/main/resources/version.txt index f3ac133c..f161b5d8 100644 --- a/src/main/resources/version.txt +++ b/src/main/resources/version.txt @@ -1 +1 @@ -2.9.0 \ No newline at end of file +2.10.0 \ No newline at end of file diff --git a/src/main/scala/com/horizon/exchangeapi/AuthCache.scala b/src/main/scala/com/horizon/exchangeapi/AuthCache.scala index 0d1b7bb1..262971ad 100644 --- a/src/main/scala/com/horizon/exchangeapi/AuthCache.scala +++ b/src/main/scala/com/horizon/exchangeapi/AuthCache.scala @@ -53,32 +53,34 @@ object AuthCache /* extends Control with ServletApiImplicits */ { def init(db: Database): Unit = { this.db = db } // we intentionally don't prime the cache. We let it build on every access so we can add the unhashed token // Try to authenticate the creds and return the type (user/node/agbot) it is, or None - def getValidType(creds: Creds, retry: Boolean = false): CacheIdType = { + def getValidType(creds: Creds, alreadyRetried: Boolean = false): Try[CacheIdType] = { //logger.debug("CacheId:getValidType(): attempting to authenticate to the exchange with " + creds) val cacheValue = getCacheValue(creds) logger.debug("cacheValue: " + cacheValue) - if (cacheValue.isFailure) return CacheIdType.None - // we got the hashed token from the cache or db, now verify the token passed in - val cacheVal = cacheValue.get - if (cacheVal.unhashedToken != "" && Password.check(creds.token, cacheVal.unhashedToken)) { // much faster than the bcrypt check below - //logger.debug("CacheId:getValidType(): successfully quick-validated " + creds.id + " and its pw using the cache/db") - return cacheVal.idType - } else if (Password.check(creds.token, cacheVal.hashedToken)) { - //logger.debug("CacheId:getValidType(): successfully validated " + creds.id + " and its pw using the cache/db") - return cacheVal.idType - } else { - // the creds were invalid - if (retry) { - // we already tried clearing the cache and retrying, so give up and return that they were bad creds - logger.debug("CacheId:getValidType(): user " + creds.id + " not authenticated in the exchange") - return CacheIdType.None - } else { - // If we only used a non-expired cache entry to get here, the cache entry could be stale (e.g. they recently changed their pw/token via a different instance of the exchange). - // So delete the cache entry from the db and try 1 more time - logger.debug("CacheId:getValidType(): user " + creds.id + " was not authenticated successfully, removing cache entry in case it was stale, and trying 1 more time") - removeOne(creds.id) - return getValidType(creds, retry = true) - } + cacheValue match { + case Failure(t) => return Failure(t) // bubble up the specific failure + case Success(cacheVal) => + // we got the hashed token from the cache or db, now verify the token passed in + if (cacheVal.unhashedToken != "" && Password.check(creds.token, cacheVal.unhashedToken)) { // much faster than the bcrypt check below + //logger.debug("CacheId:getValidType(): successfully quick-validated " + creds.id + " and its pw using the cache/db") + return Success(cacheVal.idType) + } else if (Password.check(creds.token, cacheVal.hashedToken)) { + //logger.debug("CacheId:getValidType(): successfully validated " + creds.id + " and its pw using the cache/db") + return Success(cacheVal.idType) + } else { + // the creds were invalid + if (alreadyRetried) { + // we already tried clearing the cache and retrying, so give up and return that they were bad creds + logger.debug("CacheId:getValidType(): user " + creds.id + " not authenticated in the exchange") + return Success(CacheIdType.None) // this is distinguished from Failure, because we didn't hit an error trying to access the db, it's just that the creds weren't value + } else { + // If we only used a non-expired cache entry to get here, the cache entry could be stale (e.g. they recently changed their pw/token via a different instance of the exchange). + // So delete the cache entry from the db and try 1 more time + logger.debug("CacheId:getValidType(): user " + creds.id + " was not authenticated successfully, removing cache entry in case it was stale, and trying 1 more time") + removeOne(creds.id) + return getValidType(creds, alreadyRetried = true) + } + } } } diff --git a/src/main/scala/com/horizon/exchangeapi/AuthorizationSupport.scala b/src/main/scala/com/horizon/exchangeapi/AuthorizationSupport.scala index 44a55498..fbe71c6f 100644 --- a/src/main/scala/com/horizon/exchangeapi/AuthorizationSupport.scala +++ b/src/main/scala/com/horizon/exchangeapi/AuthorizationSupport.scala @@ -268,10 +268,14 @@ trait AuthorizationSupport { //else throw new InvalidCredentialsException("invalid token") <- hint==token means it *could* be a token, not that it *must* be } AuthCache.ids.getValidType(creds) match { - case CacheIdType.User => return toIUser - case CacheIdType.Node => return toINode - case CacheIdType.Agbot => return toIAgbot - case CacheIdType.None => throw new InvalidCredentialsException() // will be caught by AuthenticationSupport.authenticate() + case Success(cacheIdType) => + cacheIdType match { + case CacheIdType.User => return toIUser + case CacheIdType.Node => return toINode + case CacheIdType.Agbot => return toIAgbot + case CacheIdType.None => throw new InvalidCredentialsException() // will be caught by AuthenticationSupport.authenticate() + } + case Failure(t) => throw t // this is usually 1 of our exceptions - will be caught by AuthenticationSupport.authenticate() } } diff --git a/src/main/scala/com/horizon/exchangeapi/OrgsRoutes.scala b/src/main/scala/com/horizon/exchangeapi/OrgsRoutes.scala index d450e272..9e98bc77 100644 --- a/src/main/scala/com/horizon/exchangeapi/OrgsRoutes.scala +++ b/src/main/scala/com/horizon/exchangeapi/OrgsRoutes.scala @@ -573,6 +573,7 @@ trait OrgsRoutes extends JacksonSupport with AuthenticationSupport { case _ => ; } val q = for { r <- qFilter.sortBy(_.changeId) } yield r //sort the response by changeId + logger.debug(s"POST /orgs/$orgId/changes db query: ${q.result.statements}") var qResp : scala.Seq[ResourceChangeRow] = null db.run(q.result.asTry.flatMap({ case Success(qResult) => diff --git a/src/test/bash/primedb.sh b/src/test/bash/primedb.sh index a75f1706..b2f84af5 100755 --- a/src/test/bash/primedb.sh +++ b/src/test/bash/primedb.sh @@ -78,6 +78,8 @@ agreementid1="${agreementbase}1" agreementid2="${agreementbase}2" agreementid3="${agreementbase}3" +encodedPubKey="QUJDCg==" # this is ABC base64 encoded + #resname="res1" #resversion="7.8.9" #resid="${resname}_$resversion" @@ -479,7 +481,7 @@ if [[ $rc != 200 ]]; then ] } ], - "publicKey": "ABC" }' + "publicKey": "'$encodedPubKey'" }' else echo "orgs/$orgid/nodes/$nodeid exists" fi @@ -487,7 +489,7 @@ fi rc=$(curlfind $userauth "orgs/$orgid/nodes/$nodeid2") checkrc "$rc" 200 404 if [[ $rc != 200 ]]; then - curlcreate "PUT" $userauth "orgs/$orgid/nodes/$nodeid2" '{"token": "'$nodetoken'", "name": "rpi1", "pattern": "'$orgid'/'$patid'", "registeredServices": [{"url": "'$orgid'/'$svcurl'", "numAgreements": 1, "policy": "", "properties": []}], "publicKey": "ABC" }' + curlcreate "PUT" $userauth "orgs/$orgid/nodes/$nodeid2" '{"token": "'$nodetoken'", "name": "rpi1", "pattern": "'$orgid'/'$patid'", "registeredServices": [{"url": "'$orgid'/'$svcurl'", "numAgreements": 1, "policy": "", "properties": []}], "publicKey": "'$encodedPubKey'" }' else echo "orgs/$orgid/nodes/$nodeid2 exists" fi @@ -496,7 +498,7 @@ if isPublicCloud; then rc=$(curlfind $userauthorg2 "orgs/$orgid2/nodes/$nodeid") checkrc "$rc" 200 404 if [[ $rc != 200 ]]; then - curlcreate "PUT" $userauthorg2 "orgs/$orgid2/nodes/$nodeid" '{"token": "'$nodetoken'", "name": "rpi1", "pattern": "'$orgid'/'$patid'", "registeredServices": [], "publicKey": "ABC" }' + curlcreate "PUT" $userauthorg2 "orgs/$orgid2/nodes/$nodeid" '{"token": "'$nodetoken'", "name": "rpi1", "pattern": "'$orgid'/'$patid'", "registeredServices": [], "publicKey": "'$encodedPubKey'" }' else echo "orgs/$orgid2/nodes/$nodeid exists" fi @@ -522,7 +524,7 @@ if isPublicCloud; then rc=$(curlfind $userauthorg2 "orgs/$orgid2/nodes/$nodeid2") checkrc "$rc" 200 404 if [[ $rc != 200 ]]; then - curlcreate "PUT" $userauthorg2 "orgs/$orgid2/nodes/$nodeid2" '{"token": "'$nodetoken'", "name": "rpi1", "pattern": "", "publicKey": "ABC" }' + curlcreate "PUT" $userauthorg2 "orgs/$orgid2/nodes/$nodeid2" '{"token": "'$nodetoken'", "name": "rpi1", "pattern": "", "publicKey": "'$encodedPubKey'" }' else echo "orgs/$orgid2/nodes/$nodeid2 exists" fi @@ -531,7 +533,7 @@ fi rc=$(curlfind $userauth "orgs/$orgid/agbots/$agbotid") checkrc "$rc" 200 404 if [[ $rc != 200 ]]; then - curlcreate "PUT" $userauth "orgs/$orgid/agbots/$agbotid" '{"token": "'$agbottoken'", "name": "agbot", "publicKey": "ABC"}' + curlcreate "PUT" $userauth "orgs/$orgid/agbots/$agbotid" '{"token": "'$agbottoken'", "name": "agbot", "publicKey": "'$encodedPubKey'"}' else echo "orgs/$orgid/agbots/$agbotid exists" fi @@ -540,7 +542,7 @@ if isPublicCloud; then rc=$(curlfind $userauthorg2 "orgs/$orgid2/agbots/$agbotid") checkrc "$rc" 200 404 if [[ $rc != 200 ]]; then - curlcreate "PUT" $userauthorg2 "orgs/$orgid2/agbots/$agbotid" '{"token": "'$agbottoken'", "name": "agbot", "publicKey": "ABC"}' + curlcreate "PUT" $userauthorg2 "orgs/$orgid2/agbots/$agbotid" '{"token": "'$agbottoken'", "name": "agbot", "publicKey": "'$encodedPubKey'"}' else echo "orgs/$orgid2/agbots/$agbotid exists" fi From 4303564ece127d5a76686af46296112229b3c13b Mon Sep 17 00:00:00 2001 From: Bruce Potter Date: Wed, 29 Jan 2020 12:38:11 -0500 Subject: [PATCH 2/3] Moved the sort of /changes data to exchange scala code (from the postgresql db), and simplified the query filters a little --- .../com/horizon/exchangeapi/OrgsRoutes.scala | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/main/scala/com/horizon/exchangeapi/OrgsRoutes.scala b/src/main/scala/com/horizon/exchangeapi/OrgsRoutes.scala index 9e98bc77..ef179d70 100644 --- a/src/main/scala/com/horizon/exchangeapi/OrgsRoutes.scala +++ b/src/main/scala/com/horizon/exchangeapi/OrgsRoutes.scala @@ -505,7 +505,11 @@ trait OrgsRoutes extends JacksonSupport with AuthenticationSupport { } // end of exchAuth } - def buildResourceChangesResponse(inputList: scala.Seq[ResourceChangeRow], maxRecords : Int): ResourceChangesRespObject ={ + def buildResourceChangesResponse(inputListUnsorted: scala.Seq[ResourceChangeRow], maxRecords : Int): ResourceChangesRespObject ={ + // Sort the rows based on the changeId. Default order is ascending, which is what we want + logger.debug(s"POST /orgs/{orgid}/changes sorting ${inputListUnsorted.size} rows") + val inputList = inputListUnsorted.sortBy(_.changeId) // Note: we are doing the sorting here instead of in the db via sql, because the latter seems to use a lot of db cpu + // fill in some values we can before processing val exchangeVersion = ExchangeApi.adminVersion() val maxChangeIdOfQuery = inputList.last.changeId // this is the maximum changeId of the entire query from the db @@ -565,17 +569,19 @@ trait OrgsRoutes extends JacksonSupport with AuthenticationSupport { // Variables to help with building the query val lastTime = reqBody.lastUpdated.getOrElse(ApiTime.beginningUTC) // filter by lastUpdated and changeId then filter by either it's in the org OR it's not in the same org but is public - var qFilter = ResourceChangesTQ.rows.filter(_.lastUpdated >= lastTime).filter(_.changeId >= reqBody.changeId).filter(u => (u.orgId === orgId) || (u.orgId =!= orgId && u.public === "true")) - ident match { + //var qFilter = ResourceChangesTQ.rows.filter(_.lastUpdated >= lastTime).filter(_.changeId >= reqBody.changeId).filter(u => (u.orgId === orgId) || (u.orgId =!= orgId && u.public === "true")) + val qFilter = ident match { case _: INode => // if its a node calling then it doesn't want information about any other nodes - qFilter = qFilter.filter(u => (u.category === "node" && u.id === ident.getIdentity) || u.category =!= "node") - case _ => ; + ResourceChangesTQ.rows.filter(_.changeId >= reqBody.changeId).filter(_.lastUpdated >= lastTime).filter(u => (u.orgId === orgId) || (u.orgId =!= orgId && u.public === "true")).filter(u => (u.category === "node" && u.id === ident.getIdentity) || u.category =!= "node") + case _ => + // Note: repeating some of the filters in both cases to make the final query less nested for the db + ResourceChangesTQ.rows.filter(_.changeId >= reqBody.changeId).filter(_.lastUpdated >= lastTime).filter(u => (u.orgId === orgId) || (u.orgId =!= orgId && u.public === "true")) } - val q = for { r <- qFilter.sortBy(_.changeId) } yield r //sort the response by changeId - logger.debug(s"POST /orgs/$orgId/changes db query: ${q.result.statements}") + //val q = for { r <- qFilter.sortBy(_.changeId) } yield r //sort the response by changeId + logger.debug(s"POST /orgs/$orgId/changes db query: ${qFilter.result.statements}") var qResp : scala.Seq[ResourceChangeRow] = null - db.run(q.result.asTry.flatMap({ + db.run(qFilter.result.asTry.flatMap({ case Success(qResult) => //logger.debug("POST /orgs/" + orgId + "/changes changes : " + qOrgResult.toString()) logger.debug("POST /orgs/" + orgId + "/changes changes : " + qResult.size) From 63244d2d496180a0b8c4f2a598e6f9b95cb86b1b Mon Sep 17 00:00:00 2001 From: Bruce Potter Date: Wed, 29 Jan 2020 15:12:36 -0500 Subject: [PATCH 3/3] fixed bug: bad user was returning 500 --- .../com/horizon/exchangeapi/AuthCache.scala | 4 +-- .../horizon/exchangeapi/auth/Exceptions.scala | 7 +++-- src/test/scala/exchangeapi/UsersSuite.scala | 27 ++++++++----------- 3 files changed, 18 insertions(+), 20 deletions(-) diff --git a/src/main/scala/com/horizon/exchangeapi/AuthCache.scala b/src/main/scala/com/horizon/exchangeapi/AuthCache.scala index 262971ad..beb2a7ec 100644 --- a/src/main/scala/com/horizon/exchangeapi/AuthCache.scala +++ b/src/main/scala/com/horizon/exchangeapi/AuthCache.scala @@ -200,7 +200,7 @@ object AuthCache /* extends Control with ServletApiImplicits */ { val isValue = respVector.head logger.debug("CacheBoolean:getId(): " + id + " was not in the cache but found in the db, adding it with value " + isValue + " to the cache") Success(isValue) - } else Failure(new IdNotFoundException) + } else Failure(new IdNotFoundForAuthorizationException) } catch { // Handle db problems case timeout: java.util.concurrent.TimeoutException => @@ -283,7 +283,7 @@ object AuthCache /* extends Control with ServletApiImplicits */ { val owner = respVector.head logger.debug("CacheOwner:getId(): " + id + " found in the db, adding it with value " + owner + " to the cache") Success(owner) - } else Failure(new IdNotFoundException) + } else Failure(new IdNotFoundForAuthorizationException) } catch { // Handle db problems case timeout: java.util.concurrent.TimeoutException => diff --git a/src/main/scala/com/horizon/exchangeapi/auth/Exceptions.scala b/src/main/scala/com/horizon/exchangeapi/auth/Exceptions.scala index 6e7a6bcc..ba5a51b3 100644 --- a/src/main/scala/com/horizon/exchangeapi/auth/Exceptions.scala +++ b/src/main/scala/com/horizon/exchangeapi/auth/Exceptions.scala @@ -57,8 +57,11 @@ class IamApiTimeoutException(msg: String) extends AuthException(HttpCode.GW_TIME // An error occurred while building the SSLSocketFactory with the self-signed cert class SelfSignedCertException(msg: String) extends AuthException(HttpCode.INTERNAL_ERROR, ApiRespType.INTERNAL_ERROR, msg) -// Only used internally: The local exchange id was not found in the db -class IdNotFoundException extends AuthException(HttpCode.INTERNAL_ERROR, ApiRespType.INTERNAL_ERROR, "id not found") +// The creds id was not found in the db +class IdNotFoundException(msg: String = ExchMsg.translate("invalid.credentials")) extends AuthException(HttpCode.BADCREDS, ApiRespType.BADCREDS, msg) + +// The id was not found in the db when looking for owner or isPublic +class IdNotFoundForAuthorizationException(msg: String = ExchMsg.translate("access.denied")) extends AuthException(HttpCode.ACCESS_DENIED, ApiRespType.ACCESS_DENIED, msg) class AuthInternalErrorException(msg: String) extends AuthException(HttpCode.INTERNAL_ERROR, ApiRespType.INTERNAL_ERROR, msg) diff --git a/src/test/scala/exchangeapi/UsersSuite.scala b/src/test/scala/exchangeapi/UsersSuite.scala index 0c636e6a..1515cc07 100644 --- a/src/test/scala/exchangeapi/UsersSuite.scala +++ b/src/test/scala/exchangeapi/UsersSuite.scala @@ -44,7 +44,8 @@ class UsersSuite extends FunSuite { val USERAUTH = ("Authorization", "Basic " + ApiUtils.encode(creds)) val ORG2USERAUTH = ("Authorization", "Basic " + ApiUtils.encode(org2user + ":" + pw)) //val encodedCreds = Base64.getEncoder.encodeToString(creds.getBytes("utf-8")) - val ENCODEDAUTH = ("Authorization", "Basic " + ApiUtils.encode(creds)) + val BADUSERAUTH = ("Authorization", "Basic " + ApiUtils.encode(s"bad$orguser:$pw")) + val USERAUTHBAD = ("Authorization", "Basic " + ApiUtils.encode(s"$orguser:${pw}bad")) val user2 = "u2" // this is NOT an admin user val orguser2 = orgid + "/" + user2 val pw2 = user2 + " pw" // intentionally adding a space in the pw @@ -376,10 +377,9 @@ class UsersSuite extends FunSuite { assert(u.email === user + "@gmail.com") } - /** Update the normal user with encoded creds */ - test("PUT /orgs/" + orgid + "/users/" + user + " - update normal with encoded creds") { + test("PUT /orgs/" + orgid + "/users/" + user + " - update normal with creds") { val input = PostPutUsersRequest(pw, admin = true, user + "-updated@gmail.com") - val response = Http(URL + "/users/" + user).postData(write(input)).method("put").headers(CONTENT).headers(ACCEPT).headers(ENCODEDAUTH).asString + val response = Http(URL + "/users/" + user).postData(write(input)).method("put").headers(CONTENT).headers(ACCEPT).headers(USERAUTH).asString info("code: " + response.code + ", response.body: " + response.body) assert(response.code === HttpCode.PUT_OK.intValue) } @@ -408,9 +408,8 @@ class UsersSuite extends FunSuite { assert(response.code === HttpCode.ACCESS_DENIED.intValue) } - /** Verify the encoded update worked */ - test("GET /orgs/" + orgid + "/users/" + user + " - encoded creds") { - val response: HttpResponse[String] = Http(URL + "/users/" + user).headers(ACCEPT).headers(ENCODEDAUTH).asString + test("GET /orgs/" + orgid + "/users/" + user + " - with creds") { + val response: HttpResponse[String] = Http(URL + "/users/" + user).headers(ACCEPT).headers(USERAUTH).asString info("code: " + response.code) // info("code: "+response.code+", response.body: "+response.body) assert(response.code === HttpCode.OK.intValue) @@ -420,7 +419,6 @@ class UsersSuite extends FunSuite { assert(u.email === user + "-updated@gmail.com") } - /** Confirm user/pw for user */ test("POST /orgs/" + orgid + "/users/" + user + "/confirm") { val response = Http(URL + "/users/" + user + "/confirm").method("post").headers(ACCEPT).headers(USERAUTH).asString info("code: " + response.code + ", response.body: " + response.body) @@ -429,19 +427,16 @@ class UsersSuite extends FunSuite { assert(postConfirmResp.code === ApiRespType.OK) } - /** Confirm encoded user/pw for user */ - test("POST /orgs/" + orgid + "/users/" + user + "/confirm - encoded") { - //info("encodedCreds=" + encodedCreds + ".") - val response = Http(URL + "/users/" + user + "/confirm").method("post").headers(ACCEPT).headers(ENCODEDAUTH).asString + test("POST /orgs/" + orgid + "/users/" + user + "/confirm - bad user") { + val response = Http(URL + "/users/" + user + "/confirm").method("post").headers(ACCEPT).headers(BADUSERAUTH).asString info("code: " + response.code + ", response.body: " + response.body) - assert(response.code === HttpCode.POST_OK.intValue) + assert(response.code === HttpCode.BADCREDS.intValue) val postConfirmResp = parse(response.body).extract[ApiResponse] - assert(postConfirmResp.code === ApiRespType.OK) + assert(postConfirmResp.code === ApiRespType.BADCREDS) } - /** Confirm user/pw for bad pw */ test("POST /orgs/" + orgid + "/users/" + user + "/confirm - bad pw") { - val response = Http(URL + "/users/" + user + "/confirm").method("post").headers(ACCEPT).headers(("Authorization", "Basic " + user + ":badpw")).asString + val response = Http(URL + "/users/" + user + "/confirm").method("post").headers(ACCEPT).headers(USERAUTHBAD).asString info("code: " + response.code + ", response.body: " + response.body) assert(response.code === HttpCode.BADCREDS.intValue) val postConfirmResp = parse(response.body).extract[ApiResponse]