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

Feature enable otp #201

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

experian-greg-myers
Copy link

@experian-greg-myers experian-greg-myers commented Mar 29, 2021

Duplicate of: benjaminwols#2
#108
But merged into master.

Edit: Also merged #200 to fix tests.

Benjamin Wols and others added 30 commits November 9, 2016 11:56
@@ -19,7 +19,7 @@ gem "rails", rails
if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.2.0')
gem "test-unit", "~> 3.0"
end

gem 'sprockets-rails', '~> 2.0'

Choose a reason for hiding this comment

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

Sprockets 3 requires a manifest, pegged sprockets to 2 for duration of this branch so the manifest can be fixed separately.

@@ -6,7 +6,7 @@

<p><%= flash[:notice] %></p>

<%= form_tag([resource_name, :two_factor_authentication], :method => :put) do %>
<%= form_tag(verify_user_two_factor_authentication_path, method: :put) do %>

Choose a reason for hiding this comment

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

Method name is not generic in this PR, seems to be locked to user for now.

require "active_model"
require "active_support/core_ext/class/attribute_accessors"
require "cgi"
require 'active_model'

Choose a reason for hiding this comment

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

PR #108 added an Activerecord dependency here that I removed in the merge. Unknown if it's needed but it doesn't seem to be and tests pass.

@@ -66,11 +66,11 @@ def send_new_otp(options = {})
end

def send_new_otp_after_login?
!totp_enabled?
otp_enabled && !totp_enabled?

Choose a reason for hiding this comment

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

This was also a merge conflict resolution.

post :update, params: { code: code }
else
post :update, code: code
def post_params(action, params)

Choose a reason for hiding this comment

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

Replaces post_code method as there are some other params needing submission.

@@ -138,7 +138,7 @@ def instance.send_two_factor_authentication_code(code)

it "returns uri with user's email" do
expect(instance.provisioning_uri).
to match(%r{otpauth://totp/houdini@example.com\?secret=\w{32}})
to match(%r{otpauth://totp/houdini%40example.com\?secret=\w{32}})
Copy link
Author

@experian-greg-myers experian-greg-myers Mar 29, 2021

Choose a reason for hiding this comment

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

New ROTP versions output URL encoded URLs hence the test change.
This will be incompatible with old ROTP versions if someone pegged the version down.

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.

2 participants