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 dashboard-only team role #2292

Merged
merged 2 commits into from
Jun 16, 2023
Merged

Add dashboard-only team role #2292

merged 2 commits into from
Jun 16, 2023

Conversation

knolleary
Copy link
Member

@knolleary knolleary commented Jun 15, 2023

Closes #1924

Description

This adds a new 'dashboard' team role that gives a user access to the http endpoints (including dashboard) of the team's node-red instances, but no access to the FF team settings or the editor.

This requires the 'FlowForge Authentication' option to be enabled in the Instance settings.

It also requires nr-launcher to have some changes applied:

When a Dashboard user views the team they are shown this:

image

The dashboard role has been added to the list of options in the 'change role' and 'invite member' dialogs:

image

If a dashboard-only user attempts to access the editor they see this error. This isn't the prettiest, but our options are somewhat limited here as we have to avoid sending them into an infinite redirect loop as Node-RED tries to get them logged in. We will be able to improve this in a future NR release to better handle auth failures when autoLogin is enabled.

image

Remaining Tasks

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on flowforge/helm to update ConfigMap Template
    • Issue/PR raised on flowforge/CloudProject to update values for Staging/Production

Labels

  • Backport needed? -> add the backport label
  • Includes a DB migration? -> add the area:migration label

Copy link
Contributor

@Pezmc Pezmc left a comment

Choose a reason for hiding this comment

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

I've given this a first pass and nothing stood out, but I'll look at it with fresh eyes tomorrow!

@Pezmc Pezmc self-requested a review June 16, 2023 12:56
@@ -71,7 +71,7 @@ module.exports = async function (app) {
if (!authClient) {
return badRequest(reply, 'invalid_request', 'Invalid client_id')
}
if (!/^editor($|-)/.test(scope)) {
if (!/^(editor($|-))|httpAuth-/.test(scope)) {
Copy link
Contributor

@Pezmc Pezmc Jun 16, 2023

Choose a reason for hiding this comment

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

I'd be tempted to extract this regex to something like the below to make it clearer what the test is checking for!

Edit: Perhaps one for both isEditor and isHttp/Node?

const isNodeReadHeaderRegex = `/^(editor($|-))|httpAuth-/`

Copy link
Contributor

@Pezmc Pezmc left a comment

Choose a reason for hiding this comment

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

I'm not super familar with this area, but I've now been over it twice and checked out the tests locally and the changes seem sensible! 👍

I won't merge in case you want second eyes

Co-authored-by: Pez Cuckow <email@pezcuckow.com>
@knolleary knolleary merged commit 34bc0ad into main Jun 16, 2023
4 checks passed
@knolleary knolleary deleted the 1924-dashboard-viewer branch June 16, 2023 16:27
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.

FlowForge RBAC for Dashboard viewers only
2 participants