[bisq-network/bisq] Security framework for the API (#2461)

Manfred Karrer notifications at github.com
Sun Feb 24 16:36:51 UTC 2019


ManfredKarrer requested changes on this pull request.



> +package bisq.api.http.service.auth;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.UUID;
+
+public class TokenRegistry {
+
+    public static final long TTL = 30 * 60 * 1000;
+
+    private final TimeProvider timeProvider;
+    private final RandomStringGenerator randomStringGenerator;
+    private Map<String, Long> tokens = new HashMap<>();
+
+    public TokenRegistry() {
+        this(() -> UUID.randomUUID().toString(), System::currentTimeMillis);

I think you have to use SecureRandom for ryptographically secure randomness. 

> +import java.util.UUID;
+
+public class TokenRegistry {
+
+    public static final long TTL = 30 * 60 * 1000;
+
+    private final TimeProvider timeProvider;
+    private final RandomStringGenerator randomStringGenerator;
+    private Map<String, Long> tokens = new HashMap<>();
+
+    public TokenRegistry() {
+        this(() -> UUID.randomUUID().toString(), System::currentTimeMillis);
+    }
+
+    public TokenRegistry(RandomStringGenerator randomStringGenerator, TimeProvider timeProvider) {
+        this.timeProvider = timeProvider;

I think you have to use SecureRandom for ryptographically secure randomness. 

> +
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+
+
+import com.github.javafaker.Faker;

I dont find that class in the source code at https://github.com/Javafaker. 
Do we really need that? It is one of those examples where I don't want to get new dependencies (unknown developer, small functionality)

> +import com.github.javafaker.Faker;
+
+public class ApiPasswordManagerTest {
+
+    @Rule
+    public ExpectedException expectedException = ExpectedException.none();
+
+    private String dataDir;
+    private BisqEnvironment bisqEnvironmentMock;
+    private TokenRegistry tokenRegistryMock;
+    private ApiPasswordManager apiPasswordManager;
+
+    private static String getRandomPasswordDifferentThan(String otherPassword) {
+        String newPassword;
+        do {
+            newPassword = Faker.instance().internet().password();

What is that supposed to do? internet() is a weird method name...
As said above I prefer to not add that dependency.

> +
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+
+
+import com.github.javafaker.Faker;

Ah sorry, just saw its a test class. So then I dont care so much about that dependency anymore, but still would like to know better what the class provides.

> +import com.github.javafaker.Faker;
+
+public class ApiPasswordManagerTest {
+
+    @Rule
+    public ExpectedException expectedException = ExpectedException.none();
+
+    private String dataDir;
+    private BisqEnvironment bisqEnvironmentMock;
+    private TokenRegistry tokenRegistryMock;
+    private ApiPasswordManager apiPasswordManager;
+
+    private static String getRandomPasswordDifferentThan(String otherPassword) {
+        String newPassword;
+        do {
+            newPassword = Faker.instance().internet().password();

Ah sorry, just saw its a test class. So then I dont care so much about that dependency anymore, but still would like to know better what the class provides.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/2461#pullrequestreview-207160069
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20190224/aa364055/attachment.html>


More information about the bisq-github mailing list