Skip to content

[BREAK] Always remove the field services from user data responses in REST API#10799

Merged
rodrigok merged 5 commits into
developfrom
fix/rest-user-fields-returned
Jun 20, 2018
Merged

[BREAK] Always remove the field services from user data responses in REST API#10799
rodrigok merged 5 commits into
developfrom
fix/rest-user-fields-returned

Conversation

@MarcosSpessatto

Copy link
Copy Markdown
Contributor

No description provided.

@MarcosSpessatto MarcosSpessatto self-assigned this May 17, 2018
@MarcosSpessatto MarcosSpessatto requested a review from rodrigok May 17, 2018 16:53
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-10799 May 17, 2018 16:54 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-10799 May 17, 2018 18:16 Inactive

@graywolf336 graywolf336 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please change this to breaking, as it heavily breaks the response we've been returning for a long time now.

@@ -1,4 +1,7 @@
RocketChat.API.helperMethods.set('parseJsonQuery', function _parseJsonQuery() {
const VIEW_FULL_USER_FIELDS_TO_EXCLUDE = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Don't define this every time the call happens, move it outside.

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-10799 May 17, 2018 18:51 Inactive
@MarcosSpessatto MarcosSpessatto changed the title [FIX] Remove sensitive user data from response, even if the user is admin [BREAK] Remove sensitive user data from response, even if the user is admin May 17, 2018
@graywolf336 graywolf336 dismissed their stale review May 18, 2018 04:29

Changes made.

@marceloschmidt

Copy link
Copy Markdown
Member

@rodrigok @graywolf336 @MarcosSpessatto what's the status on this? we need to get it merged... has the mobile team been informed of the breaking change?

@geekgonecrazy

Copy link
Copy Markdown
Contributor

@RocketChat/android @RocketChat/ios just in case not on radar.

@@ -52,8 +60,12 @@ RocketChat.API.helperMethods.set('parseJsonQuery', function _parseJsonQuery() {
// Verify the user has permission to query the fields they are
if (typeof query === 'object') {
let nonQuerableFields = Object.keys(RocketChat.API.v1.defaultFieldsToExclude);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"nonQueryable"

@cardoso

cardoso commented May 29, 2018

Copy link
Copy Markdown
Member

@geekgonecrazy @marceloschmidt @MarcosSpessatto @graywolf336 this doesn't break anything for us

@MarcosSpessatto MarcosSpessatto added this to the 0.66.0 milestone Jun 5, 2018
ggazzo
ggazzo previously requested changes Jun 12, 2018

@ggazzo ggazzo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please see the comments

if (!RocketChat.authz.hasPermission(this.userId, 'view-full-other-user-info') && this.request.route.includes('/v1/users.')) {
nonSelectableFields = nonSelectableFields.concat(Object.keys(RocketChat.API.v1.limitedUserFieldsToExclude));
if (this.request.route.includes('/v1/users.')) {
if (RocketChat.authz.hasPermission(this.userId, 'view-full-other-user-info')) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what do you think

	...
	const getFields = () => Object.keys(RocketChat.authz.hasPermission(this.userId, 'view-full-other-user-info') ? RocketChat.API.v1.limitedUserFieldsToExcludeIfIsPrivilegedUser : RocketChat.API.v1.limitedUserFieldsToExclude);
	nonSelectableFields = nonSelectableFields.concat(getFields());
	...

Unknown added 2 commits June 13, 2018 15:25
Refactor the if else statement on parseJsonQuery function
@marceloschmidt

Copy link
Copy Markdown
Member

Can we merge this yet?

@MarcosSpessatto

Copy link
Copy Markdown
Contributor Author

@ggazzo what do you think?

@rodrigok rodrigok changed the title [BREAK] Remove sensitive user data from response, even if the user is admin [BREAK] Always remove the field services from user data responses Jun 20, 2018
@rodrigok rodrigok changed the title [BREAK] Always remove the field services from user data responses [BREAK] Always remove the field services from user data responses in REAT API Jun 20, 2018
@rodrigok rodrigok changed the title [BREAK] Always remove the field services from user data responses in REAT API [BREAK] Always remove the field services from user data responses in REST API Jun 20, 2018
@rodrigok rodrigok merged commit c53fa4d into develop Jun 20, 2018
@rodrigok rodrigok deleted the fix/rest-user-fields-returned branch June 20, 2018 21:55
@rodrigok rodrigok mentioned this pull request Jun 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants