Add tests

[menzow]

Hey @homakov,

We talked a bit through twitter yesterday and thought I might as well elaborate a bit more on some of my issues with the project. I'll post each of them separately so they can be fixed and tracked.

Separation of Concerns

Spec and Implementation

My most pressing point is the confusion between specification and implementation. You wrote a medium post about a protocol; but delivered a repo with an implementation. Adding to that confusion is the complete lack of structure within the example implementation.

If SecureLogin is a protocol; I would advice treating it as such and create a clear separation between specification and implementation. Spec should get a repo, and each implementation should get a repo. Changes and discussions to the protocol are relevant for everyone. But someone figuring out why Cordova 6.4 didn't compile doesn't interest someone who runs everything in GO.

Related to: https://github.com/sakurity/securelogin/issues/24, https://github.com/sakurity/securelogin/issues/34

Repo and Code

This brings me to my second point; presentation of implementation. In general it benefits everyone if code is consistently structured and follows common programming patterns. In case of a public repo it gets even more important to follow a consistent format.

First, please read up on the best practices for Cordova and Electron. How do you structure the base of your project? What are the distribution channels and how to correctly track dependencies? After that I'd suggest implementing something like Angular, React, Ember or Vue.js for your Cordova app. When you pick a framework, make sure you either follow best practices or document your own practices. This way people can navigate through your project without being familiar with it.

Put SL into modules

Like I said in spec; implementation must be strict but is irrelevant. It follows the same logic no matter what language you use. Because of this I would advice providing modules for popular languages which expose the SL api in the language of choice. If you maintain these modules you limit the effort required for implementing SL and also keep some control over the rollout of feature changes / bug fixes.

Of course each module should get its own repo with bug tracking, documentation and examples

Testing

Please write tests.. You're working on a serious project that requires 100% stability before anything is pushed to master. Imagine users losing the ability to login into existing websites, and at the same time registering new profiles with a wrongly determined password. You'll end up with a scenario where you can't roll back.

---

Have a nice day

[homakov]

Spec and Implementation

Spec is coming, we finally figured out possible implementation issues and after that ready to finalize it.

First, please read up on the best practices for Cordova and Electron.

Are there any? I thought their job is to work on existing HTML, not enforce their conventions.

How do you structure the base of your project? What are the distribution channels and how to correctly track dependencies?

Will work on it.

After that I'd suggest implementing something like Angular, React, Ember or Vue.js for your Cordova app. When you pick a framework, make sure you either follow best practices or document your own practices. This way people can navigate through your project without being familiar with it.

We don't use framework on purpose:

1) the app is small and doesn't intend to do anything beyond authentication. So benefits of full blown framework over some DOM manipulations are low

2) It's easier to audit for anyone not just for a programmer in respective React / Angular etc framework.

We also will refactor code and follow standardjs in near future.

Put SL into modules

Leftpad problem is bothering me, and the fact that we can push code to many apps just modifying a gem is scary. Once protocol is finalized, it's safe to copy-paste the library to your app, instead of adding another dependency. I think anything < 100 LOC and static doesn't deserve it's own package.

One argument for packaging though is https://nacl.cr.yp.to/box.html - we can fork existing implementations of sig verification, slim them down to just verify method and include as part of the package (vs how RbNaCl is a requirement now)

Imagine users losing the ability to login into existing websites, and at the same time registering new profiles with a wrongly determined password

Can you explain how this can be solved by tests? Users type wrong password?

Testing is spot on. We need to add high level tests https://en.wikipedia.org/wiki/System_testing - any recommendations? And a unit test for scrypt derivation.

[andrewda]

I think anything < 100 LOC and static doesn't deserve it's own package.

There are a couple problems with this. One is just simplicity. Being able to just npm install or bower install a package is way easier than going to GitHub and copy-and-pasting a file, especially as SecureLogin grows. In addition, not publishing it to package managers opens up the possibility of someone else publishing code under SecureLogin's name. If you don't publish it to a package manager, someone else will, and there's no guarantee for that code to be safe.

[homakov]

Actually offering both is better. Will try now. Sensitive apps should vendor their dependencies. Others are fine with trysting rubygems or whatever middleman is used.

[homakov]

Started from this https://rubygems.org/gems/securelogin

[menzow]

Are there any? I thought their job is to work on existing HTML, not enforce their conventions.

For both there's documentation and opinion pieces available that describe practices to follow. Of course it isn't enforced on anyone and you're free to develop in a structure that's best suited to your project and needs. What is important is to document your reasoning behind the structure. This helps guiding consistency throughout the project and helps new contributors find code / add new code.

We don't use framework on purpose:

the app is small and doesn't intend to do anything beyond authentication. So benefits of full blown framework over some DOM manipulations are low

It's easier to audit for anyone not just for a programmer in respective React / Angular etc framework.

We also will refactor code and follow standardjs in near future.

I can only agree with and fully support this decision. This does make consistency through formatting and structure so much more important. The efforts from @andrewda in https://github.com/sakurity/securelogin/issues/31 is a great step in the right direction!

I'd also suggest splitting code into modules and templates. Checkout the LIFT principle for that.

Leftpad problem is bothering me, and the fact that we can push code to many apps just modifying a gem is scary. Once protocol is finalized, it's safe to copy-paste the library to your app, instead of adding another dependency. I think anything < 100 LOC and static doesn't deserve it's own package.

I can understand your reasoning on this issue. And your concerns are valid. The angle I'm coming from is sustaining healthy implementation strategy. Critical issues can't be fixed without breaking a majority of the websites / apps. Due to dependency management, hijacks can occur on many levels of the stack. Right now you depend on multiple cordova plugins which could be hijacked or break builds.

I think both concerns are valid. What ever decision is made in this case must be consistent.

Can you explain how this can be solved by tests? Users type wrong password?

Testing is spot on. We need to add high level tests https://en.wikipedia.org/wiki/System_testing - any recommendations? And a unit test for scrypt derivation.

What you can solve by tests is ensuring that the deterministic scheme will successfully create a profile. From this profile you should test if password generation is working for both valid and invalid passwords. This would become easier if there's a single module used for the SL protocol and all public apis exposed from that module are tested.

---

Have a nice day :)

[homakov]

What you can solve by tests is ensuring that the deterministic scheme will successfully create a profile.

It's a real problem. In the beginning cordova module gave totally different hash on Samsung phones (now fixed thanks to them). Smoke tests will be added because who knows what cheap android phones do to plugins.

[menzow]

Hey @homakov,

That is because right know you depend on the javascript and webview implementation of those devices.There's a few ways you can fix this:

1. Embed a fixed WebView component (this drastically app size, decreases portability and doesn't 100% ensure solving the problem.) https://cordova.apache.org/docs/en/latest/guide/platforms/android/webview.html
2. Use cordova module that exposes native hashing API's to JS. Not sure if there's a solution for this that supports your case.
3. Write native SL modules in Swift (IOS) and Java (Android), and hook those into a Cordova module. (I would suggest this)

[homakov]

That's exactly what 'cordova-plugin-scrypt' does. it just some internal bug that Samsung failed: https://github.com/Crypho/cordova-plugin-scrypt/issues/6

It's quite fast.

[menzow]

Just wanted to edit my comment. Forgot you're already using scrypt natively 👍

Edit: If you want to do real device tests checkout https://www.xamarin.com/test-cloud