Skip to content
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

Add QR code generation #514

Merged
merged 9 commits into from
May 5, 2016
Merged

Add QR code generation #514

merged 9 commits into from
May 5, 2016

Conversation

AndyTaylorTweet
Copy link
Contributor

Add QR code generation to user profile information using the Google API.
This is a pre-cursor to an upgrade of the Android app to be able to scan in the QR code (rather than typing the huge API key).

Adding QR code for the Read Only API key, generated by the Google API, the idea being that later the smartphone apps can be modified to read the code.
@glynhudson
Copy link
Member

Just reviewed your QR code PR. It works great but I have some security concerns regarding sending the API key to a google API. I know it's over a secure connection but Google could still be logging this request. I think out of principal it would be best to generate the QR code locally.

Modified to remove my previous changes (Using Google API for QR code generation.
Added changes to use onboard Java based QR generation.
Added qrcode.js form https://raw.githubusercontent.com/davidshimjs/qrcodejs/master/qrcode.js
This is the supporting Java for on-board QR Code generation.
@AndyTaylorTweet
Copy link
Contributor Author

Glyn,

I agree with the security concerns with using any external API, the original method was "quick and dirty". So in order to keep moving on, I have now modified my original pull request to add some javascript for on-board QR code generation, no more reliance on Google's API.

Please test this and see what you think.

Javascript to support copying the API key using a button on the page.
Added code for the clipboard button; error handling included;
This works on most browsers, but there is no working code for Safari, however this does produce a human readable error on un-supported browsers.
@AndyTaylorTweet
Copy link
Contributor Author

The latest commits include the clipboard javascript to add the ability to copy the read API key to the clipboard. This could be easily expanded to include the write API key if that is desirable.
Button placement could be simply moved too.

@Paul-Reed
Copy link
Member

It's good that you've changed this so there is no reliance on Google's API, and having tried your proposed changes tonight, it works great, and a phone's QR scanner reads the API key OK.
There are no browser or javascript errors.

My only observations are;

  1. This is only useful if the Android App is able to read the QR code - I've asked the main app developer if this is a viable option (or were you intending to further develop the app yourself?)
  2. Again about usefulness... I'm not sure if the convenience of the 'copy to clipboard' function, justifies the additional code, when the API key can be easily and quickly copied via 'copy & paste'.

Paul

@AndyTaylorTweet
Copy link
Contributor Author

@Paul-Reed The plan is to integrate QR code reading in the android app, so that when you click to enter the api key, it will start the QR scanner instead; I've been looking at the android application already, so far adding the QR scanner module is beyond me - but its only a matter of time until either I work it out or the original developer may add the module for testing.

@JumpMaster
Copy link
Collaborator

Reading the QR code from the Android app should be easy. Could the QR code also include the URL for the emoncms site? From JavaScript use window.location.href?

@AndyTaylorTweet
Copy link
Contributor Author

So long as someone can mangle / adjust the android application to take whatever is included in the QR, I can probably make the QR code include whatever you like (within reason). 2D barcodes are able to cope with much more data than the old style 1D versions, but try not to stuff too much into a small space or the readability can suffer. So for example, we could output XML to make the input into the app more simple to include the hostname where the QR was generated and the api key? What I really need is someone to help work on the Android App (or a week off work and a few gallons of coffee to enable me to wrap my mind around it...)

@glynhudson
Copy link
Member

Nice! Thanks a lot, I've merged this in. For anyone interested this is what it looks like (obviously
without the red lines!). QR code will be fantastic once Android App has been updated. In the meantime the 'Copy to clipboard' works well. Would it be possible to add another copy to clipboard button for the R/W API key? We will then get this added to Emoncms.org. This will make it a little bit easier to copy the R/W API key to ready to paste into emonhub.conf for posting remotely to an emoncms.org account

emoncms user view

@glynhudson glynhudson closed this May 4, 2016
@glynhudson glynhudson reopened this May 4, 2016
@glynhudson
Copy link
Member

Shall I keep this open while discussion continues? I know the Android app developer @JumpMaster is very busy at the moment but might be able to give some input into the viability and best approach to reading from a QR code scanner?

Added 'copy to clipboard' for the write API key.
@AndyTaylorTweet
Copy link
Contributor Author

Added the copy to clipboard code for the write API key also.

@Paul-Reed
Copy link
Member

@JumpMaster how do you prefer the URL/API key delimiting within the QR code.

@JumpMaster
Copy link
Collaborator

From the following Google document it mentions "supported formats". URL being one of them.

https://developers.google.com/vision/barcodes-overview

A valid URL such as https://emoncms.org/?apikey=myreadonlyapikey could easily be manipulated to split the two.

@AndyTaylorTweet
Copy link
Contributor Author

If that's what you would like, I'll hammer out some proof of concept code...

Small code clean up around the HTML for the QR Code

QR Code output modified to produce a usable URL (Including the WRITE API key - this is done so that the QR code is usable on its own and opens the "My Electric" page under default setup.

URL generated from the Java "window.location.href" so it will use whatever the client used (http / https etc).
@AndyTaylorTweet
Copy link
Contributor Author

AndyTaylorTweet commented May 4, 2016

Proof of concept code added. See if that's what you were looking for.

URL generated is now in this format: http://host.name/emoncms/app?apikey=0000000000000000000000000000000#myelectric

Where obviously the apikey is the real key and the host.name is that of the host generating the QR code using the Java window.location.href as requested (taking care of http / https etc etc).

Additional comment:
I used the write API this time, because now the QR code works stand alone and has an actual function even if you don't use the android app, you can scan the code and you will be prompted to open a browser - this will then display the "My Electric" page correctly, this works in the default setup without the need for the user to have done anything special.

@Paul-Reed
Copy link
Member

I don't think that we need the /app in the URL, just the path to http://host.name/emoncms can it be removed?
Also, feel a little uncomfortable using the write API key...

@chaveiro
Copy link
Contributor

chaveiro commented May 4, 2016

Hi all i've been following this active thread, nice idea.
Do you guys know about the devicekey? - Its already supported since 9.2.
It adds much more security by only allowing access to a particular pre-configured device per its devicekey. A device maps to a node.
See https://openenergymonitor.org/emon/node/11755

@AndyTaylorTweet
Copy link
Contributor Author

@Paul-Reed I understand your concerns, but in the default config using the read key doesn't work, it makes a URL that will not open anything;

Similarly using the URL without /app results in the feeds page and not something the user might want to see (although you may be able to change my mind on that).

We have to make sure we remove any assumptions about the user, so I am trying to build the URL in such a way that without the android app installed, the URL generated still does something useful without relying on the end user to create a device key / make their feed(s) public etc or install the android app - although clearly the latter would be desirable).

@chaveiro it might make more sense to make similar output QR codes on the device management page rather than the user management page, now that we have the code to make this possible, doing so should be trivial to achieve.

@Paul-Reed
Copy link
Member

Sorry Andy, I don't agree.
In your first post above, you were pretty clear about your objective and said;

This is a pre-cursor to an upgrade of the Android app to be able to scan in the QR code (rather than typing the huge API key).

which I was hugely supportive of, but would not want to sacrifice security, for the sake of making this multi-functional.
Rather than start using the write API key, maybe the apps need work to enable them to use the read only key...

Paul

@AndyTaylorTweet
Copy link
Contributor Author

@Paul-Reed I agree with the sentiment totally; but you also have to put yourself in the users shoes, if they see a QR code, but don't know about the app - it would be handy for it to do something on its own.

In stock setup, the read API key has no access to view anything, the URL I generate is the same one that emoncms uses to display the "My Electic" page, not sure how this impacts security since to display the QR code in the first place you require either the write API key or a valid logon, although, as always, I am quite prepared to change things about and to have my point of view changed.

@GeoffAtHome
Copy link

QR codes can be created for Android that will launch a specific Android application.
http://stackoverflow.com/questions/10258633/android-start-application-from-qr-code-with-params

To test just create a private QR code with whatever you need. For example
http://www.qr-code-generator.com/

The URL must be included in the manifest file for the Android application. Not sure if the same can be done for iOS.

The point is the user does not need to know about the app they just need a barcode scanner app on the device. Anyhow the text around the QR can inform the user about what their need todo. It could also include a link to the play store to download and install the application.

Security if you can see the screen with the API key you can see the screen with QR code. I obviously don't understand the problem.

@JumpMaster
Copy link
Collaborator

I agree that I would prefer the users of the Android app to only use the read only key. I don't want to inadvertly screw up someone's setup or possibly be to blame for doing so.

I think defining a "device setup barcode" that isn't just for the Android app would be useful. It could contain the URL as well as read and write keys.

If there was a way to automate the setup of a device key that authorised itself using a temporary access token that would be even better.

@AndyTaylorTweet
Copy link
Contributor Author

AndyTaylorTweet commented May 4, 2016

@JumpMaster before I make the next commit; my current test code creates a url with both keys in it, so that the QR code works when scanned as a URL, but you can choose to just pick out the read key for your app;

Does that work for you?

URL example: http://host.name/emoncms/app?apiread=111111111111111111111111111111111&apikey=0000000000000000000000000000000#myelectric

Tested and my phone can open the default "My Electric" page but as above, both keys are available for app intigration.

@Paul-Reed
Copy link
Member

We just shouldn't be using a 'Write' API key for what is a read-only function, not just for the reason that @JumpMaster gave (although it's a very valid one in it's own right!), but because if it falls into the wrong hands the consequences could be dire, as the api now allows whole feeds to be deleted or values to be changed.

By including the 'Write' API key in the URL, it would be stored in the browser history, access logs on the server side, is susceptible to eavesdropping or packet sniffing, plus a whole host of other weaknesses, including public Wi-Fi hotspots exploitation.

True, the web version of My Electric doesn't work unless you use the 'Write' API key, but surely if this is the case, they why doesn't it get fixed instead of working around it? @JumpMaster has overcome the issue in his Android app, so surely it's surmountable. If we are using the 'Write' API key for all apps, why do we have a 'Read' API, is it redundant?

@TrystanLea can you clarify where we are with this pls.

Paul

@pb66
Copy link
Collaborator

pb66 commented May 5, 2016

The web apps use the read apikey but it is called a "readkey" (Trystan would need to explain why that is though)

https://emoncms.org/app?readkey=a4******************************90#myelectric

is a url thet I have issued to several users for read only access to the myelectric app which is basically the same access rights and page displayed as using the app. Try using
URL example: http://host.name/emoncms/app?readkey=11111111111111111111111111111111111&apikey=0000000000000000000000000000000#myelectric

In this instance both keys would be the readonly apikey so better still use a simplified

http://host.name/emoncms/app?readkey=0000000000000000000000000000000#myelectric

which will not only look neater (ie make sense) in the browsers address box and give read only access to the myelectric web app, it will also provide the correct key value, from which the app can extract the "readkey" and use it as a read-only "apikey".

I have to agree there could be some unification of the way the readkey and read-only apikey work, but by far the most important thing is to get away from using the write apikey for everything because it's easier, it really prevents emoncms progressing beyond a single "admin" user (per account) tool since nothing can be achieved without exposing the full write key and making all the most significant stuff vulnerable to accidents and attacks.

Paul

@AndyTaylorTweet
Copy link
Contributor Author

While the URL may work on emoncms.org it doesn't work on my local Pi.
I used the URL http://host.name/emoncms/app?readkey=1111111111111111111111111111111111#myelectric
and used only the read API key - but the page just prompts me for a login.

I may do some digging to work out why this is, it could be for example that I need to make the feed public - but I'm trying to make this part of the design as simple as possible for the end user. I believe that if we deliver a URL within the QR code, that QRL should do somthing rather than just prompt for a login.

So long as we can include the server's address and the read key into whatever that something is - I'm open to ideas and will gladly write the back end code for it.

@Paul-Reed
Copy link
Member

I used the URL http://host.name/emoncms/app?readkey=1111111111111111111111111111111111#myelectric
and used only the read API key - but the page just prompts me for a login

.
I've just tried here and it works fine, I get MyElectric ok without being logged in.

Paul

@AndyTaylorTweet
Copy link
Contributor Author

I'm using the latest RC build of the SD card, maybe thats why; I've also monkeyed with mine quite a lot fixing issues here and there. I'll modify the pull to match the readkey version, and I guess we'll re-visit this later should the need arise.

URL embedded in the QR code modified
@AndyTaylorTweet
Copy link
Contributor Author

Code added - ready for test.

@Paul-Reed Paul-Reed merged commit 1be0b53 into emoncms:master May 5, 2016
@Paul-Reed
Copy link
Member

Just tried your latest commit @AndyTaylorTweet and it works fine, allowing web app access without the 'Write' API Key. No browser or javascript errors, merged to Master to assist @JumpMaster in development of Android App QR reader.
Nice solution @pb66

@JumpMaster
Copy link
Collaborator

Looks really good and I've got an initial test app ready to download from the new forum.

https://community.openenergymonitor.org/t/mobile-app-qr-code/149

@AndyTaylorTweet
Copy link
Contributor Author

Tested and feedback provided.

@Paul-Reed
Copy link
Member

Paul-Reed commented May 5, 2016

Nice work @AndyTaylorTweet this is a very welcome addition to emoncms, thanks for developing this, and your patience!
IMO the only thing missing has been highlighted by @GeoffAtHome -

Anyhow the text around the QR can inform the user about what their need todo. It could also include a link to the play store to download and install the application

.Paul

@AndyTaylorTweet
Copy link
Contributor Author

@Paul-Reed I'll take a stab at this today and post something for review.

@AndyTaylorTweet
Copy link
Contributor Author

@Paul-Reed Since this PR is closed / merged I had to create a new one with the modified code; Take a look when you get a few mins and see if you think I'm on the right track with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants