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 httpNodeAuth settings #965

Merged
merged 4 commits into from
Sep 27, 2022
Merged

Add httpNodeAuth settings #965

merged 4 commits into from
Sep 27, 2022

Conversation

hardillb
Copy link
Contributor

@hardillb hardillb commented Sep 9, 2022

Allows the setting of a single username/password in the template

Fixes ##578

Requires FlowFuse/nr-launcher#64

Screenshot from 2022-09-09 14-03-17

@hardillb hardillb added this to the 1.0 milestone Sep 9, 2022
@hardillb hardillb self-assigned this Sep 9, 2022
@hardillb
Copy link
Contributor Author

hardillb commented Sep 9, 2022

Does the password bcrypt in page so only the hash is ever sent to the backend and stored in the database

@hardillb
Copy link
Contributor Author

@knolleary I have pushed what might be a slightly naive fix for the password getting overwitten on any other change to the template.

@knolleary
Copy link
Member

I've reworked how passwords are handled in template settings.

We now hash them in the backend before they are stored. We never return the value in the api - we return a boolean to indicate if a non-blank value exists.

The UI then does the Right Thing to show a placeholder or not.

I have verified the settings are applied properly by the launcher - and that the values can be overridden in the project.

There is one PR to come against the launcher to avoid righting blank settings into httpNodeAuth.

@knolleary
Copy link
Member

Huh - @hardillb as the person who raised this PR originally, it will now let me add you as a reviewer (which is why I tried unassigning it from you first...). Please take a look.

@hardillb
Copy link
Contributor Author

Yep, was already looking, got confused for a second as to why there was no change to package.json to remove the bcryptjs, but makes sense now

@hardillb
Copy link
Contributor Author

Code looks OK, but 2 of the UI tests are with something to do with httpNodeAuth

@hardillb
Copy link
Contributor Author

All the tests pass locally, so I'm happy to merge this (and we need to set aside some time next sprint to fix the postgres tests and work out what's happening here)

@hardillb hardillb merged commit 0d3a237 into main Sep 27, 2022
@hardillb hardillb deleted the httpNodeAuth branch September 27, 2022 14:00
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.

None yet

2 participants