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

change default profile visibilty to users-only #47170

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cherti
Copy link

@cherti cherti commented Aug 10, 2024

  • Resolves: # none, I can open one in addition to sending the fix if the process requires that

Summary

This commit changes the default visibility of all profile properties to logged-in users only, so that users do not accidentially leak part of their information assuming that a privacy-oriented software like nextcloud would not expose their profile information to the outside by default.
This is especially relevant given that the profile visibility settings only show up on the main screen depending on resolution and size, see screenshots below. There is the "show profile visibility", but it is subtle and I consider it a valid assumption that the default settings for users of a privacy- and user-oriented product like nextcloud are unsurprising and safe by default without the need to take a look. If they are not, it is just a question of time until someone has their profiles with settings they don't want or intend.
This PR only changes the default values, no functionality is added or removed. The change also, as far as I can tell, only affects newly created profiles, and public visibility can be restored with a click in the profile visibility settings if desired.

nextcloud1

nextcloud2

Checklist

This commit changes the default visibility of all profile properties to
logged-in users only, so that users do not accidentially leak part of
their information assuming that a privacy-oriented software like nextcloud
would not expose their profile information to the outside by default.
@susnux
Copy link
Contributor

susnux commented Aug 10, 2024

Not really sure who to ask if we should adjust those fields, as they are not that private fields and in the default use case you probably want them to be public. For example "role" or "organization" you do not use them for personal use but on enterprise and then you probably want them accessible.

So maybe ask @jancborchardt ?

@cherti
Copy link
Author

cherti commented Aug 10, 2024

Well, in the current configuration they are world-readable. So every Nextcloud as a service for small businesses and organisations, basically everyone who doesn't have an intranet or that Nextcloud on the intranet, has them world-exposed in the current default configuration. If an organization has enough staff to host this on an intranet, they might have enough staff to change the default configurations if they want to, but especially smaller organisations, also voluntary organisations, are currently vulnerable to everyone having to remember changing that visibility to not having it exposed. It just feels like a very non-sane default especially given who significant portions of the userbase of Nextcloud are consists of, especially given that these defaults are hardcoded in the codebase, there is no way of configuring those defaults in the admin-UI of Nextcloud.

Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

So we would like to promote people using the profile, possibly having it useful as some sort of federated address book. If the default is changed like this, people will likely never enable it.

@cherti your use-case of course is reasonable, and to achieve that I would rather say there should be an admin setting (or config.php setting). @AndyScherzinger @sorbaugh

@SergioArbarviro
Copy link

I thank you @cherti for this commit. It goes in the right direction, but in my views not far enough. As I mentioned in #32086 , I believe that the defaults should be:

  • logged in users only for: username, profile text, organisation, position and avatar
  • "private" (visible only to the user him/herself) for all other fields, in particular: e-mail address, location, telephone number, etc that relate to personal and identifiable data as described in the EU General Data Protection Regulation - GDPR. As a citizen of the EU, I expect a privacy-oriented software like NextCloud, developed moreever in the EU, to comply by default with this legislation.

@cherti
Copy link
Author

cherti commented Aug 28, 2024

* "private" (visible only to the user him/herself) for **all** other fields, in particular: e-mail address, location, telephone number, etc

I agree for the e-mail field, for the rest you could just not fill this. However, for e-mail making this private here seems insufficient, because Nextcloud also provides a built-in address book of all users of that Nextcloud. That would have to be changed as well to make this change effective, and I think this is beyond the scope of this PR.

@cherti
Copy link
Author

cherti commented Aug 28, 2024

So we would like to promote people using the profile, possibly having it useful as some sort of federated address book. If the default is changed like this, people will likely never enable it.

The current situation is that people will never use the profile if they found out that it unintentionally by them exposed information to the wide internet. I think that breach of trust will have a significantly higher impact on this not becoming a federated address book than people opting to not use this as a federated address book. If there was an admin setting to change these defaults, this would likely be a non-issue (even though I still think it doesn't act upon the "least surprise" principle given that one of Nextcloud's points of pride is privacy), but there is no such thing. There is no way as an admin to influence this setting except with fiddling with the code.
As a matter of fact, the only reason we are using these profiles (and we actually want to) is because in the setup that we have, I can monkeypatch the Nextcloud code to remedy this issue. Otherwise we would flatout disable profiles alltogether. So I think you are actually harming adoption of profiles with the current state of these visibilities.

@cherti your use-case of course is reasonable, and to achieve that I would rather say there should be an admin setting (or config.php setting). @AndyScherzinger @sorbaugh

I am indeed irritated why this was introduced this way without having an admin setting for it. I spend good chunks of time looking for it before realizing that it simply does not exist. Hence this PR.

@susnux
Copy link
Contributor

susnux commented Aug 28, 2024

"private" (visible only to the user him/herself) for all other fields, in particular: e-mail address, location, telephone number, etc that relate to personal and identifiable data as described in the EU General Data Protection Regulation - GDPR. As a citizen of the EU, I expect a privacy-oriented software like NextCloud, developed moreever in the EU, to comply by default with this legislation.

What reason would one have to fill in this information and then just see it themself?

@cherti
Copy link
Author

cherti commented Aug 29, 2024

@jancborchardt it says you requested changes, but I'm unable to see which changes are requested. Can you elaborate?

@AndyScherzinger
Copy link
Member

@cherti I think it is #47170 (review) itself where you referred to earlier already via #47170 (comment)

And I think that could/would be the way to go and fine to get merged then too 👍 from my perspective

@cherti
Copy link
Author

cherti commented Aug 29, 2024

Ok, but that is something I cannot provide as my familiarity with PHP and the entire environment does not extend to this.

I wholeheartedly agree that Nextcloud should have such a feature and until that gets implemented, I would propose tuning the defaults in a way that it doesn't accidentially leak people's information, and then change those defaults back once we have a setting that allows to set this, as done in this PR.

Accidentially exposing user's information to the outside doesn't seem to provide a way of instilling confidence to (and hence make people) use this feature, quite the opposite. At least it did not with the userbase I am involved in, why I am currently patching the nextcloud code prior to deployment on every update to prevent such leakage to happen in the future.

@AndyScherzinger
Copy link
Member

Ah, that I wasn't aware of. So I'll discuss the implementation part with @sorbaugh and @jancborchardt then 👍

@cherti
Copy link
Author

cherti commented Aug 29, 2024

Given that the scope of this PR is fairly minimal, and given that I know very well how busy everyone is, could we merge this as an immediate fix and then once the feature is fully implemented, see if these values need adjustment again? :)

@cherti
Copy link
Author

cherti commented Sep 13, 2024

@jancborchardt in #32086 (comment) :

I can not make any promises of course. As Nextcloud is an open community, if anyone with more deep PHP knowledge wants to have a go at it, please go ahead.
But I would stop discussing here since it is just making the thread longer and anyone who possibly would work on this more confused.

But if you can't make any promises, is there any harm in merging this now as the immediate fix and possibly adjusting it when somebody does the full implementation? Given it's an open community, somebody put in the work to prepare this simple fix for easy merging, after all, and it would resolve most of the issues for the time being until the full implementation of #32086 is done. :)

@susnux
Copy link
Contributor

susnux commented Sep 13, 2024

is there any harm in merging this now as the immediate fix and possibly adjusting it when somebody does the full implementation

It changes the default state for all to a state which we think is not an optional default state. There is also no way for instances that want the default behavior back to change it. So this PR here is quite a huge breaking change for a very limited use case.
So until the feature (restricting the default visibility) is implemented I would rather recommend you to adjust your user provisioning workflow to set the desired visibility for new users, e.g. using the OCS API:
https://docs.nextcloud.com/server/latest/developer_manual/_static/openapi.html#/operations/core-profile_api-set-visibility

@cherti
Copy link
Author

cherti commented Sep 13, 2024

So, to understand this correctly, you say this necessitates a fix after the fact which needs to be executed after every user registered? Or after the user activated their profile?

@SergioArbarviro
Copy link

"private" (visible only to the user him/herself) for all other fields, in particular: e-mail address, location, telephone number, etc that relate to personal and identifiable data as described in the EU General Data Protection Regulation - GDPR. As a citizen of the EU, I expect a privacy-oriented software like NextCloud, developed moreever in the EU, to comply by default with this legislation.

What reason would one have to fill in this information and then just see it themself?

There is at least one field that the user has no other choice than providing information on, it is the e-mail address, which is a personal data enabling identification, hence requiring protection under the EU GDPR.

NextCloud must, at least in the EU, make this information private by default, and not disclose it to anyone without the prior, explicit consent of the user - not, in my views, even to the other users of the NextCloud instance. This stands in particular for open communities.

@SergioArbarviro
Copy link

is there any harm in merging this now as the immediate fix and possibly adjusting it when somebody does the full implementation

It changes the default state for all to a state which we think is not an optional default state. There is also no way for instances that want the default behavior back to change it. So this PR here is quite a huge breaking change for a very limited use case.

I disagree that this is a "very limited use case". Privacy by default is the law in the whole European Union, as per the GDPR. I expect the European Union to constitute a very significant share of the installed base of NextCloud. I would not rate it as a "very limited use case", and don't see on the other hand the use case for disclosing one's personal data by default to the whole Internet. Even FaceBook keeps that data for itselft (and, presumably, for the US intelligence services).

@cherti
Copy link
Author

cherti commented Sep 15, 2024

Are there actually numbers of how many people actively use the federated address book functionality? Because my personal impression is that Nextcloud is primarily used by institutions and people who want to retain their data and not spread it. As @SergioArbarviro says, this is particularly a topic in Europe, and Nextcloud is recommended in these contexts specifically for privacy reasons, so I'm wondering if actual numbers exist on how this feature is actually used?

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.

5 participants