-
Notifications
You must be signed in to change notification settings - Fork 93
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
ZCS-13176 : Add mail recall message verification header #1468
base: develop
Are you sure you want to change the base?
Conversation
d156509
to
01c1c45
Compare
common/src/java/com/zimbra/common/account/ZAttrProvisioning.java
Outdated
Show resolved
Hide resolved
5a58dc6
to
7c5890f
Compare
The mail recall feature is not planned in the upcoming release so this PR should not be merged. |
b5427ef
to
4ef5e2b
Compare
*/ | ||
public static String generateRandomString() throws ServiceException { | ||
try { | ||
SecureRandom random = SecureRandom.getInstance("SHA1PRNG"); |
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.
Consider using:
SecureRandom random = new SecureRandom();
https://stackoverflow.com/questions/27622625/securerandom-with-nativeprng-vs-sha1prng
SecureRandom random = SecureRandom.getInstance("SHA1PRNG"); | ||
byte[] key = new byte[KEY_SIZE_BYTES]; | ||
random.nextBytes(key); | ||
return new String(Hex.encodeHex(key)); |
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.
Consider using Base64 for encoding to make the key more compact and URL-safe.
try { | ||
MessageDigest md = MessageDigest.getInstance(MSGVRFY_ALGORITHM_NAME); | ||
byte[] messageDigest = md.digest(input.getBytes(StandardCharsets.UTF_8)); | ||
BigInteger number = new BigInteger(1, messageDigest); |
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 convert the BigInteger to a string and then encode it to Base64. This could be simplified:
return Base64.getEncoder().encodeToString(messageDigest);
*/ | ||
public static String getMessageVerificationHeaderValue(String id, String date, String from) throws MessagingException, ServiceException { | ||
String secretKey = Provisioning.getInstance().getConfig().getFeatureMailRecallSecretKey(); | ||
String guid = (id + date + from + secretKey); |
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.
Repeated use of the +
operator can degrade performance as strings are immutable in Java. Each concatenation creates a new String object, leaving old strings for garbage collection, which can increase memory usage and slow down execution. Leverage Java 8 and above features.
String guidHash = getHashForMessageVerification(guid); | ||
String hash = MSGVRFY_HEADER_PREFIX + guidHash; | ||
return hash; | ||
} |
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.
Maybe you can use Optional.ofNullable(...).orElseThrow(...)
to ensure secretKey
is not null
and throw an exception if it is, to avoid potential null pointer and String.join(...)
for concatenation.
import java.util.LinkedList; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.*; |
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.
Don't use implicit imports
import com.zimbra.cs.account.Domain; | ||
import com.zimbra.cs.account.Identity; | ||
import com.zimbra.cs.account.Provisioning; | ||
import com.zimbra.cs.account.*; |
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.
Don't use implicit imports
Date date = new Date(); | ||
mm.setSentDate(date); | ||
|
||
String value = SecretKey.getMessageVerificationHeaderValue(mm.getMessageID(), mailDateFormat.format(date), from.getAddress()); |
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.
Make the code more expressive, and ensure it's null-safe by leveraging Java 8+ features such as Optional
, method references etc.
} | ||
} catch (ServiceException e) { | ||
throw ServiceException.FAILURE("Unable to initialize SecureRandom for mail recall", e); | ||
} |
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.
Please optimize the code to improve the readability and efficiency of the code. Consider using Java 8 and above features such as:
Optional
to wrap thevalue
andfilter
it to only proceed if it equalsTRUE
,method references
,try-with-resources
lambda expression
to encapsulate the logic for handling the secret key and its invocation, to reduce nested conditions, improve resource management.
Also, consider catching specific exceptions.
} catch (ServiceException e) { | ||
ZimbraLog.misc.warn("Encountered exception during FlushCache after creating Secret Key", e); | ||
} | ||
} |
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.
Consider using Try-with-Resources
while (hexString.length() < 64) { | ||
hexString.insert(0, '0'); | ||
} | ||
|
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 String.format("%064x", ...)
to format the BigInteger directly into a zero-padded hexadecimal string of the desired length and remove StringBuilder
.
@@ -303,6 +303,10 @@ public static ServiceException LICENSE_ERROR(String message, Throwable cause) { | |||
return new ServiceException("license error: "+message, LICENSE_ERROR, RECEIVERS_FAULT, cause); | |||
} | |||
|
|||
public static ServiceException ERROR_MESSAGE(String str, Throwable cause){ | |||
return new ServiceException("error: " + str, null, SENDERS_FAULT, cause); |
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.
Avoid using +
operator for String concatenation, consider using String.format
or formatted
if upgraded to Java 15 or above.
683be93
to
e7d900d
Compare
Requirement of ticket [ZCS-13176]
Requirements:
Solution:
hash=[string] --the algorithm used to produce the hash value in the "guid" tag. Current valid values are "SHA1" and "SHA256" [[SHA]].
guid=[base64] --the base64 encoding [[Base64]] of the hash of a globally unique ID (GUID) for the message, using the hash algorithm specified in the "hash" tag. The actual GUID (the pre-image of the hash) is kept secret by the sending side.
Testing: