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

Require 2FA for admin to access constrained route #486

Merged
merged 1 commit into from Sep 22, 2016

Conversation

monfresh
Copy link
Contributor

Why: To better protect access to dashboards such as Sidekiq and
Split.

@pkarman
Copy link
Contributor

pkarman commented Sep 20, 2016

code lgtm. The travis errors are about Redis, which I assume is because the route is succeeding and the sidekiq initialization is being invoked. Maybe need to mock that better?

@monfresh
Copy link
Contributor Author

Weird. It passes locally when Redis is not running. I'll try in Docker.

@monfresh
Copy link
Contributor Author

@amoose Feel free to take over. I won't have time to address this before I leave. Thanks!

**Why**: To better protect access to dashboards such as Sidekiq and
Split.
@pkarman pkarman force-pushed the require-2fa-for-sidekiq-access branch from e52e3b0 to 20eb85a Compare September 21, 2016 16:11
@pkarman
Copy link
Contributor

pkarman commented Sep 21, 2016

I enabled the redis service at travis ci and that got the tests passing again. I'm not sure why it is suddenly needed, as like @monfresh it works for me locally in test with redis not running.

@pkarman
Copy link
Contributor

pkarman commented Sep 21, 2016

I'm +1 to merge unless there is some specific reason we don't want redis running at travis ci.

@amoose
Copy link
Contributor

amoose commented Sep 21, 2016

We will need to enable it for #474 anyhow, so I think it's OK. I want to try one more test then will give green light.

Copy link
Contributor

@pkarman pkarman left a comment

Choose a reason for hiding this comment

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

lgtm

@amoose amoose merged commit 551d871 into master Sep 22, 2016
@amoose amoose deleted the require-2fa-for-sidekiq-access branch September 22, 2016 17:36
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

3 participants