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.

Sign up for GitHub

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

[FIX] REST API OAuth services endpoint were missing fields and flag to indicate custom services #10299

Merged
merged 4 commits into from
Apr 19, 2018
Merged

[FIX] REST API OAuth services endpoint were missing fields and flag to indicate custom services #10299

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c52e458
Add field to indicate whether the oauth service is customized and cha…
MarcosSpessatto Apr 2, 2018
71b11e8
change comparision with boolean
MarcosSpessatto Apr 2, 2018
97f205e
Merge branch 'develop' into rest-api-settings-oauth-add-field
MarcosSpessatto Apr 3, 2018
45dab3c
Add missing fields when oauth service is custom
MarcosSpessatto Apr 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 0 additions & 23 deletions packages/rocketchat-api/server/v1/misc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,29 +18,6 @@ RocketChat.API.v1.addRoute('info', { authRequired: false }, {
}
});

RocketChat.API.v1.addRoute('settings.oauth', { authRequired: false }, {
get() {
const mountOAuthServices = () => {
const oAuthServicesEnabled = ServiceConfiguration.configurations.find({}).fetch();

return oAuthServicesEnabled.map((service) => {
return {
id: service._id,
name: service.service,
appId: service.appId || service.clientId,
buttonLabelText: service.buttonLabelText || '',
buttonColor: service.buttonColor || '',
buttonLabelColor: service.buttonLabelColor || ''
};
});
};

return RocketChat.API.v1.success({
services: mountOAuthServices()
});
}
});

RocketChat.API.v1.addRoute('me', { authRequired: true }, {
get() {
const me = _.pick(this.user, [
Expand Down
24 changes: 24 additions & 0 deletions packages/rocketchat-api/server/v1/settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,30 @@ RocketChat.API.v1.addRoute('settings.public', { authRequired: false }, {
}
});

RocketChat.API.v1.addRoute('settings.oauth', { authRequired: false }, {
get() {
const mountOAuthServices = () => {
const oAuthServicesEnabled = ServiceConfiguration.configurations.find({}).fetch();

return oAuthServicesEnabled.map((service) => {
return {
id: service._id,
name: service.service,
appId: service.appId || service.clientId,
buttonLabelText: service.buttonLabelText || '',
buttonColor: service.buttonColor || '',
buttonLabelColor: service.buttonLabelColor || '',
custom: Boolean(service.custom)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why you're using Boolean(service.custom) here when we don't use it anywhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

@graywolf336 to convert to Boolean if it is some value considered false by JS.

Copy link
Contributor

Choose a reason for hiding this comment

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

@MarcosSpessatto Please reconsider another way, because if the string is "false" then it will result in a value of true: https://stackoverflow.com/a/264037

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I'm going to change the validation, but I don't agree that the string 'false' should be considered Boolean, IMHO 'false' is a string and not a boolean, just as 'true' should be considered a string and not a boolean. So in both cases, casting a Boolean must be true. Why should I consider the string 'false'?

Copy link
Contributor

Choose a reason for hiding this comment

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

You misunderstood what I was saying. If the value of service.custom is a string and the value is 'false' then doing Boolean(service.custom) will result in true being the value instead of the expected false.

Copy link
Contributor

@geekgonecrazy geekgonecrazy Apr 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Just for clarity:
image
a non-empty string in javascript evaluates to true, regardless of content of string

Copy link
Member Author

Choose a reason for hiding this comment

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

The question was, why store in the database a Boolean value as a string and not as Boolean, if you expect it to be Boolean, but @graywolf336 already explained that to me. Thanks.

};
});
};

return RocketChat.API.v1.success({
services: mountOAuthServices()
});
}
});

RocketChat.API.v1.addRoute('settings', { authRequired: true }, {
get() {
const { offset, count } = this.getPaginationItems();
Expand Down
24 changes: 0 additions & 24 deletions tests/end-to-end/api/00-miscellaneous.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,29 +73,5 @@ describe('miscellaneous', function() {
.end(done);
});

describe('/settings.oauth', () => {
it('should have return list of available oauth services when user is not logged', (done) => {
request.get(api('settings.oauth'))
.expect('Content-Type', 'application/json')
.expect(200)
.expect((res) => {
expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('services').and.to.be.an('array');
})
.end(done);
});

it('should have return list of available oauth services when user is logged', (done) => {
request.get(api('settings.oauth'))
.set(credentials)
.expect('Content-Type', 'application/json')
.expect(200)
.expect((res) => {
expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('services').and.to.be.an('array');
})
.end(done);
});
});

});
25 changes: 25 additions & 0 deletions tests/end-to-end/api/08-settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,4 +65,29 @@ describe('[Settings]', function() {
.end(done);
});
});

describe('/settings.oauth', () => {
it('should have return list of available oauth services when user is not logged', (done) => {
request.get(api('settings.oauth'))
.expect('Content-Type', 'application/json')
.expect(200)
.expect((res) => {
expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('services').and.to.be.an('array');
})
.end(done);
});

it('should have return list of available oauth services when user is logged', (done) => {
request.get(api('settings.oauth'))
.set(credentials)
.expect('Content-Type', 'application/json')
.expect(200)
.expect((res) => {
expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('services').and.to.be.an('array');
})
.end(done);
});
});
});