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

Application access control: Rolambert_dev_07202020 #174

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rolambert
Copy link
Contributor

@rolambert rolambert commented Jul 21, 2020

application access control

application create user, login user

intent:

The intent is increase CNC safety by availability control i.e. application access control. This will ultimately give the ability to control the access to the CNC through application access control. However there are many other additional capabilities such as:

  • multi-user configurations
  • multi level configuration modes i.e. dev mode, admin mode, user mode.
  • user based stats
  • user based command and control of the CNC

features:

  • access control
  • database

future:

  • settings to configure access control i.e disable, default, global device settings
  • admin vs regular user based settings
  • expand SQL database to include user base statistics for run time path length cut etc

@Orob-Maslow
Copy link
Contributor

had to install extra modules for python:

flask-sqlalchemy
flask_login
flask_migrate
flask_wtf
email_validator

After installing, I was required to enter user name and password before system would allow any control from the web page. I registered a user name, email, and password, then put me back at the login screen and I logged in and it appears to work.

Copy link
Contributor

@Orob-Maslow Orob-Maslow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gpiozero removed from requirements.txt and I think it should be reinserted.

@rolambert rolambert requested a review from Orob-Maslow July 22, 2020 14:36
@rolambert
Copy link
Contributor Author

rolambert commented Jul 24, 2020

gpiozero removed from requirements.txt and I think it should be reinserted.

gpiozero was not remove just version was explicitly defined, what are your thoughts?

@Orob-Maslow
Copy link
Contributor

Orob-Maslow commented Jul 24, 2020 via email

@Orob-Maslow
Copy link
Contributor

For the record, I think it is a cool feature and it certainly took some work to get it to function, which I can certainly appreciate, but after trying it out, it isn't something that I want to have mandated on my machine. I don't want a sign-in required to access from my phone because then I have to fat finger another password to get access. For those with systems in public spaces, I can see why this is useful and certainly if the system is exposed to the web in general (which I think is a bad idea anyway). Is there a way to disable this password wall without having to log in each device? Perhaps we can put an activation toggle in the maslow settings to enable or disable this option? I could help add one.

@rolambert
Copy link
Contributor Author

rolambert commented Jul 24, 2020

For the record, I think it is a cool feature and it certainly took some work to get it to function, which I can certainly appreciate, but after trying it out, it isn't something that I want to have mandated on my machine. I don't want a sign-in required to access from my phone because then I have to fat finger another password to get access. For those with systems in public spaces, I can see why this is useful and certainly if the system is exposed to the web in general (which I think is a bad idea anyway). Is there a way to disable this password wall without having to log in each device? Perhaps we can put an activation toggle in the maslow settings to enable or disable this option? I could help add one.

TY, I like your points, lets do it.

  • I would still like to 'initialize' with the a user for db creation this makes available a bunch of fun stats and use case things available.

  • some stats could even integrate into the online community such as total run time, lifetime router path cut distance, fun things.

  • or reorder bits after a set distance etc..

  • data is everything :)

I see two possible ways this disable can be done (other than password manager ) :

  1. at first login screen add a toggle/button to disable password logins for single user setup, change in settings later

or.

  1. after initial sign-up and login the initial 'admin account' can disable login feature

I think 2. would be easier to implement and is more industry standard.
a HINT: note could be listed at the login screen to help people disable the login wall if needed.

Thoughts?

@Orob-Maslow
Copy link
Contributor

Orob-Maslow commented Jul 24, 2020 via email

Copy link
Contributor

@gb0101010101 gb0101010101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rolambert, Thank you for contributing this great feature. I whole heatedly support implementing login. In multi-user environments such as hacker/maker spaces it is required. Even in home/single user environments it is good to lock things down so other household members do not cause havoc.

I don't want to discourage your effort but this feature is not is a state to be committed. With your current code a user can click the create_user link, provide info, and immediately login, without validation/approval. This renders the whole login feature redundant as it can be bypassed in seconds.

I see you have laid the framework for storing accountlevel but have not implemented it. I don't see anything that could store approved user account state that allows or denies login.

It does not look like making login an optional feature is easy? If you think it is then please let me know as some Maslow user may not want mandatory login.

For those that want login perhaps you could code it so:
If user accounts = 0, then create admin level (1) account, and grant immediate access.
Otherwise if user accounts > 0, then create second level (2), and deny access by default.

There is also no UI to manage users. In the least there should be a page that lists all users accounts and has UI to change their approved state that is only accessible to admin users.

Alternatively new user account creation page could be disabled once an admin account is created. So only admins can access the page, set access level, and approve them.

This leads on to problems with forgotten passwords. Email is collected but there does not seem to be any system in place to send emails or recover passwords. What happens if the admin account password is forgotten/lost? Can this instance send email?

This also calls into question the hard coded SECRET_KEY for the database which should be user configurable. Perhaps during first Admin account creation?

Finally the the login and register pages are not mobile responsive and currently break UI making them difficult to use. I think I have fixed most UI issue in PR #117. Added blocks for HEAD, JAVASCRIPT, CONTENT in appropriate places. If you have the time please test it and let me know if anything needs to be changed. I am eager to help in fixing UI issues which have been overlooked. I think your login UI code could be easily fixed once PR #117 is committed.

To summarize: I welcome the addition of login feature but this code does not accomplish basic minimum user access control to make it worth while. I completely support login access control and hope to see it implemented. Happy to help in making this happen.

<h6 class="card-category">
{% if msg %}
<span class="text-danger">{{ msg | safe }}</span>
{% else %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo "wellcome" and poor wording. Consider removing the {else} block as this link and text is already provided lower on line 67.


<button type="submit" name="login" class="btn btn-primary pull-left">Login</button>

&nbsp; &nbsp;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove unnecessary &nbsp; &nbsp;

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

Successfully merging this pull request may close these issues.

9 participants