Webaccess basic auth #980

enbyted · 2017-06-21T19:49:59Z

So, I've wanted to implement this for a while.This is basically a simple implementation on HTTP basic auth.

About this PR:

This change is 100% backwards compatible - without new program argument everything works as before.

Changes to existing QLC+ stuff:

New command line argument: --web-passwords it accepts a path to a file with authorized users list
WebAccess constructor accepts new argument (optional)
configuration page's JS moved to separate file - configuration.js

Additions:

Introduced new class WebAccessAuth responsible for managing user/passwords list
Introduced new API section QLC+AUTH that enables runtime user management
Introduced new config section - Authorized users where user can manage data in web passwords file

Behavior:

When no web passwords file is provided all stuff related to basic auth is disabled (including configuration section)
When no username/password pairs are present in memory (empty or nonexistent web passwords file) authorization will be disabled and configuration section enabled
Changes to user list are immediately taking effect and persisted to web passwords file
Web passwords file is a text file containing one entry per line in form username:sha256(password)

Known issues:

Username cannot contain characters: : and | or things will break
Password cannot contains character | or things will break

I'm not sure how these should be approached - ban these characters or escape them?

Also I don't know how Qt translations work and how to update existing translation files to match new code (line number changes and new translation entries)

Other things I noticed:

HTML on configuration page is not formatted correctly - each sections opens two divs and closes only one I'm not sure if it is intentional or not. Here you can see patch (against latest commit in this PR) that fixes it (and screenshots before and after the patch)

diff --git a/webaccess/src/webaccessconfiguration.cpp b/webaccess/src/webaccessconfiguration.cpp
index 27d0460..cdb81e1 100644
--- a/webaccess/src/webaccessconfiguration.cpp
+++ b/webaccess/src/webaccessconfiguration.cpp
@@ -292,31 +292,35 @@ QString WebAccessConfiguration::getHTML(Doc *doc, WebAccessAuth *auth)
                        "</div>\n";

     // ********************* IO mapping ***********************
-    bodyHTML += "<div style=\"margin: 30px 7% 30px 7%; width: 86%; height: 300px;\" >\n";
+    bodyHTML += "<div style=\"margin: 30px 7% 30px 7%; width: 86%;\" >\n";
     bodyHTML += "<div style=\"font-family: verdana,arial,sans-serif; font-size:20px; text-align:center; color:#CCCCCC;\">";
     bodyHTML += tr("Universes configuration") + "</div><br>\n";
     bodyHTML += getIOConfigHTML(doc);
+    bodyHTML += "</div>";

     // ********************* audio devices ********************
-    bodyHTML += "<div style=\"margin: 30px 7% 30px 7%; width: 86%; height: 300px;\" >\n";
+    bodyHTML += "<div style=\"margin: 30px 7% 30px 7%; width: 86%;\" >\n";
     bodyHTML += "<div style=\"font-family: verdana,arial,sans-serif; font-size:20px; text-align:center; color:#CCCCCC;\">";
     bodyHTML += tr("Audio configuration") + "</div><br>\n";
     bodyHTML += getAudioConfigHTML(doc);
+    bodyHTML += "</div>";

     // **************** User loaded fixtures ******************

-    bodyHTML += "<div style=\"margin: 30px 7% 30px 7%; width: 86%; height: 300px;\" >\n";
+    bodyHTML += "<div style=\"margin: 30px 7% 30px 7%; width: 86%;\" >\n";
     bodyHTML += "<div style=\"font-family: verdana,arial,sans-serif; font-size:20px; text-align:center; color:#CCCCCC;\">";
     bodyHTML += tr("User loaded fixtures") + "</div><br>\n";
     bodyHTML += getUserFixturesConfigHTML();
+    bodyHTML += "</div>";

     // ******************* User management ********************
     if(auth)
     {
-        bodyHTML += "<div style=\"margin: 30px 7% 30px 7%; width: 86%; height: 300px;\" >\n";
+        bodyHTML += "<div style=\"margin: 30px 7% 30px 7%; width: 86%;\" >\n";
         bodyHTML += "<div style=\"font-family: verdana,arial,sans-serif; font-size:20px; text-align:center; color:#CCCCCC;\">";
         bodyHTML += tr("Authorized users") + "</div><br>\n";
         bodyHTML += getPasswordsConfigHTML(auth);
+        bodyHTML += "</div>";
     }

     QString str = HTML_HEADER + m_JScode + m_CSScode + "</head>\n<body>\n" + bodyHTML + "</body>\n</html>";

Screenshots:

before	after

Any feedback is highly appreciated. I'm happy to change anything here if required, however tomorrow I'm going on small vacation and I'll be back available on Wednesday.

Removed old changes

- User is informed about such incident - Application will behave as if basic auth was disabled - This allows for later creation of this file by online tool

Note: this is untested

enbyted · 2017-06-21T20:11:10Z

Why did travis fail? I don't think it's my fault (looks like missing lib)? or is it?

…zation

mcallegari · 2017-06-24T12:45:09Z

Hi, thanks for this PR.
I've got one question though: does it really make sense to have multi user access in a system where users cannot be distinguished ?
I mean, you normally give different users different levels of access. In this case there is none of it, as every user has full access to the web interface.
The current solution looks a bit overwhelming for QLC+, unless you plan to implement also access levels. (e.g admin can configure, guest cannot)

enbyted · 2017-06-29T10:52:58Z

I was thinking this should work like basic auth in apache or nginx - access to everything or nothing.
But it's not a problem to implement permissions.

I'm thinking about 3 levels of access:

Only Virtual Console
Virtual Console + Simple desk (Since technically simple desk can break things)
Everything

What do you think about it?

enbyted · 2017-06-29T10:57:36Z

Also, what do you think about adding this HTML fix to this PR?
I can make it look as if nothing changed, but I personally like it more whit the 'fixed' look.

These are only under the hood changes - no UI stuff done yet. Passwords file format is now: `username:password:level` where level is positive integer and is optional. Current levels: - 10 - Virtual Console only - 20 - Virtual Console and Simple Desk - 100 - Access to everything

enbyted · 2017-06-29T17:58:18Z

Okay, so above two commits add permission levels.
I thought about hiding elements that user cannot access, but this would disable users from switching to user with higher permissions (since Basic Auth doesn't have any logout scheme).

It is possible to logout user using javascript (sending an XHR request with invalid credentials) but it is hacky and I've seen cases where it didn't work for unknown reasons.

PS. I'll look at and fix codacy later, when I have the time.

mcallegari · 2017-07-01T10:50:11Z

Alright, I gave it a go.
1- the configuration page DIV issue is indeed a mistake. Your fix is OK, thanks
2- I launched qlcplus -w and I cannot access the configuration page. It gives 401 error

I think by default there should be complete access with no authentication. This is to preserve backward compatibility (think for example about users who already created scripts to control QLC+ via web)


~~~Also, non-admin users cannot be accepted until an admin user is added. Otherwise the configuration page becomes unreachable again.~~~

[EDIT] providing also --web-passwords option gives me access to config. That shouldn't be a mandatory argument (see backward compatibility above)
I think by default there should be a "factory" password file ([something like this](https://github.com/mcallegari/qlcplus/blob/master/webaccess/src/webaccess.cpp#L346)) unless a specific one is provided.
I'd also rename the option as "`-waf or --web-auth-file`"

enbyted · 2017-07-01T11:47:32Z

webaccess/src/webaccess.cpp

-        return;
+    WebAccessUser user;
+
+    if(m_auth)


Bug here!

If m_auth is null (when no --web-passwords is specified) there is no default user.
This may be fixed by having default constructor set WebAccessUserLevel::NOT_PROVIDED, I'm not sure now about implications of this - will check on Monday.

enbyted · 2017-07-01T11:47:39Z

2- I launched qlcplus -w and I cannot access the configuration page. It gives 401 error
I think by default there should be complete access with no authentication.

Umm, sorry, I broke it when I added permissions. I'll fix it tomorrow.

providing also --web-passwords option gives me access to config. That shouldn't be a mandatory argument (see backward compatibility above)

The idea behind this was that --web-passwords option turns on this feature in case someone doesn't want it at all. The fact that you get 401 without it is a bug.

I think by default there should be a "factory" password file (something like this) unless a specific one is provided.

This is also a possibility, I'm not sure if I like it though, because it would mean that user cannot disable this feature (i.e. the user management panel is always visible)

…d create WebAccessAuth instance inside WebAccess Fixed configuration page divs

enbyted · 2017-07-01T12:26:22Z

@mcallegari
Thanks for this change. Quickly looking through change log I noticed this:

2862f3f#diff-88036c1aff96ca72233e90ca2009ad2cR47

QFile file(filePath);

I believe this should be QFile file(m_passwordsFile) then since it may contain the default.

mcallegari · 2017-07-01T12:34:00Z

I believe this should be QFile file(m_passwordsFile) then since it may contain the default.

Indeed ! Fixed now.

This is also a possibility, I'm not sure if I like it though, because it would mean that user cannot disable this feature (i.e. the user management panel is always visible)

Makes sense, but then we should add another option to enable/disable web auth
OR
if no user is present, the first time one tries to add a user, a default authorization could be requested. Eg. admin / 1234 (or maybe something stronger 😅 )
Otherwise, leave it as it is. If someone wants to make a joke and add a user, in any case the "system administrator" has access to the device where QLC+ runs, and can delete the auth file
OR
distinguish normal web access (-w option) from web access + auth (-wa option) and add a bool flag to the WebAccess constructor

enbyted · 2017-07-01T12:47:53Z

if no user is present, the first one tries to add a user, a default authorization could be requested. Eg. admin / 1234

Not trivial to implement and doesn't really protect from anything plus adds another information that user has to acquire from documentation/manual/anything.

Otherwise, leave it as it is. If someone wants to make a joke and add a user, in any case the "system administrator" has access to the device where QLC+ runs, and can delete the auth file

This may not always be the case. For example in my usecase we sell device and users don't have shell access to it. That's why I've decided that this feature should be "opt in" rather than "opt out" in the first place - to avoid confusion and people locking themselves out + I believe that most users do not need this feature at all.

Makes sense, but then we should add another option to enable/disable web auth

I would go with this one. It can be "opt out" if you want, but I think that there should be an option to disable it entirely.

Maybe adding information that removing passwords file (and providing path to it) will disable authorization requirement will limit confusion of users who accidentally locked themselves out(provided they have read it before). (under existing one about at least one admin user?)

EDIT:

distinguish normal web access (-w option) from web access + auth (-wa option) and add a bool flag to the WebAccess constructor

This may be the best option or adding a separate option that enables auth. This is a matter of taste I think.

PS. I'm on the road and won't get access to computer/programming environment until tomorrow evening.

…thorization feature If -w/--web is provided, authorized user list is not even displayed in configuration page

mcallegari · 2017-07-01T13:17:24Z

distinguish normal web access (-w option) from web access + auth (-wa option) and add a bool flag to the WebAccess constructor

This may be the best option or adding a separate option that enables auth. This is a matter of taste I think.

Done. And fixed also 401 error I've had before if no auth is enabled

enbyted · 2017-07-02T17:54:20Z

@mcallegari is there anything that I should change here (not counting bugfixes if anything comes up)?

mcallegari · 2017-07-03T14:49:23Z

@enbyted I have done some quick tests of this PR (thanks) and it looks OK so far.
For a full test I'd need more time. but if you're happy with the changes I've made, I think we can merge this upstream

enbyted · 2017-07-03T16:52:54Z

I'm happy with how it looks now. I'm sure someone will break it sooner or later, I'll merge this change onto few devices in my city, so we'll have some real world test before any release with this happens.

BTW. Any idea what might have be wrong with windows QtCreator installation? I wanted to work on some of the VC stuff and tried to install it, but it only has broken my existing Qt install and doesn't build QLC+ (looks like it thinks that it's on linux and tries to use linux utilities like rm and such). Also I cannot start normal QLC+ install because of that :(

I'm sorry, I know it's not right place to ask, but maybe you know the answer right away.

mcallegari · 2017-07-03T17:20:40Z

I'm happy with how it looks now. I'm sure someone will break it sooner or later, I'll merge this change onto few devices in my city, so we'll have some real world test before any release with this happens.

On the contrary, I'm afraid a patch release is needed quite soon, cause of some regressions on VCSlider.
In any case, we can consider this as part of the 4.11.0 version and get some feedbacks soon.

BTW. Any idea what might have be wrong with windows QtCreator installation?

On Windows I use MSYS2 (which is a linux-like system) and I don't use QtCreator to build.
Basically what you see here:
https://github.com/mcallegari/qlcplus/blob/master/appveyor.yml#L18

Once you start using it, if you are a Linux user you will find yourself at home

enbyted · 2017-07-03T20:14:03Z

On the contrary, I'm afraid a patch release is needed quite soon, cause of some regressions on VCSlider.
In any case, we can consider this as part of the 4.11.0 version and get some feedbacks soon.

I didn't have time to apply our patches to 4.11 yet, thus we (my company) haven't really used it in any useful environment (plus I have expected problems to occur, cause it's a pretty big change in QLC+). I'm pretty sure that this PR doesn't break anything when it is not turned on, so it should be safe to push it in 4.11.

On Windows I use MSYS2 (which is a linux-like system) and I don't use QtCreator to build.

Yeah, that's what I used and then I installed QtCreator and it broke everything.

Anyway, I feel that I'm going mostly off-topic in this thread. I'm happy with this PR. Once you'll test it yourself and nothing comes up I think it can be merged. I'll port our patches and do an arm build on Wednesday and I'll pass it to our staging team, they'll test it.

If any problems with anything in webaccess come up I can work on them, just give me a @-ping in case I don't notice it (I don't follow forums, no time for that, but I do subscribe to this repo).

mcallegari · 2017-07-08T15:50:57Z

@enbyted see what happens ?
3019cb3
f6cd857
8c5d609
2d7a40d
9d32f4b
000599c

Every single build on OBS was broken.
Adding code to QLC+ is not just a matter of making it build with one GCC/Qt configuration :(

janosvitok · 2017-07-09T20:59:34Z

Two quick notes:

better format would be username|salt|shaXXX(salt+password) to prevent dictionary attacks
the hash used should be somehow explicitly noted, so that QT5 version can read (and possibly upgrade) files written by QT4 version.

enbyted · 2017-07-10T08:20:37Z

@mcallegari I'm sorry, I wasn't aware that QLC+ still supports QT4. I'll be much more cautious next time.

With that in mind @janosvitok 's idea is probably the way to go. I'm on the road and I'll be moving for next week. I can implement it on weekend.

I would propose this format: username|hash|hashType|salt.
It'll allow for the salt to be optional if someone for some reason wants to use pre-made hash and is flexible enough that different hash types that might come up in future and might require different arguments can be easily supported.

And hashType would be an integer:

1 - sha256 - only QT5
2 - sha1
any unsupported - complain to the console, but do not ignore the line - to preserve unsupported entries when saving file

BTW.
What is OBS?

janosvitok · 2017-07-10T09:11:54Z

OBS (openSuse Build Service): https://build.opensuse.org/ https://build.opensuse.org/project/show/home:mcallegari79

enbyted · 2017-07-15T13:04:56Z

@mcallegari Would you be happy with proposed solution? Maybe you have some suggestions?
I'm waiting for your feedback, I don't want to spend time implementing something that won't be merged.

mcallegari · 2017-07-15T13:32:09Z

@enbyted I think that will work OK

enbyted · 2017-07-15T14:08:22Z

Awesome, I'll work on that next weekend. Should be done before Sunday arrives.

…

-- Pozdrawiam Bartosz Grabias +48 793669953 HIPERNET Sp z o.o. ul. Kcyńska 48 85-304 Bydgoszcz 2017-07-15 15:32 GMT+02:00 Massimo Callegari <notifications@github.com>:

@enbyted <https://github.com/enbyted> I think that will work OK — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#980 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAtkE2WCFecUYFhSWUT33iRS9NrGdhs4ks5sOL9ZgaJpZM4OBb5j> .

mcallegari · 2017-08-08T09:37:10Z

@enbyted any news on this ? I think I need to release a 4.11.1 with some fixes of issues slipped into 4.11.0 and it would be nice to have the web auth feature completed.

enbyted · 2017-08-16T09:07:44Z

@mcallegari I'm sorry for the delay, I've fallen very sick month ago. I'm working on it right now (or rather fixing my qt install...), should be ready in a couple of hours.

Again, I'm very sorry that I couldn't work on it sooner.

EDIT:

You can take a look at the commit here: enbyted@3da3703

I ran compilation of QLC, it takes about an hour for me to do a full rebuild. I've got to go for now, but I will get back to it later today. Note that this is not testes, so might as well crash.
As soon as I test it (Qt5 only though) I'll submit a PR with that.

It's in a comment somewhere, but new line format is:

username:passwordHash:userLevel:hashType:salt
where everything after passwordHash is optional. Defaults are:

userLevel: super admin
hashType: sha256 (Qt5), sha1 (Qt4)
salt: empty string

Should be backwards compatible.

mcallegari · 2017-08-16T13:13:41Z

@enbyted no problem for the delay. I'm still finishing some things and by the way your health is far more important than some lines of code !

Enbyted (Bartosz Grabias) added 10 commits September 23, 2015 19:38

External input tap button now blinks with virtual one

dbad57e

Merge branch 'master' of github.com:enbyted/qlcplus

b9dbd57

Removed old changes

Merge remote-tracking branch 'upstream/master'

8a28e9c

added missing const

121056d

Initial implementation of RFC7617 basic auth

b31a0f0

Tidying up debug stuff.

50d66d0

Changed behaviour on error loading passwords file

1a8e118

- User is informed about such incident - Application will behave as if basic auth was disabled - This allows for later creation of this file by online tool

Added API for managing user entires

92977d2

Note: this is untested

Web interface for user management

d8fe901

Reverted some old changes

6ed258c

Enbyted added 3 commits June 21, 2017 22:35

Hopefully make codacy happy

cc8cf15

Merge remote-tracking branch 'upstream/master' into webaccess-authori…

65537a8

…zation

Fix last 2 hints on codacy

4a23b08

Enbyted added 2 commits June 29, 2017 15:00

Added access level management to ui

000a3df

enbyted commented Jul 1, 2017

View reviewed changes

webaccess: pass password file path (if present) to WebAccess class an…

2862f3f

…d create WebAccessAuth instance inside WebAccess Fixed configuration page divs

mcallegari added 2 commits July 1, 2017 14:27

webaccess: codacy fixes

36695da

webaccess: use the correct auth password file

317c8c0

webaccess: added -wa (or --web-auth) command line option to enable au…

051c155

…thorization feature If -w/--web is provided, authorized user list is not even displayed in configuration page

mcallegari and others added 4 commits July 1, 2017 15:20

main: fixed command line options tabs

45e5e2a

webaccess: fixed weird button shade CSS

db82af2

Fix codacy (hopefully)

ec4ed23

Make new user form reset after adding

b620a6f

mcallegari merged commit e0306fd into mcallegari:master Jul 6, 2017

mcallegari mentioned this pull request Jul 7, 2017

Web console password protection #933

Closed

enbyted mentioned this pull request Aug 16, 2017

Change webaccess auth file format to accommodate different hashing algorithms #992

Merged

This was referenced Oct 27, 2024

Show manager zoom and scroll with mouse wheel #1629

Closed

Track labels should be always visible in Show Manager #1630

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Webaccess basic auth #980

Webaccess basic auth #980

enbyted commented Jun 21, 2017

enbyted commented Jun 21, 2017

mcallegari commented Jun 24, 2017

enbyted commented Jun 29, 2017

enbyted commented Jun 29, 2017

enbyted commented Jun 29, 2017 •

edited

Loading

mcallegari commented Jul 1, 2017 •

edited

Loading

enbyted Jul 1, 2017

enbyted commented Jul 1, 2017 •

edited

Loading

enbyted commented Jul 1, 2017

mcallegari commented Jul 1, 2017 •

edited

Loading

enbyted commented Jul 1, 2017 •

edited

Loading

mcallegari commented Jul 1, 2017

enbyted commented Jul 2, 2017

mcallegari commented Jul 3, 2017

enbyted commented Jul 3, 2017

mcallegari commented Jul 3, 2017 •

edited

Loading

enbyted commented Jul 3, 2017

mcallegari commented Jul 8, 2017 •

edited

Loading

janosvitok commented Jul 9, 2017

enbyted commented Jul 10, 2017

janosvitok commented Jul 10, 2017

enbyted commented Jul 15, 2017

mcallegari commented Jul 15, 2017

enbyted commented Jul 15, 2017 via email

mcallegari commented Aug 8, 2017

enbyted commented Aug 16, 2017 •

edited

Loading

mcallegari commented Aug 16, 2017

Webaccess basic auth #980

Webaccess basic auth #980

Conversation

enbyted commented Jun 21, 2017

About this PR:

Other things I noticed:

enbyted commented Jun 21, 2017

mcallegari commented Jun 24, 2017

enbyted commented Jun 29, 2017

enbyted commented Jun 29, 2017

enbyted commented Jun 29, 2017 • edited Loading

mcallegari commented Jul 1, 2017 • edited Loading

enbyted Jul 1, 2017

Choose a reason for hiding this comment

enbyted commented Jul 1, 2017 • edited Loading

enbyted commented Jul 1, 2017

mcallegari commented Jul 1, 2017 • edited Loading

enbyted commented Jul 1, 2017 • edited Loading

mcallegari commented Jul 1, 2017

enbyted commented Jul 2, 2017

mcallegari commented Jul 3, 2017

enbyted commented Jul 3, 2017

mcallegari commented Jul 3, 2017 • edited Loading

enbyted commented Jul 3, 2017

mcallegari commented Jul 8, 2017 • edited Loading

janosvitok commented Jul 9, 2017

enbyted commented Jul 10, 2017

janosvitok commented Jul 10, 2017

enbyted commented Jul 15, 2017

mcallegari commented Jul 15, 2017

enbyted commented Jul 15, 2017 via email

mcallegari commented Aug 8, 2017

enbyted commented Aug 16, 2017 • edited Loading

mcallegari commented Aug 16, 2017

enbyted commented Jun 29, 2017 •

edited

Loading

mcallegari commented Jul 1, 2017 •

edited

Loading

enbyted commented Jul 1, 2017 •

edited

Loading

mcallegari commented Jul 1, 2017 •

edited

Loading

enbyted commented Jul 1, 2017 •

edited

Loading

mcallegari commented Jul 3, 2017 •

edited

Loading

mcallegari commented Jul 8, 2017 •

edited

Loading

enbyted commented Aug 16, 2017 •

edited

Loading