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.

Sign up for GitHub

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

Jump to bottom

PoC: Adding a basic login system to Puppetboard #468

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

PoC: Adding a basic login system to Puppetboard #468

wants to merge 2 commits into from

Conversation

RobReus
Copy link

@RobReus RobReus commented Apr 30, 2018 •
edited by maxadamo
Loading

Hi all,

I have written a very basic login system for Puppetboard. The reason being that in my case, I am running puppetboard in a docker so I cannot use the authentication mechanism of apache/nginx/haproxy. With the new GDPR law coming next month, we have a new requirement that everyone needs a named account for all systems, this includes puppetboard.

In the absence of such system, I have decided to make it myself. The very basic PoC is this merge request. It is far from a finished product. The following items still need to be done:

  • Add code/unit tests for the new code
  • Add "Add user" modal to the users.html template
  • Add "Change password" modal to the layout.html template
  • Add password encryption
  • Implement other storage backends besides SQLite
  • Make the "Edit" and "Remove" icons functional on the users.html template
  • Improve the HTML/CSS (I suck at CSS, I did my best)
  • Perhaps implement groups with group permissions using "Flask-Principal"
  • Perhaps implement LDAP authentication with Group support
  • Perhaps make it possible to change Puppetboard settings in the newly introduced "Settings" dropdown
  • Make Travis happy

If you want to test my code, all you need to do is checkout my branch/PR and adjust the default_settings.py accordingly. You need to set LOGIN_DISABLED to False. When the app first starts up, a user is created with admin as username and admin123 as password. Once you are logged in, 2 new menu bar items appear; Settings and "Logged in as: ...". See my screenshots below to get an idea.

The reason I am creating this PR is that I would really appreciate some feedback and ideas on how to improve it. Feel free to submit your own code.

Following are some screenshots of the UI.

Login screen:
login screen

Login screen with "Login required" flash:
login screen login required flask

Login screen with "Logged out" flash:
login screen logged out

Settings dropdown:
settings dropdown

User dropdown:
user dropdown

When not logged in (nothing changes compared to current master branch):
index not logged in

@mterzo
Copy link
Contributor

mterzo commented Apr 30, 2018

I’m concerned with some of changes when not using login. Also, there are a few Travis ci issues. Looks like we need to drop 2.6 in the chain as well.

@RobReus
Copy link
Author

RobReus commented Apr 30, 2018

@mterzo fixing the travis issues is in the list of tasks to be completed.

What are your concerns regarding changes when not using login?

@mterzo
Copy link
Contributor

mterzo commented Apr 30, 2018

Couple places where I’m not sure how flask will behave. Once I have a chance to spawn it in a container and look at the changes not on my phone.

@othalla
Copy link
Contributor

othalla commented Sep 27, 2018

Can you please resolve conflict?
Has anyone tried this?

@RobReus
Copy link
Author

RobReus commented Oct 1, 2018

@othalla the merge conflict has been pushed.
I have tested the code when I wrote it and it worked for me. However, I did not yet have the time to go over the to-do list items. Due to the lack of response on this PR I have not given it any priority on my side.

Feel free to make any changes/additions you see fit to the code.

@othalla
Copy link
Contributor

othalla commented Oct 1, 2018 •
edited
Loading

Hello, i will check this out. However front is not my specialty. Any help will be glad ;)
You have some line length warning reported by pytest --flake8

@othalla
Copy link
Contributor

othalla commented Oct 1, 2018 •
edited
Loading

@RobReus first of all, i think we should reverse the LOGIN_DISABLED to ENABLE_LOGIN assuming it is not activated by default, as in this PR.
Globally, as login must be explicitly enabled, it may not break any basic feature.
@mterzo what do you think?

@RobReus
Copy link
Author

RobReus commented Oct 1, 2018

@othalla changing LOGIN_DISABLED into ENABLE_LOGIN sounds fair to me. As for the pep8/pycodestyle errors, I have not spent any time tidying up the code according to style specs. Nor have I fixed any travis reported errors. Since this change is not that big, it should not be that much work to do.

Going on a 3 week holiday this week without my laptop. Might have some time after my holidays to look into this.

@othalla
Copy link
Contributor

othalla commented Oct 31, 2018

Plan to test it soon. Plan to give some feedback next week.

@othalla
Copy link
Contributor

othalla commented Apr 13, 2019

@RobReus could you check for the modification regarding login enable?

@faku99
Copy link

faku99 commented Sep 28, 2020

Hey guys, any news on this feature?

@RobReus
Copy link
Author

RobReus commented Sep 28, 2020

I have since stopped using puppet and puppetboard. If anyone wants to take ownership of this change, feel free to do so.

@elfranne
Copy link

I got a working example of active directory login, is it useful to post here, or maybe in a markdown example ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
needs-new-owner needs-rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@RobReus @mterzo @othalla @faku99 @elfranne @gdubicki