Skip to content

[IMPROVE] Get avatar from oauth#14131

Merged
rodrigok merged 8 commits intodevelopfrom
avatar-custom-oauth
Apr 16, 2019
Merged

[IMPROVE] Get avatar from oauth#14131
rodrigok merged 8 commits intodevelopfrom
avatar-custom-oauth

Conversation

@geekgonecrazy
Copy link
Contributor

Implements what I started with #9045

A lot has changed so figured i'd start over.

Brings oauth avatar in and sets it when registering with oauth provider.

Also exposes avatarUrl in info endpoint so our oauth server is providing the avatar url

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14131 April 13, 2019 00:38 Inactive
@geekgonecrazy geekgonecrazy requested a review from rodrigok April 13, 2019 00:38
@graywolf336
Copy link
Contributor

Can we get this on v1.0 as well? Would help out several people and I don't see any issue with getting it merged :)


addUserRoles(_id, roles);

if (settings.get('Accounts_SetDefaultAvatar') === true) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only portion I don’t like... but it’s needed to add on creation where the username is set by the so provider. If the username is not set then the logic in the setUsername flow works and sets the avatar

@geekgonecrazy
Copy link
Contributor Author

geekgonecrazy commented Apr 13, 2019

Yes hopefully we can get it and the other few oauth in the pipeline merged. Will really help round out oauth as a serious way to do things

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14131 April 15, 2019 21:52 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14131 April 15, 2019 21:57 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14131 April 15, 2019 22:03 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14131 April 15, 2019 22:05 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14131 April 15, 2019 22:06 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14131 April 15, 2019 22:30 Inactive
rodrigok
rodrigok previously approved these changes Apr 15, 2019
@rodrigok
Copy link
Member

@geekgonecrazy can you fix the lint?

@geekgonecrazy
Copy link
Contributor Author

@rodrigok done

@rodrigok rodrigok merged commit 97393fa into develop Apr 16, 2019
@rodrigok rodrigok deleted the avatar-custom-oauth branch April 16, 2019 13:39
@rodrigok rodrigok mentioned this pull request Apr 28, 2019
@ralfbecker
Copy link
Contributor

@geekgonecrazy This is NOT working for me:
OAuth config does not contain the avatar_field, but even if I add it directly to MongoDB:

rs0:PRIMARY> db.rocketchat_settings.insert({ "_id" : "Accounts_OAuth_Custom-Egroupware-avatar_field", "_updatedAt" : ISODate("2019-04-28T09:38:24.674Z"), "autocomplete" : true, "blocked" : false, "createdAt" : ISODate("2019-04-23T16:15:33.886Z"), "group" : "OAuth", "hidden" : true, "i18nDescription" : "Accounts_OAuth_Custom-Egroupware-avatar_field_Description", "i18nLabel" : "Accounts_OAuth_Custom_Avatar_Field", "packageValue" : "picture", "persistent" : true, "section" : "EGroupware", "sorter" : 80, "ts" : ISODate("2019-04-23T16:15:33.887Z"), "type" : "string", "value" : "picture", "valueSource" : "packageValue" })
WriteResult({ "nInserted" : 1 })
rs0:PRIMARY> db.rocketchat_settings.find({_id: /egroupware/i},{_id:1,value:1})
{ "_id" : "Accounts_OAuth_Custom-Egroupware", "value" : true }
{ "_id" : "Accounts_OAuth_Custom-Egroupware-access_token_param", "value" : "access_token" }
{ "_id" : "Accounts_OAuth_Custom-Egroupware-authorize_path", "value" : "/authorize" }
{ "_id" : "Accounts_OAuth_Custom-Egroupware-avatar_field", "value" : "picture" }
{ "_id" : "Accounts_OAuth_Custom-Egroupware-button_color", "value" : "#1d74f5" }
{ "_id" : "Accounts_OAuth_Custom-Egroupware-button_label_color", "value" : "#FFFFFF" }
{ "_id" : "Accounts_OAuth_Custom-Egroupware-button_label_text", "value" : "EGroupware users click here" }
{ "_id" : "Accounts_OAuth_Custom-Egroupware-id", "value" : "rocket.chat" }
{ "_id" : "Accounts_OAuth_Custom-Egroupware-identity_path", "value" : "/userinfo" }
{ "_id" : "Accounts_OAuth_Custom-Egroupware-identity_token_sent_via", "value" : "header" }
{ "_id" : "Accounts_OAuth_Custom-Egroupware-login_style", "value" : "redirect" }
{ "_id" : "Accounts_OAuth_Custom-Egroupware-merge_roles", "value" : true }
{ "_id" : "Accounts_OAuth_Custom-Egroupware-merge_users", "value" : true }
{ "_id" : "Accounts_OAuth_Custom-Egroupware-roles_claim", "value" : "roles" }
{ "_id" : "Accounts_OAuth_Custom-Egroupware-scope", "value" : "openid email profile roles" }
{ "_id" : "Accounts_OAuth_Custom-Egroupware-secret", "value" : "pnGVYhE8R307aAJe" }
{ "_id" : "Accounts_OAuth_Custom-Egroupware-token_path", "value" : "/access_token" }
{ "_id" : "Accounts_OAuth_Custom-Egroupware-token_sent_via", "value" : "payload" }
{ "_id" : "Accounts_OAuth_Custom-Egroupware-url", "value" : "https://bb-trunk.egroupware.de/egw/openid/endpoint.php" }
{ "_id" : "Accounts_OAuth_Custom-Egroupware-username_field", "value" : "id" }

Accounts_SetDefaultAvatar is on:

rs0:PRIMARY> db.rocketchat_settings.find({_id: /Accounts_Set/i},{_id:1,value:1})
{ "_id" : "Accounts_SetDefaultAvatar", "value" : true }

After initial login, I can get to User >> My account >> Profile and see that provider "egroupware" has set a picture, but it is not selected by default. Every user has to do that manually.

Do you want me to open a bug-report for that?

I can also have a look in the code, thought adding that setting requires a database migration, as far as I understand, never done that for Meteor.

Ralf

@geekgonecrazy
Copy link
Contributor Author

It only does for new users. Forcing on existing users would be bad UX I think.

Regarding the setting... we probably should have included a migration to add the new oauth fields to existing custom oauth

@ralfbecker
Copy link
Contributor

ralfbecker commented Apr 28, 2019 via email

@ralfbecker
Copy link
Contributor

ralfbecker commented Apr 28, 2019 via email

@geekgonecrazy
Copy link
Contributor Author

Stores it for convenience and for when you goto profile to change it its available as an option

@ralfbecker
Copy link
Contributor

ralfbecker commented Apr 29, 2019 via email

@ralfbecker
Copy link
Contributor

I'll try to debug, why the avatar is not set for me, when a user logs in the first time.

I came across an other missing OAuth feature, at least OpenID Connect standard claims, which are returned eg. by /userinfo endpoint, contain a locale eg. "DE_de" for German language. That would also make sense to be used when a user logs in for the first time to set his preferred language. What do you think?

@ralfbecker
Copy link
Contributor

Ok, got a bit further: our first login happens via the Rocket.Chat Api. Api creates the user, if it does not exist, but it not even adds the avatar to the user, left alone makes it the default avatar.
Unfortunately our call to Rocket.Chap Api is always much quicker then the user :(
Would it be possible that Api uses the same code to create a user, as regular logins to Rocket.Chat?
What do you think?

@ralfbecker
Copy link
Contributor

I can confirm, if I disable our Rocket.Chat API login, I get the avatar.
So problem is the first login via API, which does not set the avatar.

@ralfbecker
Copy link
Contributor

@geekgonecrazy Can you comment on this:
If user is created via an API call with Bearer token, the avatar is not added, not even to MongoDB.
That behavier is most unfortunate for us. I'd like to change it in one of the following ways:
a) reject API calls made with non-existing users (can also be a setting or a header on the request)
b) API calls with non-existing users create a complete user, like the interactive login does
I'm happy with a), as it leaves the choice to the user, if he wants to be added to RC or not.

Please also comment on how I should / can proceed with the locale/language mentioned above.

Ralf

@geekgonecrazy
Copy link
Contributor Author

If user is created via an API call with Bearer token, the avatar is not added, not even to MongoDB.
That behavier is most unfortunate for us.

Do you mean when using oauth provider access token? Or an rc token using the create user api?

If using oauth provider access token.. and the user doesn’t exist it should create, and it should use the avatar.

The api create user how ever would not since its not doing anything oauth related

@ralfbecker
Copy link
Contributor

I'm talking about using an oauth access-token from my own oauth provide to access the api (listing online users) for a now yet existing user.

It does create that user, but NOT setting the avatar!
It seems to use a different code-path. I can also see in users collection the avatar info (from oauth) is missing.

So you agree that, if it creates the user, it should do so like for a regular login?

What do you think about a header or setting not to create not yet existing users on arbitrary api calls?
This is important for GDPR compliance (sending the email address) and it's hard for us to track, if a user already logged in himself, or our api call to list online users is first.

Ralf

@geekgonecrazy
Copy link
Contributor Author

Hm... it shouldn’t be creating on arbitrary calls. This should only be happening on login. If login is called I’d say there is an intent to access.

That being said I think there is a setting somewhere.

Regarding the avatar... it must be following a different code path. Will need to create an issue and track this down.

@ralfbecker
Copy link
Contributor

With arbitrary I meant on login, but to do an arbitrary api call, not create a user.

That setting would be good :)

@ralfbecker
Copy link
Contributor

What about the language of new users, if we get that via oauth?

Should we also create an issue / feature request for that, or how do you want to handle it?

@geekgonecrazy
Copy link
Contributor Author

geekgonecrazy commented May 7, 2019

Yes feel free to create an issue with as much details about it as you can. I don't think i've seen language included in responses before, so will likely need to be another field to specify. Also need to be sure about the format of the language string to be sure its the same as ours.

@lordrhodos
Copy link

lordrhodos commented Jan 23, 2021

@geekgonecrazy

Regarding the avatar... it must be following a different code path. Will need to create an issue and track this down.

I think I stumbled across the same behaviour here and wonder if any issue has already been created?

In my case I have a custom OAuth provider configured integrating our identity provider as login option. If I login with the OAuth login on the login form the avatar gets read from the userinfo endpoint of the Identity provider and set, all good.

In our frontend app I login the user via the rest api with the custom oauth provider and the avatar is not set.

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.

6 participants

Comments