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

[feature/password-policy] Password Policy support #1325

Merged
merged 13 commits into from
Apr 12, 2024

Conversation

felix-schwarz
Copy link
Contributor

@felix-schwarz felix-schwarz commented Feb 23, 2024

Description

This PR implements password policy support throughout the iOS client app, including:

  • an extensible password policy system based on rules, policies and validation reports with verbose error reporting for
  • the generation of password policies based on server capabilities
  • a default password policy for servers that do not provide a password policy
  • a password generator based on password policy rules using "cryptographically secure random bytes"
  • a password composer for entering, editing and generating passwords with instant rule verification and feedback
  • one-tap password generation based on a server's password policy within Public Link creation
  • sharing of combined public link URL and password to the clipboard, Messages, Mail and more via the system share sheet directly after link generation, like f.ex.:
Photos (https://demo.owncloud.org/s/D3WkWZOW8BUjeKr) | Password: 46CPou|#Pw5.

Related Issue

#973

Screenshots (if appropriate):

Inline Generator button Password Composer Password Composer Share Sheet
Simulator Screenshot - iPhone 15 - 2024-02-23 at 08 43 34 Simulator Screenshot - iPhone 15 - 2024-02-23 at 08 43 40 Simulator Screenshot - iPhone 15 - 2024-02-23 at 08 43 49 Simulator Screenshot - iPhone 15 - 2024-02-23 at 08 46 06

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

QA

Test Plan:

https://github.com/owncloud/QA/blob/master/Mobile/iOS/Executions/Version%2012.2/Password%20Policy.md

Reports:

- ShareViewController: add "generate" button to the password field that uses the password policy's generator to generate a password
- BottomButtonBar: add new alternative button to allow for an alternative main action; fix layout bug
- PasswordComposerViewController:
	- new view controller to compose passwords
	- interactive feedback based on OCPasswordPolicy
	- integration with password generator based on OCPasswordPolicy
	- ability to copy passwords to clipboard
	- ability to show/hide entered passwords
	- support for editing and creation of password strings
- ShareViewController:
	- replace UIAlert with PasswordComposerViewController for entering passwords
	- add Generate button to generate a password based on the currently applicable password policy
	- add new "Share" button for links (in addition to "Create") that invoke the share sheet to directly share a link (including password) to the clipboard or directly to other apps like Mail or Messages
- add missing localizations
- BottomButtonBar: include alternativeButton in .modalActionRunning auto-enable/disable
- SegmentViewItem / SegmentViewItemView:
	- add extension to easily create button segments
	- add support for UIImage rendering modes for .icon
- ThemeCollection: add CSS entry for proper PasswordComposerViewController cell background coloring in dark mode
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@felix-schwarz felix-schwarz changed the base branch from master to milestone/12.2 February 23, 2024 07:47
@jesmrec
Copy link
Contributor

jesmrec commented Mar 11, 2024

Some preliminary inputs meanwhile the Code Review finishes:

  • I miss some feedback message when the user clicks on Copy (just next to Generate). Every action should have a reaction, otherwise the user does not know if the performed action is correct or not. Password is correctly copied, but a message like Copied to clipboard would be welcome

  • I would avoid empty conditions to build password policy like:

Screenshot 2024-03-11 at 13 20 01

Those conditions will be always green, they are no real requirements.

  • If i set more digits than the allowed, i see a condition like Longer than 72 bytes in utf-8 encoding that is not fulfilled just with the 73th character. i know that this is related with the string encoding, but, how does it overflows the limit? could you ellaborate?

  • How does it work in oC10? i've seen it enabled in servers that does not return values in capabilities

- PasswordComposerViewController: add notification upon copying password to clipboard
@felix-schwarz
Copy link
Contributor Author

Thanks for the feedback @jesmrec!

  • I miss some feedback message when the user clicks on Copy (just next to Generate). Every action should have a reaction, otherwise the user does not know if the performed action is correct or not. Password is correctly copied, but a message like Copied to clipboard would be welcome

I added a message with 98de65c.

  • I would avoid empty conditions to build password policy like:

Those conditions will be always green, they are no real requirements.

Good point. The code generating the policy from capabilities now no longer includes rules with a zero length requirement.

  • If i set more digits than the allowed, i see a condition like Longer than 72 bytes in utf-8 encoding that is not fulfilled just with the 73th character. i know that this is related with the string encoding, but, how does it overflows the limit? could you ellaborate?

My choice of wording here was off by 1. It should be At most rather than Less than of course.

  • How does it work in oC10? i've seen it enabled in servers that does not return values in capabilities

For OC10 or if no policy is provided, a default policy is used:
https://github.com/owncloud/ios-sdk/blob/3096454383cdfb24a11aa6b8a84b53c853308ed7/ownCloudSDK/Password%20Policy/OCPasswordPolicy%2BDefault.m#L27

@jesmrec
Copy link
Contributor

jesmrec commented Mar 12, 2024

@felix-schwarz this is how password policy works in oC10:

Open an admin account (list of files) > Top Left hamburger button > Market

Look for Password Policy in the lit of plugins and install it

Then, in oC10's admin dashboard, you'll find the setup for the password policy. Tick the conditions to fulfill and click on Save (this is important, no autosave available in the feature):

Screenshot 2024-03-12 at 15 07 47

capabilities endpoint will return the following JSON object:

"password_policy": {
                    "password_requirements": {
                        "configuration": {
                            "lower_case": {
                                "generate_from": {
                                    "characters": "abcdefghijklmnopqrstuvwxyz"
                                },
                                "minimum": 1
                            },
                            "numbers": {
                                "generate_from": {
                                    "characters": "1234567890"
                                },
                                "minimum": 1
                            },
                            "special_characters": {
                                "generate_from": {
                                    "any": true
                                },
                                "minimum": 1
                            },
                            "upper_case": {
                                "generate_from": {
                                    "characters": "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
                                },
                                "minimum": 1
                            }
                        },
                        "minimum_characters": 8
                    }
                }
            },

as you can see, nothing to do with oCIS response. It may not worth to implement support for this if oC10 is not going to be supportable in the st, but this is up to you.

@felix-schwarz felix-schwarz changed the base branch from milestone/12.2 to master March 26, 2024 09:57
@felix-schwarz felix-schwarz changed the base branch from master to milestone/12.2 March 26, 2024 09:57
@felix-schwarz felix-schwarz reopened this Mar 26, 2024
@jesmrec
Copy link
Contributor

jesmrec commented Mar 26, 2024

Let's QA here...

@jesmrec
Copy link
Contributor

jesmrec commented Mar 26, 2024

(1) [FIXED]

A little UX improvement:

Create and Share buttons should be disabled if password is mandatory and was not set yet. An error missing required password is displayed, but i think it's more friendly to allow it when it's really posible.

Indeed, this is the current behaviour for permission selection

iPhoneXR v17.4.1
b4ab539bd

@jesmrec
Copy link
Contributor

jesmrec commented Mar 26, 2024

(2) [FIXED]

In a non-oCIS server, the Generate button is there but no action happens.

video taken with demo.owncloud.com (not sure if the clicks are visible, but they are there)

RPReplay_Final1711454487.MP4

Should be either actioned or removed.

iPhoneXR v17.4.1
b4ab539bd

@jesmrec
Copy link
Contributor

jesmrec commented Mar 26, 2024

(3) [FIXED]

Look at the following steps:

  1. Create a new link with password (no matter which password or how to generate it)
  2. Open the link to edit
  3. Remove the existing password with the x
  4. Click on Generate

Current:

At this point, there is no way to recover the automatically generated password, because the edit mode lacks of Share option (create mode does). Just Save Changes and password is missing.

Set option allows it.

Expected:

After generation of new password, there should be a way to get it to be shared. Options that come to my mind:

  • automatic copy to the clipboard after clicking on Generate
  • other button called Share link in the creation mode

iPhoneXR v17.4.1
b4ab539bd

@jesmrec
Copy link
Contributor

jesmrec commented Mar 26, 2024

(4) [WONT FIX]

Just a tiny detail about wording:

The Share button on the bottom of the screen does two actions:

  • First, save the link
  • Then, shares the link by displaying the share sheet

I'd call the button Create & Share, maybe less confusing. Also, affecting the suggestion in (3) about the posibility of creating new button in edit mode.

iPhoneXR v17.4.1
b4ab539bd

- ios-sdk:
	- add basic support for OC10 password policies
	- fix password generator error due to "empty" password policies derived from capabilities (finding 2)
- ShareViewController:
	- support for the requirement to set a password by disabling buttons and adding a warning triangle if no password is set (finding 1)
	- support for expiration date constraints:
		- support for the requirement by disabling buttons and adding a warning triangle if no date is set
		- add support for maximum date
		- add support for pre-setting an expiration date
	- add a "Copy" button next to the password if the password is known (finding 3)
	- clean up button creation code, avoiding duplication
@felix-schwarz
Copy link
Contributor Author

@jesmrec Thanks for spotting all of this. Let me go through (1) - (4) in chronological order:
(1) The UI now respects if a password is required. It disables the respective buttons if no password is set and also adds a ⚠️ if no password has been set.
(2) That was a consequence of OC10 using the same name space for password policies (PPs), but a different format, so that PPs derived from capabilities ended up nonsensical, preventing the generator from generating a password. I've added basic support for OC10 PPs now, and also some logic checks to ensure incomplete PPs are brought up to a minimum standard (minimum number of characters). That should fix the issue.
(3) I've added a Copy button next to the password. I want to avoid an automatic copy to clipboard because it could overwrite something important the user has stored there at the time. Adding a Share button here creates a UI issue insofar as it could only be used if the password has been re-generated and remain grayed out otherwise, possibly confusing users.
(4) I initially called it Create & Share but reduced it to Share after I realized that meant running out of space in certain localizations, which would require a different button layout.

@jesmrec
Copy link
Contributor

jesmrec commented Apr 1, 2024

(1) -> was the improvement pushed? i still see the same behaviour as before (as ebad324e):

RPReplay_Final1711956649.MP4

(2) -> fixed. Just as a comment, the applied policy is not standard with minimum chars, it applies all existing criteria. That's OK for me (enforces security)

Screenshot 2024-04-01 at 09 49 21

(3) -> fixed

(4) -> OK

So, (1) may need other review, the others are OK

@hosy hosy self-requested a review April 3, 2024 08:49
Copy link
Collaborator

@hosy hosy left a comment

Choose a reason for hiding this comment

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

@felix-schwarz CR looks good to me, the only thing I would consider is to change At most / Less than xx bytes.
Would be better to give the user a feedback who many characters are needed instead of bytes.

@felix-schwarz
Copy link
Contributor Author

felix-schwarz commented Apr 4, 2024

@hosy Thanks for the feedback. I fully agree that characters would be more intuitive here, yet the criteria from ocis really is about byte count which can vary per character depending on what character is entered.

From the docs:

Note that a password can have a maximum length of 72 bytes. Depending on the alphabet used, a character is encoded by 1 to 4 bytes, defining the maximum length of a password indirectly. While US-ASCII will only need one byte, Latin alphabets and also Greek or Cyrillic ones need two bytes. Three bytes are needed for characters in Chinese, Japanese and Korean etc.

…ring.public.password.enforced in capabilities is false, but the respectively matching files_sharing.public.password.enforced_for is true
@felix-schwarz
Copy link
Contributor Author

felix-schwarz commented Apr 11, 2024

@jesmrec The issue here was that capabilities indicated files_sharing.public.password.enforced as false whereas the values for files_sharing.public.password.enforced_for values were true.

Other than ocis, OC10 reports enforced is true in case any of the enforced_for cases are true. This makes sense insofar as ocis added a new internal permission (called Invited people in the UI), for which a password can't be set, but for which a password would be required if files_sharing.public.password.enforced is true.

Bildschirmfoto 2024-04-11 um 12 01 39

I've changed the implementation to enforce a password if the selected permissions match a respective enforced_for case for which true (enforcement) is indicated. In my testing, this worked well for all roles/permissions, including Invited people.

Please run another test to verify you now get the same results.

@jesmrec
Copy link
Contributor

jesmrec commented Apr 12, 2024

Tested for the following cases:

  • Enforce password protection for read-only links
  • Enforce password protection for read + write links
  • Enforce password protection for read + write + delete links
  • Enforce password protection for upload-only (File Drop) links

@jesmrec
Copy link
Contributor

jesmrec commented Apr 12, 2024

I've changed the implementation to enforce a password if the selected permissions match a respective enforced_for case for which true (enforcement) is indicated. In my testing, this worked well for all roles/permissions, including Invited people.

That means that you only mind the enforced_for and reject the enforced, right @felix-schwarz ?

In my test server (oCIS 5.0.0), i'm getting:

"password": {
                            "enforced": false,
                            "enforced_for": {
                                "read_only": true,
                                "read_write": true,
                                "read_write_delete": true,
                                "upload_only": true
                            }
                        },

with such setup, the viewer, uploaded, contributor and editor roles are password-enforced but the invited persons (this one requires login, so there is an existing security layer). Is that expected then? so enforced and enforced_for don't seem to be related

@felix-schwarz
Copy link
Contributor Author

@jesmrec
If enforced is true, the app requires a password for all links.

If enforced is false, the app goes on to check if any of the enforced_for cases that are true apply to permissions of the selected role. If there's a match, it requires a password for these.

For Invited people links, according to the text in the screenshot from ocis-web above, passwords aren't supported by the server. Whereas all other roles for links use a combination of read, create, update, delete permission, the invited people role uses a new internal permission (and only that).

So far as I understand it then, the server picks values for enforced and enforced_for wisely:

  • by sending false for enforced, capabilities don't indicate a general requirement for passwords for links. That allows invited people links to be created without password.
  • for all other roles, the password requirement is enforced by enforced_for cases, as all of them target combinations of permissions used by the other roles only (read, create, update, delete). The invited people role on the other hand only uses the internal permission, so isn't covered by enforced_for at all.

Hope that clarifies it.

@jesmrec
Copy link
Contributor

jesmrec commented Apr 12, 2024

Ok, that covers all the cases i tested in advance. We can move forward this one

Approved

@jesmrec jesmrec added the Approved by QA Approved by QA label Apr 12, 2024
@felix-schwarz felix-schwarz merged commit f0399b0 into milestone/12.2 Apr 12, 2024
3 of 4 checks passed
@delete-merged-branch delete-merged-branch bot deleted the feature/password-policy branch April 12, 2024 14:45
hosy pushed a commit that referenced this pull request May 8, 2024
* - SDK update
- ShareViewController: add "generate" button to the password field that uses the password policy's generator to generate a password
- BottomButtonBar: add new alternative button to allow for an alternative main action; fix layout bug

* - SDK update: latest password policy progress and bug fixes
- PasswordComposerViewController:
	- new view controller to compose passwords
	- interactive feedback based on OCPasswordPolicy
	- integration with password generator based on OCPasswordPolicy
	- ability to copy passwords to clipboard
	- ability to show/hide entered passwords
	- support for editing and creation of password strings
- ShareViewController:
	- replace UIAlert with PasswordComposerViewController for entering passwords
	- add Generate button to generate a password based on the currently applicable password policy
	- add new "Share" button for links (in addition to "Create") that invoke the share sheet to directly share a link (including password) to the clipboard or directly to other apps like Mail or Messages
- add missing localizations
- BottomButtonBar: include alternativeButton in .modalActionRunning auto-enable/disable
- SegmentViewItem / SegmentViewItemView:
	- add extension to easily create button segments
	- add support for UIImage rendering modes for .icon
- ThemeCollection: add CSS entry for proper PasswordComposerViewController cell background coloring in dark mode

* - add line to copy the password to the clipboard (accidentally left out from previous commit)

* - update SDK to address findings
- PasswordComposerViewController: add notification upon copying password to clipboard

* Address findings (1) (2) (3) in #1325:
- ios-sdk:
	- add basic support for OC10 password policies
	- fix password generator error due to "empty" password policies derived from capabilities (finding 2)
- ShareViewController:
	- support for the requirement to set a password by disabling buttons and adding a warning triangle if no password is set (finding 1)
	- support for expiration date constraints:
		- support for the requirement by disabling buttons and adding a warning triangle if no date is set
		- add support for maximum date
		- add support for pre-setting an expiration date
	- add a "Copy" button next to the password if the password is known (finding 3)
	- clean up button creation code, avoiding duplication

* - ShareViewController: enforce password requirement even if files_sharing.public.password.enforced in capabilities is false, but the respectively matching files_sharing.public.password.enforced_for is true

---------

Co-authored-by: felix-schwarz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved by QA Approved by QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants