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

Update the login/Keycloak docs page #1289

Merged
merged 2 commits into from
May 24, 2022
Merged

Conversation

gabalafou
Copy link
Contributor

@gabalafou gabalafou commented May 16, 2022

Related to #1076.

Changes introduced in this PR:

  • Update login/Keycloak page in docs: Installation > Login.
  • A few other various minor fixes in the docs

Types of changes

What types of changes does your PR introduce?

Put an x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features to not work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

Requires testing

  • Yes
  • No

Documentation

Does your contribution include breaking changes or deprecations?
If so have you updated the documentation?

  • Yes, docstrings
  • Yes, main documentation
  • Yes, deprecation notices

Screenshot

Here's how the page looks after these changes:

Full page screenshot of docs page generated using Chrome Dev Tools

Copy link
Contributor Author

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

done self-reviewing

After the initial deployment, it is **highly** recommended that you change the Keycloak `root` user password as soon as you can.

> NOTE: Once you change the root password you will not be able to [add users from the command line](login.md#add-user-from-the-command-line)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why change the root password? Is it because the yaml config file gets checked in to version control?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not needed but is advised, I agree that this text does not reflect this idea correctly. Another thing that can be done is changing the Keycloak password to use qhub secrets (though I haven't tested it yet in this case).

docs/source/installation/login.md Outdated Show resolved Hide resolved
docs/source/installation/login.md Outdated Show resolved Hide resolved
Groups represent a collection of users that perform similar actions and therefore require the similar permissions. By default, QHub is deployed with the following groups: 'admin',
'developer', 'analyst' and 'viewer'.
Groups represent a collection of users that perform similar actions and therefore require similar permissions. By default, QHub is deployed with the following groups: 'admin',
'developer', 'analyst' and 'viewer' (in roughly descending order of power).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why isn't 'viewer' in the table below?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't even have the viewer anymore, but I would need to do a fresh deploy to be sure.


![Keycloak group conda-store-manager form - role mappings tab focused with expanded client roles dropdown](../images/keycloak_new_group2.png)
In this example, the new group only has one mapped role (`conda_store_admin`); however it's possible to attached multiple 'Client Roles' to a single group.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The relationship between groups and roles here is not at all intuitive if you don't have a background in user permission systems, I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree this is a tough topic as some of those groups as acting as a permission system and some as user "groups" only. c.c @costrouc here to better elucidate this if needed.


![QhHub admin view - Root Login to Keycloak form](../images/keycloak_master_login.png)
![QHub admin view - Root Login to Keycloak form](../images/keycloak_master_login.png)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't indent here, then Sphinx will not continue the list numbering—it will render the "2." below as "1."

docs/source/installation/login.md Outdated Show resolved Hide resolved

Visit <https://myqhubsite.com/> (or whatever domain you have chosen for your QHub).

Click 'Sign in with Keycloak'.
Click 'Sign in with Keycloak'. This will take you to the login form:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't love the two-step login, even if it's used at places as preeminent as Gmail. I wonder if the button should say "Sign in with Keycloak" or just "Sign In".

Copy link
Contributor

Choose a reason for hiding this comment

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

well, that's customizable, but I don't see a good enough reason for the change. But is open for discussion 🙂

[Groups](./login.md#groups) section below for more details).

Enter the name you would like for the user then click 'Save'.
7. Click save.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I don't love the indentation that these markdown lists create. I wonder if it would be better to reformat as:

Step 1. Blah blah blah

![some screenshot](url-to-screenshot.jpeg)

Step 2.

in order to avoid the indentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

feel free to adopt it, we can always switch back if needed 🙂

@viniciusdc
Copy link
Contributor

Hi, @gabalafou thanks for opening this pull request and adding those considerations!!! I don't know if you need my comments in there or not, but I did it anyway 😃. We are still discussing the v0.4.1 post-release changes, and once that's done we can merge this!!! Thanks again for the contribution 🚀

@viniciusdc viniciusdc added area: documentation 📖 Improvements or additions to documentation needs: review 👀 This PR is complete and ready for reviewing area: user experience 👩🏻‍💻 labels May 19, 2022
@costrouc costrouc merged commit ee8379f into nebari-dev:main May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: documentation 📖 Improvements or additions to documentation area: user experience 👩🏻‍💻 needs: review 👀 This PR is complete and ready for reviewing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants