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

Chore: create setAvatarFromService endpoint #26402

Open
wants to merge 22 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
cbb99a9
add setAvatarFromService endpoint
felipe-rod123 Jul 28, 2022
d5b6daf
Merge branch 'develop' into chore/create-setAvatarFromService-endpoint
felipe-rod123 Aug 2, 2022
a93420b
Merge branch 'develop' into chore/create-setAvatarFromService-endpoint
felipe-rod123 Aug 2, 2022
5c896aa
add tests
felipe-rod123 Aug 2, 2022
8718a51
Merge branch 'develop' into chore/create-setAvatarFromService-endpoint
ggazzo Aug 8, 2022
5808244
fix tests
felipe-rod123 Aug 12, 2022
8d656d7
Merge branch 'develop' into chore/create-setAvatarFromService-endpoint
felipe-rod123 Aug 12, 2022
f3244b3
fix tests
felipe-rod123 Aug 12, 2022
6fcfa0a
add imgURL to tests
felipe-rod123 Aug 12, 2022
846d0ed
fix avatarURL
felipe-rod123 Aug 12, 2022
b25a654
Update apps/meteor/tests/data/interactions.js
felipe-rod123 Aug 17, 2022
b7b5dbf
Update apps/meteor/tests/data/interactions.js
felipe-rod123 Aug 17, 2022
fd12b3f
remove field property from tests
felipe-rod123 Aug 18, 2022
2340590
fix
felipe-rod123 Aug 18, 2022
f8ced02
fix test
felipe-rod123 Aug 18, 2022
3498f94
Merge branch 'develop' into chore/create-setAvatarFromService-endpoint
felipe-rod123 Aug 19, 2022
0bcd9e0
Merge remote-tracking branch 'origin/develop' into chore/create-setAv…
ggazzo Sep 13, 2022
c9d9b24
review
ggazzo Sep 13, 2022
61e4faa
fix lint
felipe-rod123 Sep 14, 2022
770c368
change 'blob' type to string
felipe-rod123 Sep 18, 2022
39e7142
Fix params
ggazzo Sep 21, 2022
ce2e59f
Merge branch 'develop' into chore/create-setAvatarFromService-endpoint
ggazzo Sep 21, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 38 additions & 0 deletions apps/meteor/app/api/server/v1/users.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
isUsersListTeamsProps,
isUsersAutocompleteProps,
isUsersSetAvatarProps,
isUsersSetAvatarFromServiceProps,
isUsersUpdateParamsPOST,
isUsersUpdateOwnBasicInfoParamsPOST,
isUsersSetPreferencesParamsPOST,
Expand Down Expand Up @@ -226,6 +227,43 @@ API.v1.addRoute(
},
);

API.v1.addRoute(
'users.setAvatarFromService',
Copy link
Member

Choose a reason for hiding this comment

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

there is no need to create a new endpoint.. we already have the endpoint users.setAvatar, if it is missing this functionality we should just improve it.

{
authRequired: true,
validateParams: isUsersSetAvatarFromServiceProps,
},
{
async post() {
const fields = this.bodyParams;
if (!settings.get('Accounts_AllowUserAvatarChange')) {
throw new Meteor.Error('error-not-allowed', 'Change avatar is not allowed', {
method: 'users.setAvatar',
});
}

const canEditOtherUserAvatar = hasPermission(this.userId, 'edit-other-user-avatar');

const user = ((): IUser | undefined => {
if (this.isUserFromParams()) {
return Meteor.users.findOne(this.userId) as IUser;
}
if (canEditOtherUserAvatar) {
return this.getUserFromParams();
}
})();

if (!user) {
return API.v1.unauthorized();
}

setUserAvatar(user, fields.blob, fields.contentType, fields.service);

return API.v1.success();
},
},
);

API.v1.addRoute(
'users.create',
{ authRequired: true, validateParams: isUserCreateParamsPOST },
Expand Down
11 changes: 6 additions & 5 deletions apps/meteor/client/hooks/useUpdateAvatar.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { AvatarObject, AvatarServiceObject, AvatarReset, AvatarUrlObj, IUser } from '@rocket.chat/core-typings';
import { useToastMessageDispatch, useMethod, useTranslation } from '@rocket.chat/ui-contexts';
import { useEndpoint, useToastMessageDispatch, useTranslation } from '@rocket.chat/ui-contexts';
import { useMemo, useCallback } from 'react';

import { useEndpointAction } from './useEndpointAction';
Expand All @@ -11,15 +11,17 @@ const isServiceObject = (avatarObj: AvatarObject): avatarObj is AvatarServiceObj
const isAvatarUrl = (avatarObj: AvatarObject): avatarObj is AvatarUrlObj =>
!isAvatarReset(avatarObj) && typeof avatarObj === 'object' && 'service' && 'avatarUrl' in avatarObj;

type AvatarObjectWithString = AvatarObject & { blob: string };

export const useUpdateAvatar = (
avatarObj: AvatarObject,
avatarObj: AvatarObjectWithString,
userId: IUser['_id'],
): (() => Promise<{ success: boolean } | null | undefined>) => {
const t = useTranslation();
const avatarUrl = isAvatarUrl(avatarObj) ? avatarObj.avatarUrl : '';

const successText = t('Avatar_changed_successfully');
const setAvatarFromService = useMethod('setAvatarFromService');
const setAvatarFromService = useEndpoint('POST', '/v1/users.setAvatarFromService');

const dispatchToastMessage = useToastMessageDispatch();

Expand Down Expand Up @@ -50,9 +52,8 @@ export const useUpdateAvatar = (
return saveAvatarUrlAction();
}
if (isServiceObject(avatarObj)) {
const { blob, contentType, service } = avatarObj;
try {
await setAvatarFromService(blob, contentType, service);
await setAvatarFromService(avatarObj);
dispatchToastMessage({ type: 'success', message: successText });
} catch (error) {
dispatchToastMessage({ type: 'error', message: error });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@ import { Match, check } from 'meteor/check';
import { DDPRateLimiter } from 'meteor/ddp-rate-limiter';

import { settings } from '../../app/settings/server';
import { setUserAvatar } from '../../app/lib';
import { setUserAvatar } from '../../app/lib/server';
import { Users } from '../../app/models/server';
import { hasPermission } from '../../app/authorization/server';
import { methodDeprecationLogger } from '../../app/lib/server/lib/deprecationWarningLogger';

Meteor.methods({
setAvatarFromService(dataURI, contentType, service, userId) {
setAvatarFromService(dataURI: string, contentType: string, service: string, userId: string) {
methodDeprecationLogger.warn('setAvatarFromService will be deprecated in future versions of Rocket.Chat');
check(dataURI, String);
check(contentType, Match.Optional(String));
check(service, Match.Optional(String));
Expand All @@ -29,7 +31,8 @@ Meteor.methods({
let user;

if (userId && userId !== Meteor.userId()) {
if (!hasPermission(Meteor.userId(), 'edit-other-user-avatar')) {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
Copy link
Member

Choose a reason for hiding this comment

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

instead of having to disable this rule, you can make your code compliant. one way to achieve that is assigning Meteor.userId() value to a variable and checking/using that variable, so something like:

const userId = Meteor.userId();
if (!userId) {
  throw new Error('generic error');
}

// now you can use userId here, no need to disable a TS lint rule
if (!hasPermission(userId, 'edit-other-user-avatar')) {
  // throw ...
}

if (!hasPermission(Meteor.userId()!, 'edit-other-user-avatar')) {
throw new Meteor.Error('error-unauthorized', 'Unauthorized', {
method: 'setAvatarFromService',
});
Expand Down
3 changes: 3 additions & 0 deletions apps/meteor/tests/data/interactions.js
Original file line number Diff line number Diff line change
@@ -1,2 +1,5 @@
import { BASE_URL } from '../e2e/config/constants'

export const targetUser = 'rocket.cat';
felipe-rod123 marked this conversation as resolved.
Show resolved Hide resolved
export const imgURL = './public/images/logo/1024x1024.png';
export const avatarURL = BASE_URL + '/images/logo/1024x1024.png';
85 changes: 85 additions & 0 deletions apps/meteor/tests/end-to-end/api/01-users.js
Original file line number Diff line number Diff line change
Expand Up @@ -751,6 +751,91 @@ describe('[Users]', function () {
});
});

describe('[/users.setAvatarFromService]', () => {
let user;
let userCredentials;
before(async () => {
user = await createUser();
userCredentials = await login(user.username, password);
});
before((done) => {
updateSetting('Accounts_AllowUserAvatarChange', true).then(() => {
updatePermission('edit-other-user-avatar', ['admin', 'user']).then(done);
});
});
Comment on lines +761 to +765
Copy link
Member

Choose a reason for hiding this comment

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

please use async/await (just like the lines above):

Suggested change
before((done) => {
updateSetting('Accounts_AllowUserAvatarChange', true).then(() => {
updatePermission('edit-other-user-avatar', ['admin', 'user']).then(done);
});
});
before(async () => {
await updateSetting('Accounts_AllowUserAvatarChange', true);
await updatePermission('edit-other-user-avatar', ['admin', 'user']);
});

after(async () => {
await updateSetting('Accounts_AllowUserAvatarChange', true);
await deleteUser(user);
user = undefined;
await updatePermission('edit-other-user-avatar', ['admin']);
});
it('should set the avatar of the logged user by a local image', (done) => {
request
.post(api('users.setAvatarFromService'))
.set(userCredentials)
.send({
username: adminUsername,
blob: 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==',
contentType: 'image/png',
service: 'gravatar',
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect((res) => {
expect(res.body).to.have.property('success', true);
})
.end(done);
});
it('should update the avatar of another user by userId when the logged user has the necessary permission (edit-other-user-avatar)', (done) => {
request
.post(api('users.setAvatarFromService'))
.set(userCredentials)
.send({
username: adminUsername,
blob: 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==',
contentType: 'image/png',
service: 'gravatar',
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect((res) => {
expect(res.body).to.have.property('success', true);
})
.end(done);
});
it('should set the avatar of another user by username and local image when the logged user has the necessary permission (edit-other-user-avatar)', (done) => {
request
.post(api('users.setAvatarFromService'))
.set(userCredentials)
.send({
username: adminUsername,
blob: 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==',
contentType: 'image/png',
service: 'gravatar',
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect((res) => {
expect(res.body).to.have.property('success', true);
})
.end(done);
});
it('should return an error if the correct params are not provided', (done) => {
updatePermission('edit-other-user-avatar', []).then(() => {
request
.post(api('users.setAvatarFromService'))
.set(userCredentials)
.send({})
.expect('Content-Type', 'application/json')
.expect(400)
.expect((res) => {
expect(res.body).to.have.property('success', false);
})
.end(done);
});
});
});

describe('[/users.resetAvatar]', () => {
let user;
before(async () => {
Expand Down
43 changes: 43 additions & 0 deletions packages/rest-typings/src/v1/users.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,46 @@ const UsersSetAvatarSchema = {

export const isUsersSetAvatarProps = ajv.compile<UsersSetAvatar>(UsersSetAvatarSchema);

type UsersSetAvatarFromService = {
userId?: IUser['_id'];
username?: IUser['username'];

blob: string;
contentType: string;
service: string;
};

const UsersSetAvatarFromServiceSchema = {
type: 'object',
properties: {
userId: {
type: 'string',
nullable: true,
},
username: {
type: 'string',
nullable: true,
},
blob: {
type: 'string',
},
contentType: {
type: 'string',
},
service: {
type: 'string',
},
url: {
type: 'string',
nullable: true,
},
},
required: ['blob', 'contentType', 'service'],
additionalProperties: false,
};

export const isUsersSetAvatarFromServiceProps = ajv.compile<UsersSetAvatarFromService>(UsersSetAvatarFromServiceSchema);

type UsersResetAvatar = { userId?: IUser['_id']; username?: IUser['username'] };

const UsersResetAvatarSchema = {
Expand Down Expand Up @@ -137,6 +177,9 @@ export type UsersEndpoints = {
'/v1/users.setAvatar': {
POST: (params: UsersSetAvatar) => void;
};
'/v1/users.setAvatarFromService': {
POST: (params: UsersSetAvatarFromService) => void;
};
'/v1/users.resetAvatar': {
POST: (params: UsersResetAvatar) => void;
};
Expand Down