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

Accept both name and username #2109

Closed
wants to merge 1 commit into from

Conversation

guy-datacom
Copy link

Login seems to generally expect username to be set (eg when using auth0's default passport module, its failure to set username meant login failed) but in this code NR only checks for name.

See here for mention of username:

https://nodered.org/docs/security

I think it would be safe to fallback to also accepting username, given other parts of NR seem to expect that to be set anyway. It seems a bit buggy to have to define both to satisfy different bits of code, but perhaps there are reasons for that I don't understand...

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Proposed changes

Checklist

  • I have read the contribution guidelines
  • For non-bugfix PRs, I have discussed this change on the mailing list/slack team.
  • I have run grunt to verify the unit tests pass
  • I have added suitable unit tests to cover the new/changed functionality

Login seems to generally expect username to be set (eg when using auth0's default passport module, its failure to set username meant login failed) but in this code NR only checks for name.

See here for mention of username: https://nodered.org/docs/security

I think it would be safe to fallback to also accepting username, given other parts of NR seem to expect that to be set anyway. It seems a bit buggy to have to define both to satisfy different bits of code, but perhaps there are reasons for that I don't understand...
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 76.414% when pulling 4c8f320 on guy-datacom:patch-1 into fe23608 on node-red:master.

@knolleary knolleary added this to To do in Development via automation Mar 28, 2019
@knolleary knolleary closed this in fbec803 Apr 2, 2019
Development automation moved this from To do to Done Apr 2, 2019
@knolleary
Copy link
Member

Thanks for this. I have investigated further and realised the use of user.name was a simple bug - it should have always been .username. Fix pushed and will be in 0.20.4

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

Successfully merging this pull request may close these issues.

None yet

3 participants