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

Fix wrong session validation in uploads #1438

Merged
merged 3 commits into from Feb 12, 2018
Merged

Fix wrong session validation in uploads #1438

merged 3 commits into from Feb 12, 2018

Conversation

ghost
Copy link

@ghost ghost commented Sep 4, 2017

app.session instance not exists

before pump 4.x session middleware was living in app instance > app.js

Closes: #1435

@ghost ghost mentioned this pull request Sep 4, 2017
@ghost ghost requested a review from strugee November 2, 2017 02:02
Copy link
Member

@strugee strugee left a comment

Choose a reason for hiding this comment

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

Definitely a regression, really nice catch @vxcamiloxv. 23e6a97 should have fixed this but didn't.

Left some minor nits inline but overall this looks super great. Can you edit the commit messages so the first one says "Fix session middleware usage in uploads", with "app.session is no longer the session middleware as of 23e6a97" (that way we have a nice reference as to when the regression was introduced)? The second one can be shortened to "Test viewing uploads from the web interface" too.

As always thanks so much for your patience!

@@ -187,6 +188,57 @@ suite.addBatch({
assert.ifError(err);
}
},
"and we get the file from web interface without login": {
Copy link
Member

Choose a reason for hiding this comment

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

"the web interface" instead of "web interface"; "logging in" instead of "login"

Copy link
Author

Choose a reason for hiding this comment

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

Done,

br.assert.status(403);
}
},
"and we login for try get file": {
Copy link
Member

Choose a reason for hiding this comment

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

"and we login and try to get the file"

Copy link
Author

Choose a reason for hiding this comment

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

Done.

assert.ifError(err);
br.assert.success();
},
"and we get the file from web interface with login": {
Copy link
Member

Choose a reason for hiding this comment

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

s/with login/while logged in/

Copy link
Member

Choose a reason for hiding this comment

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

And add a "the" before "web interface" too.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@strugee
Copy link
Member

strugee commented Jan 25, 2018

Also, I assume you tested the test? I.e. reverting the first commit causes the test to fail?

Camilo QS and others added 3 commits February 3, 2018 21:43
@ghost
Copy link
Author

ghost commented Feb 4, 2018

@strugee yes, if you revert the commit the test will fail

@coveralls
Copy link

coveralls commented Feb 4, 2018

Coverage Status

Coverage increased (+0.4%) to 73.62% when pulling ffe7e31 on vxcamiloxv:fix-uploads-session into 3d393b7 on pump-io:master.

Copy link
Member

@strugee strugee left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks very much @vxcamiloxv 🎉

@strugee strugee merged commit 4a63619 into pump-io:master Feb 12, 2018
@ghost ghost deleted the fix-uploads-session branch February 12, 2018 21:20
@ghost
Copy link
Author

ghost commented Feb 12, 2018

;)

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

Successfully merging this pull request may close these issues.

None yet

2 participants