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

Redact token in configurator #73

Merged
merged 4 commits into from
Aug 20, 2022
Merged

Redact token in configurator #73

merged 4 commits into from
Aug 20, 2022

Conversation

JOJ0
Copy link
Owner

@JOJ0 JOJ0 commented Aug 16, 2022

No description provided.

in "Config entry read.." debug output for token.
@JOJ0 JOJ0 requested a review from Ascurius August 16, 2022 08:38
@JOJ0
Copy link
Owner Author

JOJ0 commented Aug 16, 2022

Hi @Ascurius, the token should never be shown in the configurator but it should be obvious whether a token IS configured or IS NOT.
Does that make sense? Please play around with it a little and tell me if you feel it's superclear from a user perspective.

As a sidenote: What's still not possible is: Setting the token to "empty string", but currently I don't consider that important. It would be important if finally I would start implementing the possibility to

  • only set user in the configurator
  • no token
  • and have the user be prompted for a password for each synadm call (The supersecure option if an occasional synadm user prefers that instead of having to save the token on a system)

But that sidenote is just dumping my "future thoughts" here and shouldn't slow down this PR ;-) Your thoughts on it are welcome certainly! Thanks!

@Ascurius
Copy link
Collaborator

Hi @JOJ0, I have tested your changes and on my machine, the token was correctly redacted both if it was seit through CLI or by using the configurator. But I think that in the case of an empty token, the configurator should not just show [] but should include a message for the user similar to "NOT SET". This would make it absolutely clear that the token was not set yet. Therefore I have commited a correspondig tiny change. Let me know what you think about this proposal.

Regarding your future thoughts, I am convinced of the benefits that your proposals would have for the users. Especially the prompting for a password to authenticate an API call. I will get in touch with you regarding further details.

@JOJ0 JOJ0 merged commit 951fcae into master Aug 20, 2022
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