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

allow configuration of one single password per circle #926

Merged
merged 1 commit into from
Mar 7, 2022

Conversation

ArtificialOwl
Copy link
Member

@ArtificialOwl ArtificialOwl commented Feb 16, 2022

  • Managing circle's settings from the command line

  • Allow multiple configuration to use this feature (in order):

    • Enforcing password protection if the Enforce password protection is enabled.
    • Enforcing password protection if the Circle App is configured to:
      ./occ config:app:set --value '1' circles enforce_password
    • Enforcing password protection if the Circle's settings is configured to:
      ./occ circles:manage:setting y8JczmViR3zZraCXhbpfR8v4x8s5A8c enforce_password true
  • Allow the use of a single password when sharing a new file to external members of a circle,

  • Allow the use of a single password when adding a new external member to a circle,

  • confirm behavior in globalscale env.

  • providing API and Feature Request for Contacts

  • filtering some setting before sending ocs data about circle

display settings from a circleId

$ ./occ circles:manage:setting y8JczmViR3zZraCXhbpfR8v4x8s5A8c 
{
    "enforce_password": "true",
    "password_single_enabled": "true",
    "password_single": "3|$argon2id$v=19$m=65536,t=4,p=1$NktudFp4UmluSGZtRGhEeA$VJuD1vhKuKG+oFFT1nlmXHU3O3aTUw1ZFkjW8Vw9gxU"
}

Add/Edit setting from a circleId

$ ./occ circles:manage:setting y8JczmViR3zZraCXhbpfR8v4x8s5A8c enforce_password true
$ ./occ circles:manage:setting y8JczmViR3zZraCXhbpfR8v4x8s5A8c password_single_enable true
$ ./occ circles:manage:setting y8JczmViR3zZraCXhbpfR8v4x8s5A8c password_single my_clear_password

Note: when setting the param password_single the clear password will be instantly be hashed

Remote a setting from a circleId

$ ./occ circles:manage:setting y8JczmViR3zZraCXhbpfR8v4x8s5A8c enforce_password --unset

@ArtificialOwl
Copy link
Member Author

/backport to stable23

@ArtificialOwl
Copy link
Member Author

/backport to stable22

// TODO: deprecated in NC27, remove those (17) lines that was needed to finalise migration to 24
// if password is not hashed (pre-22), hash it and update new settings in DB
$curr = $this->get('password_single', $this->getSettings());
if (strlen($curr) > 1 && strlen($curr) < 64) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be strlen($cur) >= 1 && ...? If a password could be strlen(2) before, I suspect it might have been strlen(1) as well? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

true, who will use a 1-char password ? :)

@@ -290,21 +294,34 @@ public function updateConfig(string $circleId, int $config): array {
* @throws RequestBuilderException
* @throws UnknownRemoteException
*/
public function updateName(string $circleId, string $name): array {
public function updateSetting(string $circleId, string $setting, ?string $value): array {
Copy link
Member

Choose a reason for hiding this comment

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

I'm still not sure whether it's a good idea to allow any string as a settings key. Why not maintain a list of allowed settings and error out if users are trying to set an unsupported settings key?

As it is currently - and with only password_single being hashed, I expect plaintext passwords to end up in the database due to typos by the users. E.g. when you do something like occ circles:manage:setting <circle_id> password_sigle my_secret_password (note the typo in "sigle").

Copy link
Member Author

Choose a reason for hiding this comment

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

I am fine with it. 99% of the people will use the front-end to set those settings (with pre-defined keys)

@mejo- mejo- self-requested a review March 3, 2022 11:03
Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Sorry, accidently approved despite having comments/questions. Changed to "comment" now.

@mejo- mejo- self-requested a review March 3, 2022 11:03
Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

again

Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants