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

Service subscriptions #887

Merged
merged 9 commits into from
May 18, 2021
Merged

Service subscriptions #887

merged 9 commits into from
May 18, 2021

Conversation

michael-topchiev
Copy link
Contributor

@@ -126,7 +126,7 @@ const createApolloServer = () => {
introspection: true, // set to true as long as user has valid token
plugins: customPlugins,
tracing: process.env.GRAPHQL_ENABLE_TRACING === 'true',
playground: process.env.GRAPHQL_ENABLE_PLAYGROUND === 'false',
playground: process.env.GRAPHQL_ENABLE_PLAYGROUND === 'true',
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this updated to 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.

I think this was a bug - the playground needs to be enabled when we export GRAPHQL_ENABLE_PLAYGROUND=true

@dalehille
Copy link
Contributor

@michael-topchiev let or const should be used in place of var

@@ -143,7 +143,7 @@ const applyQueryFieldsToResources = async(resources, queryFields={}, args, conte

if(queryFields.cluster){
var clusterIds = _.map(resources, 'clusterId');
var clusters = await models.Cluster.find({ org_id: orgId, cluster_id: { $in: clusterIds } }).lean({ virtuals: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

why was the org_id part of the query removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good question. I was thinking why in many places we retrieve data from MongoDb collections by a combination of a unique key AND orgId when obviously only unique id would be enough. I think the answer is: for security. When clients submit requests we need to make sure they see and change only objects from their own orgId. At the start of request processing we make calls to auth plug-in to verify the user can do the action on the object for the org. That's clearly not enough and we also ensure every parameter passed to the API references objects from the same OrgId. For some reason this practice is expanded to more places than required for enforcing security and this appears to be one such instance. The problem here is that here orgId parameter prevents queries on service subscriptions from retrieving cluster and resource data because those objects belong to different OrgId (in addition to harm performance). That's why it's removed and this should not impact security since access to all stored objects has already been checked long ago on entry in mutators.

@michael-topchiev
Copy link
Contributor Author

@michael-topchiev let or const should be used in place of var

Replacing vars in serviceSubscriptions.js

@michael-topchiev
Copy link
Contributor Author

Storage team completed their testing. Dale & Ryan, please review and approve if all is good.

dalehille
dalehille previously approved these changes May 17, 2021
rmgraham
rmgraham previously approved these changes May 18, 2021
@michael-topchiev michael-topchiev merged commit 6b9bc90 into master May 18, 2021
@michael-topchiev michael-topchiev deleted the Service-Subscriptions-PR branch May 18, 2021 20:11
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.

3 participants