Skip to content
This repository has been archived by the owner on Aug 4, 2023. It is now read-only.

fixes security test order #456

Merged
merged 1 commit into from
Dec 8, 2016
Merged

fixes security test order #456

merged 1 commit into from
Dec 8, 2016

Conversation

smoebody
Copy link
Contributor

@smoebody smoebody commented Dec 4, 2016

this pull request fixes the security testing order. Imagine a security-setup like this

  security:
    -
      client_credentials: []
      user_credentials: []
    -
      client_credentials: []

I want to process the security definitions in order so that satisfying client_credentials AND user_credentials always wins.
Appearently this does not resolve reliable until now since the security middleware invokes the security handlers with async.map() so that both OR-sets are tried to resolve in parallel, so that sometimes wins the first, sometimes the second.
In my usecase i save client- and user-credentials to the request-object so that i do not have to do it again in the controller. with this pull request i changed the implementation to work sequentially for the OR-set by using async.mapSeries instead of async.map. this ensures that testing the first set has to finish in order to test the second one. also the second set is never invoked if the first satisfys successfully.

i hope i could make myself clear. feel free to include the docker-stuff. i find it convenient, but not necessary.

@smoebody
Copy link
Contributor Author

smoebody commented Dec 4, 2016

i just saw that this migh be related to #419

Copy link
Member

@whitlockjc whitlockjc left a comment

Choose a reason for hiding this comment

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

The Docker support should also be in a separate commit, just like the changes to the npm scripts. Do you mind taking those out and submitting them in a subsequent pull request?

@@ -4,7 +4,8 @@
"description": "Various tools for using and integrating with Swagger.",
"main": "index.js",
"scripts": {
"test": "gulp"
"test": "gulp",
"test-debug": "node --debug-brk node_modules/.bin/gulp"
Copy link
Member

Choose a reason for hiding this comment

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

This could be a separate commit, also "test": "gulp" could be updated to use node node_modules/.bin/gulp if/when you do it.

Copy link
Contributor Author

@smoebody smoebody Dec 6, 2016

Choose a reason for hiding this comment

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

I would rather not change the default test-command to explicit call the gulp script. npm should take care of script path resolution.
the reason i do manually for test-debug is because gulp has no option to enable node's debug mode. this is more of a workaround than a reliable solution i think.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, I'm cool with keeping this part as-is.

@ghost ghost force-pushed the master branch 2 times, most recently from 448357d to 3f26ae5 Compare December 7, 2016 14:12
@smoebody
Copy link
Contributor Author

smoebody commented Dec 7, 2016

ok, this pull request only contains the security order stuff. i made separate pull requests for docker-compose and test-debug, as you wished.

thanks for all the work

@whitlockjc
Copy link
Member

Do you mind removing the browser/* changes from the PR? You only changed middleware and those changes should not touch the browser builds. (I assume this is because your npm install installed different versions of the dependencies last used to build the browser binaries.) It likely wouldn't break anything but it's safer to avoid bringing in unnecessary changes.

Thanks for your contribution, and your patience with my review. As soon as you get this taken care of, I'll merge all 3 PRs from you.

* added tests for delayed security functions
* modified OR sequence to async.mapSeries
* modified tests securityDef to resolve delayed
@smoebody
Copy link
Contributor Author

smoebody commented Dec 8, 2016

ok, no problem.

@whitlockjc whitlockjc merged commit f543392 into apigee-127:master Dec 8, 2016
@whitlockjc
Copy link
Member

Thanks for your help.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants