Skip to content

Commit

Permalink
Improve gRPC error handling
Browse files Browse the repository at this point in the history 
This change removes non-idiomatic gRPC *Reply proto message fields.
The client should not receive success/fail values from server methods
with a void return type, nor an optional error_message from any server
method.  This change improves error handling by wrapping an appropriate
gRPC Status with a meaningful error description in a StatusRuntimeException,
and placing it in the server's response StreamObserver.  User error
messages are mapped to general purpose gRPC Status codes in a new ApiStatus
enum class.  (Maybe ApiStatus should be renamed to CoreApiStatus.)
  • Loading branch information
@ghubstan
ghubstan committed May 1, 2020
1 parent c5fcafb commit 2a9d1f6
Show file tree
Hide file tree
Showing 5 changed files with 164 additions and 76 deletions.
42 changes: 14 additions & 28 deletions cli/src/main/java/bisq/cli/CliMain.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,21 +152,13 @@ public static void main(String[] args) {
var stub = GetBalanceGrpc.newBlockingStub(channel).withCallCredentials(credentials);
var request = GetBalanceRequest.newBuilder().build();
var reply = stub.getBalance(request);
if (reply.getBalance() == -1) {
err.println(reply.getErrorMessage());
exit(EXIT_FAILURE);
}
out.println(formatBalance(reply.getBalance()));
exit(EXIT_SUCCESS);
}
case lockwallet: {
var stub = LockWalletGrpc.newBlockingStub(channel).withCallCredentials(credentials);
var request = LockWalletRequest.newBuilder().build();
var reply = stub.lockWallet(request);
if (!reply.getSuccess()) {
err.println(reply.getErrorMessage());
exit(EXIT_FAILURE);
}
stub.lockWallet(request);
out.println("wallet locked");
exit(EXIT_SUCCESS);
}
Expand All @@ -190,11 +182,7 @@ public static void main(String[] args) {
var request = UnlockWalletRequest.newBuilder()
.setPassword(nonOptionArgs.get(1))
.setTimeout(timeout).build();
var reply = stub.unlockWallet(request);
if (!reply.getSuccess()) {
err.println(reply.getErrorMessage());
exit(EXIT_FAILURE);
}
stub.unlockWallet(request);
out.println("wallet unlocked");
exit(EXIT_SUCCESS);
}
Expand All @@ -205,11 +193,7 @@ public static void main(String[] args) {
}
var stub = RemoveWalletPasswordGrpc.newBlockingStub(channel).withCallCredentials(credentials);
var request = RemoveWalletPasswordRequest.newBuilder().setPassword(nonOptionArgs.get(1)).build();
var reply = stub.removeWalletPassword(request);
if (!reply.getSuccess()) {
err.println(reply.getErrorMessage());
exit(EXIT_FAILURE);
}
stub.removeWalletPassword(request);
out.println("wallet decrypted");
exit(EXIT_SUCCESS);
}
Expand All @@ -222,11 +206,7 @@ public static void main(String[] args) {
var request = (nonOptionArgs.size() == 3)
? SetWalletPasswordRequest.newBuilder().setPassword(nonOptionArgs.get(1)).setNewPassword(nonOptionArgs.get(2)).build()
: SetWalletPasswordRequest.newBuilder().setPassword(nonOptionArgs.get(1)).build();
var reply = stub.setWalletPassword(request);
if (!reply.getSuccess()) {
err.println(reply.getErrorMessage());
exit(EXIT_FAILURE);
}
stub.setWalletPassword(request);
out.println("wallet encrypted" + (nonOptionArgs.size() == 2 ? "" : " with new password"));
exit(EXIT_SUCCESS);
}
Expand All @@ -236,11 +216,17 @@ public static void main(String[] args) {
}
}
} catch (StatusRuntimeException ex) {
// This exception is thrown if the client-provided password credentials do not
// match the value set on the server. The actual error message is in a nested
// exception and we clean it up a bit to make it more presentable.
// The actual server error message is in a nested exception and we clean it
// up a bit to make it more presentable.
Throwable t = ex.getCause() == null ? ex : ex.getCause();
err.println("Error: " + t.getMessage().replace("UNAUTHENTICATED: ", ""));
String message = t.getMessage()
.replace("FAILED_PRECONDITION: ", "")
.replace("INTERNAL: ", "")
.replace("INVALID_ARGUMENT: ", "")
.replace("UNAUTHENTICATED: ", "")
.replace("UNAVAILABLE: ", "")
.replace("UNKNOWN: ", "");
err.println("Error: " + message);
exit(EXIT_FAILURE);
}
}
Expand Down
76 changes: 76 additions & 0 deletions core/src/main/java/bisq/core/grpc/ApiStatus.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/*
* This file is part of Bisq.
*
* Bisq is free software: you can redistribute it and/or modify it
* under the terms of the GNU Affero General Public License as published by
* the Free Software Foundation, either version 3 of the License, or (at
* your option) any later version.
*
* Bisq is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public
* License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with Bisq. If not, see <http://www.gnu.org/licenses/>.
*/

package bisq.core.grpc;

import io.grpc.Status;

/**
* Maps meaningful CoreApi error messages to more general purpose gRPC error Status codes.
* This keeps gRPC api out of CoreApi, and ensures the correct gRPC Status is sent to the
* client.
*
* @see <a href="https://github.com/grpc/grpc/blob/master/doc/statuscodes.md">gRPC Status Codes</a>
*/
enum ApiStatus {

INCORRECT_OLD_WALLET_PASSWORD(Status.INVALID_ARGUMENT, "incorrect old password"),
INCORRECT_WALLET_PASSWORD(Status.INVALID_ARGUMENT, "incorrect password"),

INTERNAL(Status.INTERNAL, "internal server error"),

OK(Status.OK, null),

UNKNOWN(Status.UNKNOWN, "unknown"),

WALLET_ALREADY_LOCKED(Status.FAILED_PRECONDITION, "wallet is already locked"),

WALLET_ENCRYPTER_NOT_AVAILABLE(Status.FAILED_PRECONDITION, "wallet encrypter is not available"),

WALLET_IS_ENCRYPTED(Status.FAILED_PRECONDITION, "wallet is encrypted with a password"),
WALLET_IS_ENCRYPTED_WITH_UNLOCK_INSTRUCTION(Status.FAILED_PRECONDITION,
"wallet is encrypted with a password; unlock it with the 'unlock \"password\" timeout' command"),

WALLET_NOT_ENCRYPTED(Status.FAILED_PRECONDITION, "wallet is not encrypted with a password"),
WALLET_NOT_AVAILABLE(Status.UNAVAILABLE, "wallet is not available");


private final Status grpcStatus;
private final String description;

ApiStatus(Status grpcStatus, String description) {
this.grpcStatus = grpcStatus;
this.description = description;
}

Status getGrpcStatus() {
return this.grpcStatus;
}

String getDescription() {
return this.description;
}

@Override
public String toString() {
return "ApiStatus{" +
"grpcStatus=" + grpcStatus +
", description='" + description + '\'' +
'}';
}
}

68 changes: 38 additions & 30 deletions core/src/main/java/bisq/core/grpc/CoreApi.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@

import javax.annotation.Nullable;

import static bisq.core.grpc.ApiStatus.*;
import static java.util.concurrent.TimeUnit.SECONDS;

/**
Expand Down Expand Up @@ -91,18 +92,21 @@ public String getVersion() {
return Version.VERSION;
}

public Tuple2<Long, String> getAvailableBalance() {
public Tuple2<Long, ApiStatus> getAvailableBalance() {
if (!walletsManager.areWalletsAvailable())
return new Tuple2<>(-1L, "Error: wallet is not available");
return new Tuple2<>(-1L, WALLET_NOT_AVAILABLE);

if (walletsManager.areWalletsEncrypted())
return new Tuple2<>(-1L, "Error: wallet is encrypted; unlock it with the 'unlock \"password\" timeout' command");
return new Tuple2<>(-1L, WALLET_IS_ENCRYPTED_WITH_UNLOCK_INSTRUCTION);

try {
long balance = balances.getAvailableBalance().get().getValue();
return new Tuple2<>(balance, "");
return new Tuple2<>(balance, OK);
} catch (Throwable t) {
return new Tuple2<>(-1L, "Error: " + t.getLocalizedMessage());
// TODO Derive new ApiStatus codes from server stack traces.
t.printStackTrace();
// TODO Fix bug causing NPE thrown by getAvailableBalance().
return new Tuple2<>(-1L, INTERNAL);
}
}

Expand Down Expand Up @@ -183,74 +187,78 @@ public void placeOffer(String offerId,

// Provided for automated wallet protection method testing, despite the
// security risks exposed by providing users the ability to decrypt their wallets.
public Tuple2<Boolean, String> removeWalletPassword(String password) {
public Tuple2<Boolean, ApiStatus> removeWalletPassword(String password) {
if (!walletsManager.areWalletsAvailable())
return new Tuple2<>(false, "Error: wallet is not available");
return new Tuple2<>(false, WALLET_NOT_AVAILABLE);

if (!walletsManager.areWalletsEncrypted())
return new Tuple2<>(false, "Error: wallet is not encrypted with a password");
return new Tuple2<>(false, WALLET_NOT_ENCRYPTED);

KeyCrypterScrypt keyCrypterScrypt = walletsManager.getKeyCrypterScrypt();
if (keyCrypterScrypt == null)
return new Tuple2<>(false, "Error: wallet encrypter is not available");
return new Tuple2<>(false, WALLET_ENCRYPTER_NOT_AVAILABLE);

KeyParameter aesKey = keyCrypterScrypt.deriveKey(password);
if (!walletsManager.checkAESKey(aesKey))
return new Tuple2<>(false, "Error: incorrect password");
return new Tuple2<>(false, INCORRECT_WALLET_PASSWORD);

walletsManager.decryptWallets(aesKey);
return new Tuple2<>(true, "");
return new Tuple2<>(true, OK);
}

public Tuple2<Boolean, String> setWalletPassword(String password, String newPassword) {
public Tuple2<Boolean, ApiStatus> setWalletPassword(String password, String newPassword) {
try {
if (!walletsManager.areWalletsAvailable())
return new Tuple2<>(false, "Error: wallet is not available");
return new Tuple2<>(false, WALLET_NOT_AVAILABLE);

KeyCrypterScrypt keyCrypterScrypt = walletsManager.getKeyCrypterScrypt();
if (keyCrypterScrypt == null)
return new Tuple2<>(false, "Error: wallet encrypter is not available");
return new Tuple2<>(false, WALLET_ENCRYPTER_NOT_AVAILABLE);

if (newPassword != null && !newPassword.isEmpty()) {
// todo validate new password
// TODO Validate new password before replacing old password.
if (!walletsManager.areWalletsEncrypted())
return new Tuple2<>(false, "Error: wallet is not encrypted with a password");
return new Tuple2<>(false, WALLET_NOT_ENCRYPTED);

KeyParameter aesKey = keyCrypterScrypt.deriveKey(password);
if (!walletsManager.checkAESKey(aesKey))
return new Tuple2<>(false, "Error: incorrect old password");
return new Tuple2<>(false, INCORRECT_OLD_WALLET_PASSWORD);

walletsManager.decryptWallets(aesKey);
aesKey = keyCrypterScrypt.deriveKey(newPassword);
walletsManager.encryptWallets(keyCrypterScrypt, aesKey);
return new Tuple2<>(true, "");
return new Tuple2<>(true, OK);
}

if (walletsManager.areWalletsEncrypted())
return new Tuple2<>(false, "Error: wallet is already encrypted");
return new Tuple2<>(false, WALLET_IS_ENCRYPTED);

// TODO Validate new password.
KeyParameter aesKey = keyCrypterScrypt.deriveKey(password);
walletsManager.encryptWallets(keyCrypterScrypt, aesKey);
return new Tuple2<>(true, "");
return new Tuple2<>(true, OK);
} catch (Throwable t) {
return new Tuple2<>(false, "Error: " + t.getLocalizedMessage());
// TODO Derive new ApiStatus codes from server stack traces.
t.printStackTrace();
return new Tuple2<>(false, INTERNAL);
}
}

public Tuple2<Boolean, String> lockWallet() {
public Tuple2<Boolean, ApiStatus> lockWallet() {
if (tempLockWalletPassword != null) {
Tuple2<Boolean, String> encrypted = setWalletPassword(tempLockWalletPassword, null);
Tuple2<Boolean, ApiStatus> encrypted = setWalletPassword(tempLockWalletPassword, null);
tempLockWalletPassword = null;
if (!encrypted.first)
if (!encrypted.second.equals(OK))
return encrypted;

return new Tuple2<>(true, "");
return new Tuple2<>(true, OK);
}
return new Tuple2<>(false, "Error: wallet is already locked");
return new Tuple2<>(false, WALLET_ALREADY_LOCKED);
}

public Tuple2<Boolean, String> unlockWallet(String password, long timeout) {
Tuple2<Boolean, String> decrypted = removeWalletPassword(password);
if (!decrypted.first)
public Tuple2<Boolean, ApiStatus> unlockWallet(String password, long timeout) {
Tuple2<Boolean, ApiStatus> decrypted = removeWalletPassword(password);
if (!decrypted.second.equals(OK))
return decrypted;

TimerTask timerTask = new TimerTask() {
Expand All @@ -267,6 +275,6 @@ public void run() {
// Cache wallet password for timeout (secs), in case
// user wants to lock the wallet for timeout expires.
tempLockWalletPassword = password;
return new Tuple2<>(true, "");
return new Tuple2<>(true, OK);
}
}
45 changes: 36 additions & 9 deletions core/src/main/java/bisq/core/grpc/GrpcServer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
import bisq.proto.grpc.UnlockWalletRequest;

import io.grpc.ServerBuilder;
import io.grpc.StatusRuntimeException;
import io.grpc.stub.StreamObserver;

import java.io.IOException;
Expand Down Expand Up @@ -114,7 +115,13 @@ class GetBalanceService extends GetBalanceGrpc.GetBalanceImplBase {
@Override
public void getBalance(GetBalanceRequest req, StreamObserver<GetBalanceReply> responseObserver) {
var result = coreApi.getAvailableBalance();
var reply = GetBalanceReply.newBuilder().setBalance(result.first).setErrorMessage(result.second).build();
if (!result.second.equals(ApiStatus.OK)) {
StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus()
.withDescription(result.second.getDescription()));
responseObserver.onError(ex);
throw ex;
}
var reply = GetBalanceReply.newBuilder().setBalance(result.first).build();
responseObserver.onNext(reply);
responseObserver.onCompleted();
}
Expand Down Expand Up @@ -191,8 +198,13 @@ class RemoveWalletPasswordService extends RemoveWalletPasswordGrpc.RemoveWalletP
public void removeWalletPassword(RemoveWalletPasswordRequest req,
StreamObserver<RemoveWalletPasswordReply> responseObserver) {
var result = coreApi.removeWalletPassword(req.getPassword());
var reply = RemoveWalletPasswordReply.newBuilder()
.setSuccess(result.first).setErrorMessage(result.second).build();
if (!result.second.equals(ApiStatus.OK)) {
StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus()
.withDescription(result.second.getDescription()));
responseObserver.onError(ex);
throw ex;
}
var reply = RemoveWalletPasswordReply.newBuilder().build();
responseObserver.onNext(reply);
responseObserver.onCompleted();
}
Expand All @@ -203,8 +215,13 @@ class SetWalletPasswordService extends SetWalletPasswordGrpc.SetWalletPasswordIm
public void setWalletPassword(SetWalletPasswordRequest req,
StreamObserver<SetWalletPasswordReply> responseObserver) {
var result = coreApi.setWalletPassword(req.getPassword(), req.getNewPassword());
var reply = SetWalletPasswordReply.newBuilder()
.setSuccess(result.first).setErrorMessage(result.second).build();
if (!result.second.equals(ApiStatus.OK)) {
StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus()
.withDescription(result.second.getDescription()));
responseObserver.onError(ex);
throw ex;
}
var reply = SetWalletPasswordReply.newBuilder().build();
responseObserver.onNext(reply);
responseObserver.onCompleted();
}
Expand All @@ -215,8 +232,13 @@ class LockWalletService extends LockWalletGrpc.LockWalletImplBase {
public void lockWallet(LockWalletRequest req,
StreamObserver<LockWalletReply> responseObserver) {
var result = coreApi.lockWallet();
var reply = LockWalletReply.newBuilder()
.setSuccess(result.first).setErrorMessage(result.second).build();
if (!result.second.equals(ApiStatus.OK)) {
StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus()
.withDescription(result.second.getDescription()));
responseObserver.onError(ex);
throw ex;
}
var reply = LockWalletReply.newBuilder().build();
responseObserver.onNext(reply);
responseObserver.onCompleted();
}
Expand All @@ -227,8 +249,13 @@ class UnlockWalletService extends UnlockWalletGrpc.UnlockWalletImplBase {
public void unlockWallet(UnlockWalletRequest req,
StreamObserver<UnlockWalletReply> responseObserver) {
var result = coreApi.unlockWallet(req.getPassword(), req.getTimeout());
var reply = UnlockWalletReply.newBuilder()
.setSuccess(result.first).setErrorMessage(result.second).build();
if (!result.second.equals(ApiStatus.OK)) {
StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus()
.withDescription(result.second.getDescription()));
responseObserver.onError(ex);
throw ex;
}
var reply = UnlockWalletReply.newBuilder().build();
responseObserver.onNext(reply);
responseObserver.onCompleted();
}
Expand Down
Loading

0 comments on commit 2a9d1f6

Please sign in to comment.