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

Password protect settings #2932

Merged

Conversation

elshimone
Copy link
Contributor

@elshimone elshimone commented Dec 7, 2022

Title

Add panel to settings page to password protect settings.

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

closes #2907

Description

Adds a panel to the settings page to password protect all settings. The password is currently stored in the clear along with other persistent settings.
If no password is set, the panel prompts the user to set a password. If a password is set, the other settings panels are hidden until the correct password is supplied. If the password is supplied, the settings are made visible and an option to remove the password is enabled.

Screenshots

Dialog to set a password to prevent access to settings
image

Dialog to enter a password to access settings (other settings are hidden)
image

Incorrect password prompt
image

Dialog to lock or remove the password when correct password has been entered
image

Testing

I have tested the PR manually by going through the following steps:

  • Start Freetube
  • Navigate to settings, observe settings are available
  • Open password protect settings panel
  • Enter a password, and click set
  • Other settings panels are hidden, observe prompt to enter password to unlock
  • Enter an incorrect password and click "unlock"; incorrect password dialog show. Dismiss by clicking ok
  • Enter correct password. Observe that settings are unlocked.
  • Navigate away from setting, e.g. to subscriptions
  • Return to settings, observe they are locked
  • Restart app, observe settings are locked
  • Enter correct password, click remove password button. Observe settings are unlocked
  • Restart app, observe settings are unlocked

I have not observed any regressions due to my changes to the ft-input component, but would like some advice on what I should look for here.

Desktop

  • OS:
    Pop_OS
  • OS Version:
    22.04
  • FreeTube version:
    18.0.0

Additional context

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) December 7, 2022 20:08
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Dec 7, 2022
@PikachuEXE
Copy link
Collaborator

I use yarn dev to start app
After setting password and restart via yarn dev again the settings is unlocked
Not sure if caused by yarn dev or what (But no issue for other settings so I dunno)

Also I am not sure if that section should be put on the top (as it might not be a frequently used setting)

@PikachuEXE
Copy link
Collaborator

@ChunkyProgrammer I have just reviewed it lol

@elshimone
Copy link
Contributor Author

I use yarn dev to start app After setting password and restart via yarn dev again the settings is unlocked Not sure if caused by yarn dev or what (But no issue for other settings so I dunno)

Also I am not sure if that section should be put on the top (as it might not be a frequently used setting)

Good point I will move it from the top to the bottom. I'll look into the setting not being persisted, perhaps I broke something when I was refactoring the code. I had previously seen my subscriptions and other settings being reset with yarn dev, but I have just checked it and that doesn't happen any more (but the password setting is lost).

auto-merge was automatically disabled December 8, 2022 08:00

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) December 8, 2022 08:01
@elshimone
Copy link
Contributor Author

Ok I have moved the password settings to the bottom and fixed the setting persistence issue. I had not read the comments in settings.js properly, and was calling the autogenerated setter rather than the update call which actually persists things - it can't have ever worked before 😊

PikachuEXE
PikachuEXE previously approved these changes Dec 8, 2022
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Tested locally
Relying on others to code review settings related code

Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

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

Thanks for your PR, the code works but I have to say I am not a fan of the UX, having the enter password box in a settings section that is collapsed by default, doesn't make for a great unlocking experience.

It might be better if you split this up into two parts, the set, change and clear password settings section and then move the unlocking part into the Settings page. You could use v-if and v-else and centre the unlock box in the middle of the screen.

@elshimone
Copy link
Contributor Author

Yes that is a good idea, I will change the UX as you suggest @absidue

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Dec 13, 2022
auto-merge was automatically disabled December 14, 2022 17:01

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) December 14, 2022 17:02
@ChunkyProgrammer ChunkyProgrammer added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Dec 14, 2022
@elshimone
Copy link
Contributor Author

I have split out the password entry:

image

and password management section:

image

The password entry is focused so you can type the password straight in.

@PikachuEXE
Copy link
Collaborator

Found an error:
image

auto-merge was automatically disabled December 15, 2022 07:20

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) December 15, 2022 07:21
@elshimone
Copy link
Contributor Author

Fixed.

PikachuEXE
PikachuEXE previously approved these changes Dec 15, 2022
Comment on lines 3 to 22
import FtSettingsSection from '../ft-settings-section/ft-settings-section.vue'
import FtSelect from '../ft-select/ft-select.vue'
import FtCard from '../ft-card/ft-card.vue'
import FtInput from '../ft-input/ft-input.vue'
import FtToggleSwitch from '../ft-toggle-switch/ft-toggle-switch.vue'
import FtFlexBox from '../ft-flex-box/ft-flex-box.vue'
import FtButton from '../ft-button/ft-button.vue'
import FtPrompt from '../ft-prompt/ft-prompt.vue'

export default Vue.extend({
name: 'PasswordDialog',
components: {
'ft-settings-section': FtSettingsSection,
'ft-select': FtSelect,
'ft-input': FtInput,
'ft-card': FtCard,
'ft-toggle-switch': FtToggleSwitch,
'ft-flex-box': FtFlexBox,
'ft-button': FtButton,
'ft-prompt': FtPrompt,
Copy link
Member

Choose a reason for hiding this comment

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

Could you please remove the unused component imports, you only need to import and register the components that you use in the associated .vue file.

auto-merge was automatically disabled December 15, 2022 10:02

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) December 15, 2022 10:03
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) December 15, 2022 11:18
@FreeTubeBot FreeTubeBot merged commit dbb5473 into FreeTubeApp:development Dec 17, 2022
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Dec 17, 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.

Password protect settings
7 participants