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 Remote avatars #14856

Merged
merged 1 commit into from
Aug 29, 2015
Merged

Allow Remote avatars #14856

merged 1 commit into from
Aug 29, 2015

Conversation

rullzer
Copy link
Contributor

@rullzer rullzer commented Mar 13, 2015

PR for #14564

Added a function to the avatar controller to deal with remote avatar calls. This function takes as input the userId, size and the share token.

This share token is used to retrieve the share, It is then matched against the userId. There are two possible scenario's.

If userA (on instance A) shares with userB (on instance B).

  • userB can request the avatar from userA, but user A cannot yet request the avatar from userB.
  • if userB has accepted the share userA can also request the avatar from userB.

Still todo:

  • Some new js logic is required to properly fetch remote avatars.
  • UserA should check if the share is accepted before sending the request

Future work:

  • Cacheing

@rullzer
Copy link
Contributor Author

rullzer commented Mar 13, 2015

@LukasReschke passing the share token around is not a problem I assume since we do it as well to initiate s2s shares?

CC @PVince81

@LukasReschke
Copy link
Member

Mhm. Thinking twice about it in my opinion it's also okay to not require any share token at all and just expose all avatars. That would allow other use-cases as well.

But that would then need to be documented somewhere. I can take care of that…

@rullzer
Copy link
Contributor Author

rullzer commented Mar 13, 2015

That would be simpler indeed.

@LukasReschke
Copy link
Member

Generally speaking: Exposing information is okay as long as we don't give a full list of users to unauthenticated 3rdparties and it is documented and can so be considered part of the threatmodel.

return new DataResponse([], Http::STATUS_BAD_REQUEST);
}

return $this->getAvatar($userId, $size);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a check for $size to avoid problems when someone tries to fool us?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the getAvatar function already checks size

@rullzer
Copy link
Contributor Author

rullzer commented Mar 16, 2015

If we will allow all avatars to be fetched anyway this PR is obsolete. Since then everybody could just use the default avatar interface.

However, this would make it possible to get the entered name of the user if no avatar is set. Is this acceptable?

@DeepDiver1975
Copy link
Member

rescheduling this for 8.2 - we are entering freeze for 8.1 tomorrow

@DeepDiver1975 DeepDiver1975 modified the milestones: 8.2-next, 8.1-current Mar 30, 2015
@jancborchardt
Copy link
Member

@rullzer @schiesbn what’s the status here? This is pretty cool for federated sharing. :)

@nickvergessen
Copy link
Contributor

@jancborchardt the status is 8.2 is less important then 8.1 at the moment ;)

@rullzer
Copy link
Contributor Author

rullzer commented Jul 1, 2015

@jancborchardt well it should not be hard.

Howeverer,

  • we need to agree on an endpoint that can be used and will not change (CC: @DeepDiver1975)
  • we do not want to leak to much sensitive information (CC: @LukasReschke)

Just serving the image is easy... the other stuff might need some discussion. Also not that hard but choices have to be made :)

@rullzer
Copy link
Contributor Author

rullzer commented Jul 21, 2015

I'd like to continue this. So that 8.2 can have awesome remote avatars!

The code now has tests!

@DeepDiver1975 the current path is: index.php/avatar///remote?token= is that fine or should that be modified? For me it makes sense to have all the avatar routes grouped

@LukasReschke should I switch back to 1 response code to make it harder to get info?

/**
* Return an avatar for remote shares. If userA shares with userB there are multiple scenarios
* - userB did not yet accept the share. userB can retrieve avatar of userA with the token
* - userB accepts the share. Now userA can retrieve the avatar of UserB
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with this is, that the activity app can therefor not make any use of remote avatars at all?
Or we need to make two requests: 1. check if the image is served and then embed the code for the avatar 2. load the avatar
Are avatars really so secret, that we can not allow displaying them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it would make the code easier of course. And indeed make avatars also better usable for other apps.

I don't think they are that secret. But maybe other people do...

@LukasReschke
Copy link
Member

Thinking about this again I'd just expose avatar#getAvatar to the public. Reduces code and avatars are not that sensitive.

@rullzer
Copy link
Contributor Author

rullzer commented Jul 27, 2015

@LukasReschke ok that should be easier. Of course it then could also expose the full name (if no avatar is set).

I'll modify the PR. Should get a lot simpler.

@rullzer
Copy link
Contributor Author

rullzer commented Jul 28, 2015

That is better. avatar#getAvatar is public now.

  • Extra test is added if the user exists.
  • Unit tests are updated.

Pretty simple really.

Review time.

@rullzer rullzer changed the title [WIP] Remote avatars Allow Remote avatars Jul 28, 2015
@rullzer
Copy link
Contributor Author

rullzer commented Aug 6, 2015

Review time: @LukasReschke @MorrisJobke @PVince81 @Xenopathic @nickvergessen

should not be hard.

@@ -84,12 +84,18 @@ public function __construct($appName,

/**
* @NoAdminRequired
Copy link
Member

Choose a reason for hiding this comment

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

PublicPage implies this. Redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed :)

@LukasReschke
Copy link
Member

Besides my remark: Should be good to go 👍

@DeepDiver1975
Copy link
Member

so this is supposed to be a public api - right?
Should be done via OCS or WebDAV

@LukasReschke
Copy link
Member

so this is supposed to be a public api - right?
Should be done via OCS or WebDAV

OCS can per definition not serve arbitrary binary content. We really do not want to go base64 encode data for this purpose.

WebDAV, well… would require some more insight on the plans here first.

@DeepDiver1975
Copy link
Member

WebDAV, well… would require some more insight on the plans here first.

DeepDiver1975/dav#2

@DeepDiver1975
Copy link
Member

OCS can per definition not serve arbitrary binary content.

hmmm

@rullzer
Copy link
Contributor Author

rullzer commented Aug 19, 2015

So probably best to wait with this PR until after the conference where @DeepDiver1975 will present us his idea's for more webdav related stuff.

@rullzer
Copy link
Contributor Author

rullzer commented Aug 29, 2015

@jancborchardt this is the PR for the avatars

@MorrisJobke
Copy link
Contributor

@jancborchardt this is the PR for the avatars

... and it needs a rebase ;)

@scrutinizer-notifier
Copy link

A new inspection was created.

@ghost
Copy link

ghost commented Aug 29, 2015

🚀 Test PASSed.🚀
chuck

@MorrisJobke
Copy link
Contributor

Works 👍

MorrisJobke added a commit that referenced this pull request Aug 29, 2015
@MorrisJobke MorrisJobke merged commit 90dfa98 into owncloud:master Aug 29, 2015
@DeepDiver1975
Copy link
Member

why was this merged?

there are open todos and the approach is still questionable

@tobiasKaminsky
Copy link
Contributor

@DeepDiver1975 Any news about this PR?
We, android app, want to integrate this, but then the api should stay the same?

@lock
Copy link

lock bot commented Aug 7, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants