-
-
Notifications
You must be signed in to change notification settings - Fork 310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dbus Identity Property and Trust methods #1235
Conversation
@AsamK Ping for feedback |
@AsamK Are you still accepting PRs? It's a bit cumbersome to keep this patch in sync with your other changes. I can understand, if you don't have time for this currently, but would at least appreciate a quick feedback if you plan to review this at some time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think this goes in the right direction.
I've now refactored the trust command in the master branch, that way you can have less copy&pasted code in the trust method.
|
||
@Override | ||
public void trust(String number) throws Error.Failure { | ||
trustWithKey(number,null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer this method to not call trustWithKey
method, but instead move the trustIdentityAllKeys
code directly here.
Then you also don't need the null check in the trustWithKey
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm struggling a bit here. I probably would need to implement the trustIdentityAllKeys() and trustIdentityVerified() in DbusManagerImpl.java like in ManagerImpl.java. However, to create the IdentityHelper it requires the context, which is not used for DbusManagerImpl. Any hint how to correctly get the IdentityHelper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have to implement it in the DbusManagerImpl, that's just required when using signal-cli as a DBus client.
Here you can just the code from the TrustCommand class, and only adapt the thrown exceptions:
try {
final var res = m.trustIdentityAllKeys(recipient);
if (!res) {
throw new UserErrorException("Failed to set the trust for this number, make sure the number is correct.");
}
} catch (UnregisteredRecipientException e) {
throw new UserErrorException("The user " + e.getSender().getIdentifier() + " is not registered.");
}
and
final IdentityVerificationCode verificationCode;
try {
verificationCode = IdentityVerificationCode.parse(safetyNumber);
} catch (Exception e) {
throw new UserErrorException(
"Safety number has invalid format, either specify the old hex fingerprint or the new safety number");
}
try {
final var res = m.trustIdentityVerified(recipient, verificationCode);
if (!res) {
throw new UserErrorException(
"Failed to set the trust for this number, make sure the number and the fingerprint/safety number are correct.");
}
} catch (UnregisteredRecipientException e) {
throw new UserErrorException("The user " + e.getSender().getIdentifier() + " is not registered.");
}
} | ||
|
||
@Override | ||
public void trust(String number) throws Error.Failure { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need the number
parameter in the trust
methods, you can get the recipient from identity.recipient()
with RecipientIdentifier.Single.fromAddress(identity.recipient())
src/main/java/org/asamk/Signal.java
Outdated
} | ||
} | ||
|
||
@DBusProperty(name = "id", type = String.class, access = DBusProperty.Access.READ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of an id
field I'd prefer two separate number
and uuid
fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm: I would use getLegacyIdentifier() for the number and getIdentifier() for uuid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use number().orElse("")
and uuid().map(UUID::toString).orElse("")
for those.
4c8c794
to
c62a1e8
Compare
Giving this a first shot with just identities and trust - see discussion #1234
To be discussed:
Naming of methods and properties
Code for establishing trust is mostly copy&paste from TrustCommand.java - it might make sense to reuse it (but what is the best place?)