apigee-127 / swagger-node-runner

The heart of Swagger-Node
MIT License
102 stars 124 forks source link

Corrected security definition #101

Closed warnersean closed 7 years ago

warnersean commented 7 years ago

Previous implementation would only check the security definition on the route. This change also checks the global api security definitions.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 96.19% when pulling a2cc50f40257b4fdb0c740397b2ec961f07f6f06 on warnersean:master into 0800ae09e8cc30265aaea9f8a0f04c63a132e0bd on theganyo:master.

warnersean commented 7 years ago

Fixes #96

theganyo commented 7 years ago

Thanks for finding this! I really appreciate it. ...Is there any chance I could convince you to write a test for it? :)

warnersean commented 7 years ago

@theganyo I looked in to testing for this and am a little concerned about where to make the correction.

Currently testing for similar concerns is handled in test/lib/common.js If this test is added to this location it will require that all routes tested have global security keys, except for those that are intended to fail.

My current approach was to put it in test/index.js, where it doesn't really match the concern but where creating/using new swagger specs make more sense.

Currently the test is as follows: ` it('should allow paths using global security', function(done) { var config = _.clone(DEFAULT_PROJECT_CONFIG); config.startWithWarnings = true; config.swagger = SWAGGER_WITH_GLOBAL_SECURITY; SwaggerRunner.create(config, function(err, runner) {

        var app = require('connect')();
        runner.connectMiddleware().register(app);

        var request = require('supertest');

        request(app)
            .get('/hello_secured?name=Scott')
            .set('Accept', 'application/json')
            .expect(200)
            .expect('Content-Type', /json/)
            .end(function(err, res) {
                should.not.exist(err);
                // should.exist(err)
                res.body.should.eql('Hello, Scott!');

                done();
            });
    });
});`

With a valid, but truncated swagger json in the same file. Do you have any preferences regarding this before I update the PR?

theganyo commented 7 years ago

I agree... Don't change the existing tests. You may create a new file for this test or integrate it into an existing file as you feel appropriate.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 96.19% when pulling 455d613b49971cb1b984393f753284947e7f1210 on warnersean:master into 0800ae09e8cc30265aaea9f8a0f04c63a132e0bd on theganyo:master.

warnersean commented 7 years ago

Test added

theganyo commented 7 years ago

Thank you!

adrai commented 7 years ago

Any ETA for a new version that includes this fix?

theganyo commented 7 years ago

Sorry, but it will have to be a couple weeks as I'm going on vacation tomorrow and I don't want to release just before vacation.