Skip to content

Conversation

mstriemer
Copy link
Contributor

This updates how the LoginRequired component works so that it integrates with Helmet a bit better.

It also updates it to not use secure cookies in development since that was breaking login persisting.

This doesn't prevent API requests from happening on the server when the user it redirected to login but I will leave that bug open for now. If it needs authentication then it will fail, if it doesn't then the data is public anyway. It's just a performance issue.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.2%) to 98.802% when pulling 2b43cc6 on mstriemer:prevent-unauthenticated-api-calls-339 into 780271e on mozilla:master.

@mstriemer
Copy link
Contributor Author

#brokeeverything. I have to run but I'll fix the tests shortly.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.2%) to 98.802% when pulling 859e37b on mstriemer:prevent-unauthenticated-api-calls-339 into 780271e on mozilla:master.

@mstriemer
Copy link
Contributor Author

So updating the config seems to have no impact on the values that I see pulled out of the config when the tests run. I'm out of ideas for today so I'll try again in the morning.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.2%) to 98.806% when pulling 4c27249 on mstriemer:prevent-unauthenticated-api-calls-339 into 780271e on mozilla:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling aecb531 on mstriemer:prevent-unauthenticated-api-calls-339 into 780271e on mozilla:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4bb67a6 on mstriemer:prevent-unauthenticated-api-calls-339 into 780271e on mozilla:master.

@mstriemer
Copy link
Contributor Author

Things are passing now! Ready for review.

@muffinresearch
Copy link
Contributor

It also updates it to not use secure cookies in development since that was breaking login persisting.

I wonder if we should consider running dev under ssl since there's so many things that don't work without it?

@muffinresearch
Copy link
Contributor

lgtm r+wc

@mstriemer
Copy link
Contributor Author

SSL would be nice in dev, I guess that'd likely mean adding docker for this project. That isn't a huge deal but being able to just use node is kind of nice. We'd be able to run an addons-server a bit easier that way though.

@mstriemer mstriemer closed this in 5344d13 May 11, 2016
mstriemer added a commit that referenced this pull request May 11, 2016
@mstriemer mstriemer deleted the prevent-unauthenticated-api-calls-339 branch May 11, 2016 14:07
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