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

Honor avatar visibility settings #17715

Merged
merged 3 commits into from
Dec 4, 2019
Merged

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Oct 28, 2019

Fixes #5456
Only when an avatar is set to public should we show it to the public.
For now this has an open question as to how to solve federated avatars.
But I assume a dedicated paramter or endpooint would make sense there.

Todo:

  • Fix tests (most likely)
  • Think about federated avatars

Fixes #5456
Only when an avatar is set to public should we show it to the public.
For now this has an open question as to how to solve federated avatars.
But I assume a dedicated paramter or endpooint would make sense there.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer added bug 2. developing Work in progress labels Oct 28, 2019
@rullzer rullzer added this to the Nextcloud 18 milestone Oct 28, 2019
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer
Copy link
Member Author

rullzer commented Nov 13, 2019

For federated avatars we need a new endpoint anyways to verify their token etc. Lets leave that till later.

@rullzer rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 13, 2019
@rullzer
Copy link
Member Author

rullzer commented Nov 13, 2019

To test. Set your avatar level in the personal settings.
And request the avatar. Depending how it is set you should see it or get a 404.

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Works!

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Code looks good!

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Nov 14, 2019
@rullzer rullzer added 3. to review Waiting for reviews and removed 4. to release Ready to be released and/or waiting for tests to finish labels Nov 14, 2019
@rullzer
Copy link
Member Author

rullzer commented Nov 14, 2019

@nickvergessen you first wanted to test something right?

@nickvergessen
Copy link
Member

The Contacts level does not work. Should we just remove it? Since we don't send any referrers, we can not know if the image was requested by a remote instance in the allowed space. Or are avatars uploaded to the lookup server?

@skjnldsv
Copy link
Member

skjnldsv commented Dec 4, 2019

I have no clue what that means Joas!
I'll follow you to what you think is best

@rullzer
Copy link
Member Author

rullzer commented Dec 4, 2019

The Contacts level does not work. Should we just remove it? Since we don't send any referrers, we can not know if the image was requested by a remote instance in the allowed space. Or are avatars uploaded to the lookup server?

We should fix it. I'd leave it for now. And see if we can come up with and endpoint to request remote avatars where you send your share token for example.

@nickvergessen
Copy link
Member

Okay, then lets merge

@nickvergessen nickvergessen merged commit 738e6bf into master Dec 4, 2019
@nickvergessen nickvergessen deleted the fix/5456/respect_avatar_privacy branch December 4, 2019 09:28
@tobiasKaminsky
Copy link
Member

Damn, I saw this too late.
@rullzer this will break our clients as they assume that we get an image back.
Returning something else was previously also the case (then you got the userId back).

Can we adjust this to show the generic colored round avatar with first char in it?

@rullzer
Copy link
Member Author

rullzer commented Dec 4, 2019

@tobiasKaminsky no it won't. As long as you send your credentials you should get back the image.

@tobiasKaminsky
Copy link
Member

Clients also use https:///nextcloud/index.php/avatar/USER/ this to get avatars from other users (on the same instance [currently no federation]) when we do a search, or show it in file list if file was shared by an user.

@rullzer
Copy link
Member Author

rullzer commented Dec 4, 2019

Clients also use https:///nextcloud/index.php/avatar/USER/ this to get avatars from other users (on the same instance [currently no federation]) when we do a search, or show it in file list if file was shared by an user.

yes I know. But read the code.

https://github.com/nextcloud/server/pull/17715/files#diff-17d17c6a954f29c12717db921f235531R134-R137

If the avatar is not public. And you do an anonymous request we return the 404.
So as long as you authenticated request your avatars all is fine.

@tobiasKaminsky
Copy link
Member

True, stupid from me, I read only have of the code :-(
Sorry for the noise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Privacy: Avatar public available
5 participants