Login with password #10

MrCustomizer · 2017-10-22T14:57:35Z

No description provided.

maxidorius

One big issue with this current code: login is mandatory and/or immutable.

The CS spec doesn't require login for some endpoints, therefore login() should exist and take a _MatrixCredentials (or similar) argument to perform login and assign internal values (token, device ID, etc.) properly.

maxidorius · 2017-10-22T21:07:30Z

src/main/java/io/kamax/matrix/client/AMatrixHttpClient.java

@@ -61,6 +64,9 @@

    public AMatrixHttpClient(MatrixClientContext context) {
        this.context = context;
+        if (!context.getToken().isPresent()) {


The CS API don't require login everywhere, like fetching media or getting the list of supported protocol version. This cannot be made mandatory.

maxidorius · 2017-10-22T21:07:54Z

src/main/java/io/kamax/matrix/client/AMatrixHttpClient.java

@@ -75,14 +81,27 @@ public _MatrixHomeserver getHomeserver() {

    @Override
    public String getAccessToken() {
-        return context.getToken();
+        return context.getToken()
+                .orElseThrow(() -> new IllegalStateException("This method can only be used with a valid token."));


The CS API don't require login everywhere, like fetching media or getting the list of supported protocol version. This cannot be made mandatory.

maxidorius · 2017-10-22T21:09:16Z

src/main/java/io/kamax/matrix/client/AMatrixHttpClient.java

    }

    protected URI getClientPath(String action) {
        try {
-            return getClientPathBuilder(action).build();
+            return getPathBuilder(getPath("client", "r0", action)).build();


Why change this for the login feature?

maxidorius · 2017-10-22T21:09:20Z

src/main/java/io/kamax/matrix/client/AMatrixHttpClient.java

        } catch (URISyntaxException e) {
            throw new IllegalArgumentException(e);
        }
    }

    protected URI getMediaPath(String action) {
        try {
-            return getMediaPathBuilder(action).build();
+            return getPathBuilder(getPath("media", "v1", action)).build();


Why change this for the login feature?

maxidorius · 2017-10-22T21:10:31Z

src/main/java/io/kamax/matrix/client/AMatrixHttpClient.java

+
+    private void login() {
+        HttpPost request = new HttpPost(getLoginClientPath());
+        _MatrixID user = context.getUser();


Login doesn't require a Matrix ID, only a localpart. This is a signed of flowed implementation.

maxidorius · 2017-10-22T21:15:41Z

src/main/java/io/kamax/matrix/client/MatrixHttpRoom.java

@@ -145,7 +136,7 @@ public void invite(_MatrixID mxId) {

    @Override
    public List<_MatrixID> getJoinedUsers() {
-        URI path = getClientPath("/rooms/{roomId}/joined_members");
+        URI path = getClientPath("/rooms/" + roomId + "/joined_members");


Why change this for the login feature?

maxidorius · 2017-10-22T21:17:26Z

src/main/java/io/kamax/matrix/client/_MatrixClientRaw.java

@@ -33,4 +35,8 @@

    _MatrixID getUser();

+    Optional<String> getDeviceId();
+
+    void logout();


Not sure this belongs in _MatrixClientRaw... _MatrixClient might be better. I wish I added javadoc! I remember I split for a reason, but not the reason... let's discuss...

maxidorius · 2017-10-22T21:17:34Z

src/main/java/io/kamax/matrix/client/regular/MatrixHttpClient.java

@@ -47,7 +43,7 @@ protected _MatrixID getMatrixId(String localpart) {

    @Override
    public void setDisplayName(String name) {
-        URI path = getClientPath("/profile/{userId}/displayname");
+        URI path = getClientPath("/profile/" + context.getUser().getId() + "/displayname");


Why change this for the login feature?

maxidorius · 2017-10-22T21:17:58Z

src/main/java/io/kamax/matrix/hs/_MatrixHomeserver.java


 public interface _MatrixHomeserver {

    String getDomain();

-    URIBuilder getClientEndpoint();
+    URI getClientEndpoint();


Why change this for the login feature?

maxidorius · 2017-10-22T21:18:35Z

src/main/java/io/kamax/matrix/json/LoginPostBody.java

+        return user;
+    }
+
+    public String getPassword() {


Security issue, should not be able to just get the password.

maxidorius · 2017-10-24T01:42:36Z

src/main/java/io/kamax/matrix/client/AMatrixHttpClient.java

@@ -258,6 +272,16 @@ protected URI getClientPath(String action) {
        }
    }

+    protected URI getMediaPathWithAccessToken(String action) {


Has nothing to do with the login feature

No media retrieval requires a token if following the CS spec. While I would definitely add this down the road, it's out of scope here and premature I think

maxidorius · 2017-10-24T01:43:30Z

src/main/java/io/kamax/matrix/client/MatrixHttpContent.java

@@ -59,7 +59,7 @@ private synchronized void load() {
            if (!StringUtils.equalsIgnoreCase("mxc", address.getScheme())) {
                log.debug("{} is not a supported protocol for avatars, ignoring", address.getScheme());
            } else {
-                URI path = getMediaPath("/download/" + address.getHost() + address.getPath());
+                URI path = getMediaPathWithAccessToken("/download/" + address.getHost() + address.getPath());


Not related to login feature

maxidorius · 2017-10-24T01:44:39Z

src/main/java/io/kamax/matrix/client/_MatrixClient.java

@@ -32,4 +34,10 @@

    _MatrixUser getUser(_MatrixID mxId);


Shouldn't this be optional as well now?

maxidorius · 2017-10-24T01:46:30Z

src/main/java/io/kamax/matrix/client/_MatrixClientRaw.java


-    _MatrixID getUser();
+    String getAccessTokenOrThrow();


Not sure this should be in the interface. While the implementation is useful, seems like bloat for "external" use with a specific exception forced.

maxidorius · 2017-10-24T01:48:38Z

src/main/java/io/kamax/matrix/client/regular/MatrixHttpClient.java

@@ -47,7 +47,7 @@ protected _MatrixID getMatrixId(String localpart) {

    @Override
    public void setDisplayName(String name) {
-        URI path = getClientPath("/profile/{userId}/displayname");
+        URI path = getClientPathWithAccessToken("/profile/" + context.getUser().getId() + "/displayname");


{userId} should remain, just like {roomId} remained

maxidorius · 2017-10-24T02:01:10Z

src/test/java/io/kamax/matrix/client/regular/MatrixHttpClientTest.java

+        MatrixPasswordLoginCredentials credentials = new MatrixPasswordLoginCredentials(user.getLocalPart(), password);
+        client.login(credentials);
+
+        assertEquals(deviceId, client.getDeviceId().get());


See loginWithDeviceId() comment

maxidorius · 2017-10-24T02:03:13Z

src/test/java/io/kamax/matrix/client/regular/MatrixHttpClientTest.java

+        client.login(credentials);
+
+        assertEquals(deviceId, client.getDeviceId().get());
+    }


What about validating the required fields are not blank (StringUtils.isNotBlank()) as per spec:

Matrix ID

Access token

Device ID

Homeserver hostname

maxidorius · 2017-10-24T02:03:44Z

src/test/java/io/kamax/matrix/client/regular/MatrixHttpClientTest.java

+
+        stubFor(post(urlEqualTo(logoutUrl)));
+        client.logout();
+    }


See loginWithDeviceId() comment

maxidorius · 2017-10-24T02:03:48Z

src/test/java/io/kamax/matrix/client/regular/MatrixHttpClientTest.java

+                .willReturn(aResponse().withStatus(200)
+                        .withBody("{\"user_id\": \"" + user.getId() + "\"," + //
+                                "\"access_token\": \"" + testToken + "\"," + //
+                                "\"home_server\": \"" + new MatrixHomeserver(domain, baseUrl) + "\"," + //


See loginWithDeviceId() comment

maxidorius · 2017-10-24T02:03:56Z

src/test/java/io/kamax/matrix/client/regular/MatrixHttpClientTest.java

+        MatrixPasswordLoginCredentials credentials = new MatrixPasswordLoginCredentials(user.getLocalPart(), password);
+        client.login(credentials);
+
+        assertEquals(deviceId, client.getDeviceId().get());


See loginWithDeviceId() comment

Arne Augenstein added 6 commits October 21, 2017 19:11

Preparations for login capability

cd114dc

Add login and logout capability

9536e91

Refactor method names

ba67820

Restore original method name

c52f0b2

Restore original method name (reverted from commit c52f0b2)

9e6e14e

Add test case for failed login (wrong password)

00cafb5

maxidorius suggested changes Oct 22, 2017

View reviewed changes

Arne Augenstein added 6 commits October 23, 2017 19:30

Fix some of the points mentioned in the review

8f17ad9

Refactor handling of context, client and login credentials

c3d6afd

Only use local part in credentials

dc439f6

Remove getters

b258f95

Add user to context after login

c177d8e

Code cosmetics

76f8372

maxidorius suggested changes Oct 24, 2017

View reviewed changes

Fix the issues mentioned in the review

f8fb978

maxidorius approved these changes Oct 25, 2017

View reviewed changes

maxidorius merged commit faa050a into kamax-matrix:master Oct 25, 2017

MrCustomizer deleted the loginWithPassword branch November 27, 2017 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Login with password #10

Login with password #10

MrCustomizer commented Oct 22, 2017

maxidorius left a comment

maxidorius Oct 22, 2017

maxidorius Oct 22, 2017

maxidorius Oct 22, 2017

maxidorius Oct 22, 2017

maxidorius Oct 22, 2017

maxidorius Oct 22, 2017

maxidorius Oct 22, 2017

maxidorius Oct 22, 2017

maxidorius Oct 22, 2017

maxidorius Oct 22, 2017

maxidorius Oct 24, 2017

maxidorius Oct 24, 2017

maxidorius Oct 24, 2017

maxidorius Oct 24, 2017

maxidorius Oct 24, 2017

maxidorius Oct 24, 2017

maxidorius Oct 24, 2017 •

edited

Loading

maxidorius Oct 24, 2017

maxidorius Oct 24, 2017

maxidorius Oct 24, 2017

Login with password #10

Login with password #10

Conversation

MrCustomizer commented Oct 22, 2017

maxidorius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxidorius Oct 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxidorius Oct 24, 2017 •

edited

Loading