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

Corrected security definition #101

Merged
merged 2 commits into from May 10, 2017
Merged

Corrected security definition #101

merged 2 commits into from May 10, 2017

Conversation

warnersean
Copy link

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

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

coveralls commented May 5, 2017

Coverage Status

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

@warnersean
Copy link
Author

Fixes #96

@theganyo
Copy link
Collaborator

theganyo commented May 5, 2017

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

@warnersean
Copy link
Author

@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
Copy link
Collaborator

theganyo commented May 7, 2017

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
Copy link

coveralls commented May 8, 2017

Coverage Status

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

@warnersean
Copy link
Author

Test added

@theganyo theganyo merged commit 54555fc into apigee-127:master May 10, 2017
@theganyo
Copy link
Collaborator

Thank you!

@adrai
Copy link

adrai commented Jun 8, 2017

Any ETA for a new version that includes this fix?

@theganyo
Copy link
Collaborator

theganyo commented Jun 9, 2017

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.

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.

None yet

4 participants