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

[AC-1046] activate autofill on page load policy #4860

Merged
merged 11 commits into from
Mar 10, 2023

Conversation

jlf0dev
Copy link
Member

@jlf0dev jlf0dev commented Feb 24, 2023

Type of change

- [ ] Bug fix
- [x] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Adds a new policy that will automatically activate auto-fill on page load for all users in the organization when the policy is enabled.

From the end-user perspective, a one-time notification will show to indicate that the organization turned on the setting. The policy will not restrict the user from modifying the setting after activation. This means that the user can:

  • Turn off auto-fill
  • Modify the “default autofill setting for login items”
  • Modify URI match detection on the item level

Server dependency at bitwarden/server#2751

Code changes

Some of the messaging on browser has been lost recently, specifically "loggedIn", "unlocked", and "syncCompleted" messages don't get caught by the bitwardenPopupMainMessageListener. This means that we can't detect when the user completes any of these actions to activate the setting and show the popup. As an alternative, I have attached the flag detection on the current-tab.component so when the user navigates to that page it activates from the policy.

  • bitwarden_license/bit-web/src/app/policies/activate-autofill.component.html,ts: New component for the ActivateAutofill policy
  • apps/browser/src/services/browser-policy.service.ts: Whenever the policies are updated, we check for the ActivateAutofill policy and set a flag for later when we know the popup is open.
  • libs/common/src/enums/policyType.ts: add flag to state service
  • apps/browser/src/vault/popup/components/vault/vault-filter.component.ts: Checks for previous flag and enables autofill on page load if true.

Screenshots

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

@jlf0dev jlf0dev requested a review from a team as a code owner February 24, 2023 15:45
/>
<label class="form-check-label" for="enabled">{{ "turnOn" | i18n }}</label>
</div>
</div>
Copy link
Member

@differsthecat differsthecat Feb 27, 2023

Choose a reason for hiding this comment

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

We had a good discussion here a couple of weeks ago about some of the impacts of the autofill on page load setting. Enabling it can be a security risk. Should we add a callout here similar to the one in the autofill settings with a warning that it can be exploited?

Screenshot 2023-02-27 at 8 59 29 AM

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a very good point, @bitwarden/dept-design what would you think about copying everything after WARNING: into a callout on the policy?

Copy link
Member Author

Choose a reason for hiding this comment

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

@differsthecat added a callout, nice catch!

image

@github-actions github-actions bot added the needs-qa Marks a PR as requiring QA approval label Feb 28, 2023
differsthecat
differsthecat previously approved these changes Feb 28, 2023
Copy link
Member

@differsthecat differsthecat left a comment

Choose a reason for hiding this comment

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

This looks good, thank you @jlf0dev !

@jlf0dev jlf0dev added the hold do not merge, do not approve yet label Mar 1, 2023
@jlf0dev
Copy link
Member Author

jlf0dev commented Mar 1, 2023

Adding hold - we need to restrict this feature to Enterprise 2020 plans

@jlf0dev jlf0dev removed the hold do not merge, do not approve yet label Mar 7, 2023
@jlf0dev
Copy link
Member Author

jlf0dev commented Mar 7, 2023

Update: This feature has been locked to the current Enterprise plans. This is more of a 'hard lock' than other features, and doesn't allow CS to toggle the feature on/off. Since this is a hackathon item, Product felt this was appropriate.

differsthecat
differsthecat previously approved these changes Mar 8, 2023
Copy link
Member

@differsthecat differsthecat left a comment

Choose a reason for hiding this comment

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

Looks good!

@jlf0dev jlf0dev removed the needs-qa Marks a PR as requiring QA approval label Mar 10, 2023
@jlf0dev jlf0dev merged commit 7892834 into master Mar 10, 2023
@jlf0dev jlf0dev deleted the ec-1046-activate-autofill-policy branch March 10, 2023 17:52
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.

2 participants