Set password with token, disable password changes via patch/replace by bajtos · Pull Request #3350 · strongloop/loopback

bajtos · 2017-04-10T12:20:12Z

Description

Add User.setPassword(id, new, cb)

Implement a new method for changing user password with password-reset token but without the old password.

REST API

POST /api/users/reset-password
Authorization: your-password-reset-token-id
Content-Type: application/json

{"newPassword": "new-pass"}

JavaScript API

User.setPassword(userId, newPassword[, cb])
userInstance.setPassword(newPassword[, cb])

Implement more secure password flow

Improve the flow for setting/changing/resetting User password to make
it more secure.

Modify User.resetPassword to create a token scoped to allow
invocation of a single remote method: User.setPassword.
Scope the method User.setPassword so that regular tokens created
by User.login are not allowed to execute it.

For backwards compatibility, this new mode (flow) is enabled only
when User model setting restrictResetPasswordTokenScope is set to true.

Changing the password via User.prototype.patchAttributes
(and similar DAO methods) is no longer allowed. Applications
must call User.changePassword and ask the user to provide
the current (old) password.

For backwards compatibility, this new mode (flow) is enabled only
when User model setting rejectPasswordChangesViaPatchOrReplace is set to true.

Related issues

Connect to Tighten security for resetting password #382
Follow-up issue to write documentation, update the example repo and workspace templates: New password-reset flow: docs, examples, project template #3349

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

raymondfeng · 2017-04-10T17:33:19Z

+   * @promise
+   */
+  User.setPassword = function(userId, newPassword, options, cb) {
+    assert(!!userId, 'resetToken is a required argument');


The argument name should be resetToken instead of userId.

raymondfeng · 2017-04-10T17:35:32Z

+
+    // Make sure to use the constructor of the (sub)class
+    // where the method is invoked from (`this` instead of `User`)
+    this.findById(userId, options, (err, inst) => {


IIUC, userId should be resetToken.userId, right?

raymondfeng

Please address my comments.

raymondfeng

LGTM

ebarault

Very nice first PR leveraging new token's access scopes 👍 .
This looks good to me in general, see my comments.

ebarault · 2017-04-11T17:33:23Z

+          ttl: ttl,
+          scopes: ['reset-password'],
+        };
+        user.createAccessToken(params, onTokenCreated);


i find the createAccessToken() method signature confusing with regard to how it's called here: see here in this file

User.prototype.createAccessToken = function(ttl, options, cb) {...}

At least the jsdoc does not refer to ttl in the options argument, neither does it states the ttl argument as optional.

Also this reference to singular scope in method's options seems to be older than the scopes we are referring to here (refers to scope.js) and will confuse people.

Considering that we might want to use this options (i don't see it right now) argument to forward remote context downstream, it adds to the underlying complexity (non explicit magic under the hood) IMO (where is the appId param of options argument handled btw?)

At the minimum it should be addressed in the jsdoc.

See #3350 (comment) and 745d20a

ebarault · 2017-04-11T17:42:17Z

+      // to turn off this password-change check
+
+      // Verify that only the password is changed and nothing more or less.
+      const data = ctx.data || ctx.instance;


suggestion: group ctx.data || ctx.instance as a shared data const at the beginning of the hook as it is also used few lines above

ebarault · 2017-04-11T17:43:54Z

+      // The password can be always set when creating a new User instance
+      return next();
+    }
+    const isPasswordChange = 'password' in (ctx.data || ctx.instance);


curiosity : is it more/less efficient than doing a string search?.

What do you mean by string search? Could you paste a code snippet showing what do you have in mind?

ebarault · 2017-04-11T17:47:37Z

  }

  function givenRemoteMethodWithCustomScope() {
+    // Delete any previosly registered instance of the method "scoped"


I am afraid I don't see the typo, could you please be more specific?

previosly-->previously

Ah, I see it now. Thank you @loay 🙇

ebarault · 2017-04-11T18:04:39Z

+
+    if (ctx.options.setPassword) {
+      // This is the option used by `setPassword()` API
+      // to turn off this password-change check


i find the comment ambiguous
why not something like
"This is the option set by setPassword() API when calling this.patchAttritubes() to change user's password" ?

ebarault · 2017-04-11T18:13:36Z

+    });
+  });
+
+  function givenAppWithModernFlow() {


Can we find something more explicit than Modern vs. Legacy? (what if we change again)

I see your point. Do you have any proposal in mind?

Note that the User setting is called legacyPasswordFlow right now. This setting controls whether:

POST /api/users/reset-password is scoped to ['reset-password']

The token created by POST /api/users/reset is granted ['reset-password'] scope

User's "before save" hooks rejects attempts to change the password via patch/replace

I have difficulties coming with a descriptive name that would cover all of that.

we had a similar discussion a few month ago in another PR

Do you have arguments against naming such options and tests according to versioning and add descriptions in the changes log? (a dedicated page on options is required at this point IMO)
e.g. passwordFlow@2.y.z, passwordFlow@3.6.0 (the version being the first LB version introducing the feature)

Do you have arguments against naming such options and tests according to versioning and add descriptions in the changes log? (a dedicated page on options is required at this point IMO)
e.g. passwordFlow@2.y.z, passwordFlow@3.6.0 (the version being the first LB version introducing the feature)

I am not sure if I understand you correctly. What is the model setting name and what are the values that you are proposing?

In general, I prefer feature flags that describe the behaviour of the application. If we cannot come up with a descriptive flag name for this situation, then I guess the second best option is to introduce two feature flags:

scopePasswordResetToken controlling the behaviour of POST /api/users/reset-password and POST /api/users/reset

rejectPasswordChangesViaPatchOrReplace or perhaps allowInsecurePasswordUpdates to enable the "before save" hook

Thoughts?

ebarault · 2017-04-11T18:23:43Z

+            .set('Authorization', info.accessToken.id)
+            .expect(200));
+      });
+


IIUC these tests are for the legacy call flow.

I don't see the two opposite tests for the new call flow that

creates token that prevent patching User with new password

creates token that allow calling setPassword

creates token that prevent calling other methods than setPassword

these tests are for the legacy call flow.

Yes, I am keeping them here to ensure backward-compatibility is preserved. The test cases you have pointed out are covered by test/user-password.test.js:

creates token that prevent patching User with new password

See

loopback/test/user-password.test.js

Lines 30 to 32 in 745d20a

it('denies patching user password', () => {

return patchPassword(regularToken).expect(401);

});

creates token that allow calling setPassword

loopback/test/user-password.test.js

Lines 58 to 60 in 745d20a

it('allows resetting user password', () => {

return resetPassword(resetToken).expect(204);

});

creates token that prevent calling other methods than setPassword

loopback/test/user-password.test.js

Lines 46 to 56 in 745d20a

it('denies patching user name', () => {

return changeName(resetToken).expect(401);

});

it('denies patching user password', () => {

return patchPassword(resetToken).expect(401);

});

it('denies changing user password', () => {

return changePassword(resetToken).expect(401);

});

ok, thanks for pointing out these tests
to ease further reading, could you add context blocks to segregate tests with "legacy" password flow and tests with "modern" password flow?, as in user-password.test.js

Well, I consider the tests in user.test.js as verifying the "default" password flow. In that light, do you still think I should move them to a context('default password flow') block?

ebarault · 2017-04-11T18:28:17Z

+        // test passed
+      });
+    });
+  });


very compact style 👍

ebarault · 2017-04-11T18:30:18Z

+    });
+
+    it('injects reset password options from remoting context', function() {
+      const User = app.models.User;


forwards vs. injects? (we shall decide which term to use)

In the light of the discussion under #3350 (comment), maybe a better test name would be:

it('configures "options" argument to be injected from remoting context'

Thoughts?

i reviewed your other comment, let's keep it simple: i agree with inject for the root call, and forwards for downstream calls

ebarault · 2017-04-11T18:31:31Z

+        });
+    });
+
+    it('forwards the "options" argument', () => {


see my previous comment, here it's: forwards

~~I am afraid I don't understand. Which previous comment do you mean?~~

I think I found the other comment: #3350 (comment)

Let me explain why I think we should keep using both "forwards" and "injects":

The test it forwards the "options" argument verifies that the implementation of User.setPassword is forwarding theoptions argument whenever calling other User and DAO methods, to ensure that context can be correctly propagated.

The second test, "it injects reset password options from remoting context" verifies that remoting metadata of setPassword provides correct configuration for the options argument, a configuration instructing loopback/strong-remoting to inject options built from the request context.

In my mind, these two test cases are different, and therefore it makes sense to me to use different verbs to describe them.

bajtos · 2017-04-13T14:08:08Z

#3350 (comment)

i find the createAccessToken() method signature confusing (...)

Good point 💯

I am proposing the change the signature of createAccessToken in a different way then. Let's keep the currently supported forms:

createAccessToken(ttl, cb)
createAccessToken(ttl, options, cb);
createAccessToken(options, cb);

and add a new form that accepts a data object as the first arg:

createAccessToken(data, options, cb);

thoughts?

@raymondfeng

bajtos · 2017-04-13T14:11:39Z

@ebarault thanks for your thoughtful review! I have addressed the first two comments, PTAL. I'll work on the remaining comments later. Most likely I won't get to this patch until next week, because Good Friday is a Public Holiday here in CZ.

Fix the code builing a scoped method to correctly handle the case when the setup method is called twice and the previously defined method has to be overriden with new remoting metadata.

bajtos · 2017-04-18T11:04:44Z

@ebarault I addressed all of your review comments, either by changing the relevant code, or by starting a discussion. Could you PTAL again?

ebarault

@bajtos : i generally approve your changes. Please see some further comments below.

Options naming convention (modern, legacy, ...) would maybe require further discussion. I let up to you whether you'd like to wait for my feedback or land with a chosen convention of your taste.

ebarault · 2017-04-19T08:40:52Z

+   * or an object with token properties to set (see below).
+   * @property {Number} [ttl] The requested ttl
+   * @property {String[]} [scopes] The access scopes granted to the token.
+   * @param {Object} [properties] Additional options (e.g. the context).


LGTM 👍

regarding @param {Object} [properties] Additional options (e.g. the context).

it should be @param {Object} [options] ...

we should come up with a final (unique) description for the options object where it's meant to possibly convey remote context options. Maybe something like:
Additional options including remoting context

ebarault · 2017-04-19T08:46:40Z

+   *
+   * @param {*} userId Id of the user changing the password
+   * @param {string} newPassword The new password to use.
+   * @param {object} [options]


missing description for options object

ebarault · 2017-04-19T08:47:07Z

+   * current password or a password-reset token.
+   *
+   * @param {string} newPassword The new password to use.
+   * @param {object} [options]


missing description for options object

ebarault · 2017-04-19T08:56:25Z

+
+    // We need to modify options passed to patchAttributes, but we don't want
+    // to modify the original options object passed to us by setPassword caller
+    options = Object.assign({}, options);


i'm probably missing something here, but as you assign the result of Object.assign({}, options) back to options, i don't see how this does not modify the original options object.

Consider the following code:

const options = { foo: 'bar' }; user.setPassword('pass', options, cb); console.log(options);

In my proposed implementation, the snippet prints { foo: 'bar' }.

Without the Object.assign call, the snippet would print { foo: 'bar', setPassword: true }.

In other words, the code inside setPassword is changing the value of the options argument (the reference to the options object), but it does not mutate the original object referenced by the initial options value.

I hope my comment have clarified the problem, please do keep asking if there is anything still not clear yet.

ebarault · 2017-04-19T09:02:17Z

+    });
+  });
+
+  function givenAppWithModernFlow() {


we had a similar discussion a few month ago in another PR

Do you have arguments against naming such options and tests according to versioning and add descriptions in the changes log? (a dedicated page on options is required at this point IMO)
e.g. passwordFlow@2.y.z, passwordFlow@3.6.0 (the version being the first LB version introducing the feature)

ebarault · 2017-04-19T09:09:12Z

+    });
+
+    it('injects reset password options from remoting context', function() {
+      const User = app.models.User;


i reviewed your other comment, let's keep it simple: i agree with inject for the root call, and forwards for downstream calls

ebarault · 2017-04-19T09:10:56Z

+        });
+    });
+
+    it('forwards the "options" argument', () => {


ebarault · 2017-04-19T09:23:26Z

+            .set('Authorization', info.accessToken.id)
+            .expect(200));
+      });
+


ok, thanks for pointing out these tests
to ease further reading, could you add context blocks to segregate tests with "legacy" password flow and tests with "modern" password flow?, as in user-password.test.js

bajtos · 2017-04-19T11:26:47Z

@ebarault thank you for another review, I think we are getting close :)

AFAICT, there are three outstanding discussion points:

Feature flag naming - Set password with token, disable password changes via patch/replace #3350 (comment)
Context blocks in user.test.js - Set password with token, disable password changes via patch/replace #3350 (comment)
Object.assign confusion - Set password with token, disable password changes via patch/replace #3350 (comment)

ebarault · 2017-04-19T12:22:57Z

Feature flag naming - #3350 (comment)

my proposal was to version the features such as password flow such as: passwordFlow@3.6.0 : true, and describe the content elsewhere. But one in all i prefer your proposal to split favor feature flags that describe the behaviour of the application. a similar situations we should use several feature flags:

scopePasswordResetToken controlling the behaviour of POST /api/users/reset-password and POST /api/users/reset

what about restrictResetPasswordTokenScope ? at least something with an explicit verb

rejectPasswordChangesViaPatchOrReplace or perhaps allowInsecurePasswordUpdates to enable the "before save" hook

rejectPasswordChangesViaPatchOrReplace 👍

ebarault · 2017-04-19T12:31:03Z

Context blocks in user.test.js - #3350 (comment)

hmm, so when the default behavior comes to be the feature flagged one ("modern password flow") we should review the code in user.test.js to match the correct behavior? I believe i'm not aware enough of the logic that leads to gather some tests in user.test.js, compared to dedicating other files to specific features. My only concern is about code maintenance when going to next major release. I leave this to you to choose the approach that minimize the code maintenance IYO.

ebarault · 2017-04-19T12:40:28Z

Object.assign confusion - #3350 (comment)

I see your point, but i'd need to do some tests on my side to be 100% convinced that assigning the options var inside the function block does not modify the original options arg.
So as your code is nevertheless 100% safe, let's close this discussion and keep your proposal.

bajtos · 2017-04-19T13:40:18Z

Feature flag naming

what about restrictResetPasswordTokenScope
rejectPasswordChangesViaPatchOrReplace

Done in 3dc012d

Context blocks in user.test.js

hmm, so when the default behavior comes to be the feature flagged one ("modern password flow") we should review the code in user.test.js to match the correct behavior?

Yes.

I believe i'm not aware enough of the logic that leads to gather some tests in user.test.js, compared to dedicating other files to specific features.

Well, I don't think there is much logic here. I am keeping existing tests to ensure full backwards compatibility is preserved.

My only concern is about code maintenance when going to next major release.

I think we have two options:

Clean the tests right now. Unless I temporarily change the default value of these flags, I may miss some tests that would break when defaults are changed in the future. When moving the tests around, one may accidentally change their behaviour, which would go against the goal of verifying backwards compatibility.
Defer test cleanup until we are changing the defaults, even though it will make such change more costly. OTOH if we never change the default, then we saved time.

I leave this to you to choose the approach that minimize the code maintenance IYO.

Let's leave test cleanup for later then.

Object.assign confusion

So as your code is nevertheless 100% safe, let's close this discussion and keep your proposal.

Sounds good 👍

@ebarault Could you please review the latest changes in 3dc012d? I think that's the last step before this patch can be landed.

loay

LGTM

loay · 2017-04-19T16:56:23Z

@slnode test please

ebarault · 2017-04-19T18:22:57Z

OTOH if we never change the default, then we saved time

oh gosh, let's hope we'll not end there !! ;-)

ebarault

👍 your changes looks good to me, let's land it ! 🎉

could you please open a new issue/PR so that we don't forget to add these 2 new options in the new project templates ?

loay · 2017-04-19T19:43:23Z

@slnode test please

superkhau

LGTM with some minor nits.

superkhau · 2017-04-19T20:06:00Z

+var loopback = require('../');
+const Promise = require('bluebird');
+var request = require('supertest');
+const waitForEvent = require('./helpers/wait-for-event');


Why vars? Seems like we can use all consts.

superkhau · 2017-04-19T20:08:14Z

+    }
+  });
+
+  context('rejectPasswordChangesViaPatchOrReplace', () => {


normal sentence for consistency with context names below? 'reject password changes via patchOrReplace'?

superkhau · 2017-04-19T20:08:36Z

+
+  let app, User, testUser, regularToken, resetToken;
+
+  context('restrictResetPasswordTokenScope', () => {


'restrict reset password token scope'?

superkhau · 2017-04-19T20:09:20Z

+    }
+  });
+
+  context('no limitations', () => {


with no restrictions/limitations? (consistent wording with line 100)

The idea is that this context disables all new feature flags. I am going to use context('all feature flags disabled')

superkhau · 2017-04-19T20:10:02Z

+  context('no limitations', () => {
+    beforeEach(givenAppWithNoRestrictions);
+
+    context('with regular access token', () => {


using regular access tokens

superkhau · 2017-04-19T20:11:19Z

        });
    });
+
+    it('resets user\'s password', function() {


superkhau · 2017-04-19T20:11:48Z

+        .then(isMatch => expect(isMatch, 'user has new password').to.be.true());
+    });
+
+    it('rejects unauthenticated reset password request', function() {


rejects unauthenticated password reset requests

superkhau · 2017-04-19T20:12:10Z

+        .expect(401);
+    });
+
+    it('injects reset password options from remoting context', function() {


uses password reset options provided by the remoting context

superkhau · 2017-04-19T20:13:57Z

+      const User = app.models.User;
+      const credentials = {email: 'inject-reset@example.com', password: 'pass'};
+
+      let injectedOptions;


replace injected with remotingContextOptions? and below

Let's use observedOptions.

superkhau · 2017-04-19T20:14:40Z

+        });
+    });
+
+    it('fails with 401 for unknown user', () => {


unknown users

loay · 2017-04-19T20:38:42Z

linting issues should be fixed now on gateway-director-bluemix@develop .. the next build should be a success.

bajtos · 2017-04-20T08:05:04Z

@ebarault

could you please open a new issue/PR so that we don't forget to add these 2 new options in the new project templates ?

I have already done that, see #3349

bajtos · 2017-04-20T08:05:21Z

@loay

linting issues should be fixed now on gateway-director-bluemix@develop .. the next build should be a success.

Thanks! 🙇

bajtos · 2017-04-20T08:14:22Z

@superkhau thank you for the detailed review, I addressed your comments in b1aeb8c.

Implement a new method for changing user password with password-reset token but without the old password. REST API POST /api/users/reset-password Authorization: your-password-reset-token-id Content-Type: application/json {"newPassword": "new-pass"} JavaScript API User.setPassword(userId, newPassword[, cb]) userInstance.setPassword(newPassword[, cb]) Note: the new REST endpoint is not protected by scopes yet, therefore any valid access token can invoke it (similarly to how any valid access token can change the password via PATCH /api/users/:id).

Improve the flow for setting/changing/resetting User password to make it more secure. 1. Modify `User.resetPassword` to create a token scoped to allow invocation of a single remote method: `User.setPassword`. 2. Scope the method `User.setPassword` so that regular tokens created by `User.login` are not allowed to execute it. For backwards compatibility, this new mode (flow) is enabled only when User model setting `restrictResetPasswordTokenScope` is set to `true`. 3. Changing the password via `User.prototype.patchAttributes` (and similar DAO methods) is no longer allowed. Applications must call `User.changePassword` and ask the user to provide the current (old) password. For backwards compatibility, this new mode (flow) is enabled only when User model setting `rejectPasswordChangesViaPatchOrReplace` is set to `true`.

bajtos · 2017-04-20T08:46:53Z

[cis-jenkins] downstream: loopback-getting-started@master — Failed! (c5ca2e1)

That was a (temporary?) CI issue:

FATAL: java.io.IOException: Unexpected termination of the channel
java.io.EOFException
	at java.io.ObjectInputStream$PeekInputStream.readFully(ObjectInputStream.java:2353)

bajtos · 2017-04-20T08:48:03Z

Landed. Thank you everybody for keeping the fast pace, allowing me to iterate on your feedback and get this complex change landed in less than two weeks 🎉

bajtos requested review from clark0x, ebarault, raymondfeng and superkhau April 10, 2017 12:20

bajtos added review needs-priority #review and removed needs-priority labels Apr 10, 2017

offlinehacker mentioned this pull request Apr 10, 2017

Tighten security for resetting password #382

Closed

7 tasks

bajtos requested a review from loay April 10, 2017 12:43

bajtos added feature major security labels Apr 10, 2017

raymondfeng reviewed Apr 10, 2017

View reviewed changes

raymondfeng requested changes Apr 10, 2017

View reviewed changes

bajtos force-pushed the feature/set-password-with-token branch from 25d1ce5 to b0de97d Compare April 11, 2017 15:23

raymondfeng approved these changes Apr 11, 2017

View reviewed changes

ebarault requested changes Apr 11, 2017

View reviewed changes

bajtos added 2 commits April 18, 2017 13:01

Add missing tests for reset password flow

9c63abe

Fix method setup in authorization-scopes.test

d95ec66

Fix the code builing a scoped method to correctly handle the case when the setup method is called twice and the previously defined method has to be overriden with new remoting metadata.

bajtos force-pushed the feature/set-password-with-token branch from 745d20a to a5ea90e Compare April 18, 2017 11:02

bajtos mentioned this pull request Apr 18, 2017

Describe Authorization scopes loopbackio/loopback.io#338

Merged

ebarault approved these changes Apr 19, 2017

View reviewed changes

loay approved these changes Apr 19, 2017

View reviewed changes

ebarault approved these changes Apr 19, 2017

View reviewed changes

superkhau approved these changes Apr 19, 2017

View reviewed changes

bajtos force-pushed the feature/set-password-with-token branch from b1aeb8c to 662b288 Compare April 20, 2017 08:20

bajtos force-pushed the feature/set-password-with-token branch from 662b288 to c5ca2e1 Compare April 20, 2017 08:22

bajtos added this to the Sprint 34 milestone Apr 20, 2017

bajtos merged commit b96605c into master Apr 20, 2017

bajtos deleted the feature/set-password-with-token branch April 20, 2017 08:47

bajtos removed the #review label Apr 20, 2017

ebarault mentioned this pull request May 13, 2017

Missing documentation on how to migrate codebase to support AccessToken scopes ? #3395

Closed

bajtos mentioned this pull request May 30, 2017

New password flow and email reverification strongloop/loopback-example-user-management#58

Merged

	it('denies patching user password', () => {
	return patchPassword(regularToken).expect(401);
	});

	it('allows resetting user password', () => {
	return resetPassword(resetToken).expect(204);
	});

	it('denies patching user name', () => {
	return changeName(resetToken).expect(401);
	});

	it('denies patching user password', () => {
	return patchPassword(resetToken).expect(401);
	});

	it('denies changing user password', () => {
	return changePassword(resetToken).expect(401);
	});


		let app, User, testUser, regularToken, resetToken;

		context('restrictResetPasswordTokenScope', () => {

Uh oh!

Conversation

bajtos commented Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Checklist

Uh oh!

raymondfeng Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymondfeng left a comment

Choose a reason for hiding this comment

Uh oh!

raymondfeng left a comment

Choose a reason for hiding this comment

Uh oh!

ebarault left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebarault Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos Apr 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

bajtos commented Apr 10, 2017 •

edited

Loading

raymondfeng Apr 10, 2017 •

edited

Loading

ebarault Apr 19, 2017 •

edited

Loading

bajtos Apr 18, 2017 •

edited

Loading

bajtos commented Apr 13, 2017 •

edited

Loading

ebarault left a comment •

edited

Loading

ebarault Apr 19, 2017 •

edited

Loading