Refactor Password Expireable #45

olbrich · 2018-06-23T17:33:26Z

Mostly this is a cleanup of the password expireable code that was started in part because of #44. I haven't yet been able to replicate the original issue, but it seemed like a good excuse to clean up some older code.

dillonwelch · 2018-06-23T17:38:51Z

lib/devise-security/models/password_expirable.rb

+        return @enabled if defined?(@enabled)
+        @enabled = expire_password_after.is_a?(1.class) ||
+                   expire_password_after.is_a?(Float) ||
+                   expire_password_after == true


Seems like you can use expire_on_demand? here

dillonwelch · 2018-06-23T17:42:05Z

lib/devise-security/models/password_expirable.rb

-
-        self.password_changed_at
+        return unless enabled?
+        self.password_changed_at = nil


Seems like this is a logic change from the original, not just cleaning up CC. Is that intentional?

This is a slightly different implementation. The original would make up a fake time that would cause the record to appear as if it needed to be changed. I didn't particularly care for that because it makes it look like the user changed their password at a time when they really didn't and it makes it impossible to distinguish between an administrative expiration of the password and a real one. Just setting the password_changed_at to nil has the same effect and it's possible to tell that the password was deliberately expired because that is the only way to get a nil in that column.

Sounds good, we'll just want to make sure do a release that indicates there's a change here.

dillonwelch · 2018-06-23T17:43:20Z

lib/devise-security/models/password_expirable.rb

+          else
+            # Expire when password_changed_at is nil or when the last time the
+            # password was changed is sufficiently old
+            password_changed_at.nil? || password_changed_at < expire_password_after.seconds.ago


Possibly extract this line into a private method to address the CC issue? It's borderline to me.

coveralls · 2018-06-23T17:44:37Z

Pull Request Test Coverage Report for Build 149

101 of 101 (100.0%) changed or added relevant lines in 2 files are covered.
5 unchanged lines in 3 files lost coverage.
Overall coverage increased (+3.4%) to 92.363%

Files with Coverage Reduction	New Missed Lines	%
test/test_password_expired_controller.rb	1	100.0%
lib/devise-security/controllers/helpers.rb	1	50.94%
test/test_security_question_controller.rb	3	100.0%

Totals
Change from base Build 141:	3.4%
Covered Lines:	895
Relevant Lines:	969

💛 - Coveralls

bjensen · 2018-07-21T16:53:51Z

+1 on someone to review

dillonwelch · 2018-07-23T02:31:21Z

.rubocop.yml

@@ -1,5 +1,5 @@
 AllCops:
-  TargetRubyVersion: 2.3
+  TargetRubyVersion: 2.2


Why the switch here?

we claim to support 2.2, but rubocop was looking for 2.3 syntax.

https://www.ruby-lang.org/en/news/2018/06/20/support-of-ruby-2-2-has-ended/

dillonwelch · 2018-07-23T02:32:21Z

README.md

 ```erb
 <p><%= captcha_tag %></p>
 <p><%= text_field_tag :captcha %></p>
 ```

 ## Schema

-Note: Unlike Devise, devise-security does not currently support mongoid.  Pull requests are welcome!
+Note: Unlike Devise, devise-security does not currently support mongoid. Pull requests are welcome!


I'd like to remove the Pull requests are welcome! given that we've discussed on an issue recently about that being pretty complicated.

This is just a reformatting, if we want to remove that line, I suggest a new PR.

dillonwelch · 2018-07-23T02:35:43Z

lib/devise-security/models/password_expirable.rb

+    alias expire_password need_change_password
+    alias request_password_change need_change_password
+
+    # Set this value to a number of seconds to have passwords expire after a time


This comment is a little confusing because it's not a setter.

dillonwelch · 2018-07-23T02:36:00Z

lib/devise-security/models/password_expirable.rb

+    # without a time limit.
+    # @return [Numeric, Boolean]
+    def expire_password_after
+      self.class.expire_password_after


Is this something you can use delegate for?

dillonwelch · 2018-07-23T02:37:42Z

lib/devise-security/models/password_expirable.rb

-        end
+    # Enabled if configuration +expire_password_after+ is set to an {Integer}, {Float}, or {true}
+    def password_expiration_enabled?
+      expire_password_after.present? &&


`` expire_password_after.present?is already part ofexpire_password_on_demand?`

dillonwelch · 2018-07-23T02:38:36Z

test/test_helper.rb

-Coveralls.wear!
+SimpleCov.start do
+  add_filter 'gemfiles'
+  add_filter 'test'


If possible I like to only restrict certain files vs all of test, because simplecov will catch dead test code.

…y#47) * update default ruby version * add codeclimate config

olbrich · 2018-10-09T01:21:11Z

@devise-security/maintainers how about a new code review?

natebird

Lots of formatting changes. Thanks for doing this cleanup to make everything consistent.

dillonwelch · 2018-10-12T21:32:15Z

.codeclimate.yml

+- "**/tests/"
+- "**/vendor/"
+- "**/*.d.ts"
+- "gemfiles/"


dillonwelch · 2018-10-12T21:32:21Z

.gitignore

@@ -38,4 +38,5 @@ test/tmp/*
 *.gem
 Gemfile.lock
 *.lock
-bin/
+bin/*
+.yardoc


olbrich added the ✨ enhancement label Jun 23, 2018

olbrich self-assigned this Jun 23, 2018

dillonwelch reviewed Jun 23, 2018

View reviewed changes

olbrich requested review from natebird and dillonwelch July 21, 2018 18:38

olbrich added this to the Next Release milestone Jul 21, 2018

dillonwelch reviewed Jul 23, 2018

View reviewed changes

viniciusoyama and others added 16 commits September 4, 2018 11:56

Improved documentation for devise-security initializer configuration

9745fa2

Update default ruby version & add codeclimate configs (devise-securit…

1feee9d

…y#47) * update default ruby version * add codeclimate config

Rubocop autocorrect

6ebcc25

refactor password_expirable

6af5591

fix class methods reference, not supported in rails 4.1

021fea8

update documentation

f0455d5

update documentation

fafd959

add scopes

ce30163

refactor DSL for clarity and simplicity

e7b32bd

require 'pry' in tests for convenience

2211dc4

cleanup, add coverage configuration

0ffc263

Beautify markdown

8846e53

configure codeclimate and markdownlint

ca40d2c

rubocop cleanup

dad6fb9

Tweaks in response to code review

0bc1626

formatting

b5bf3e6

olbrich force-pushed the issue-44-deprecation-warning branch from 36b6e91 to b5bf3e6 Compare September 4, 2018 16:00

Merge branch 'master' into issue-44-deprecation-warning

3050791

olbrich requested a review from a team October 9, 2018 01:14

natebird approved these changes Oct 9, 2018

View reviewed changes

dillonwelch approved these changes Oct 12, 2018

View reviewed changes

EOL issues

7055de6

olbrich merged commit 96bfdd8 into devise-security:master Oct 13, 2018

olbrich mentioned this pull request Oct 23, 2018

Deprecation warnings about the behavior of attribute_changed? #59

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Password Expireable #45

Refactor Password Expireable #45

olbrich commented Jun 23, 2018

dillonwelch Jun 23, 2018

dillonwelch Jun 23, 2018

olbrich Jun 23, 2018

dillonwelch Jun 23, 2018

dillonwelch Jun 23, 2018

coveralls commented Jun 23, 2018 •

edited

Loading

bjensen commented Jul 21, 2018

dillonwelch Jul 23, 2018

olbrich Jul 23, 2018

dillonwelch Oct 13, 2018

dillonwelch Jul 23, 2018

olbrich Sep 3, 2018

dillonwelch Jul 23, 2018

dillonwelch Jul 23, 2018

dillonwelch Jul 23, 2018

dillonwelch Jul 23, 2018

olbrich commented Oct 9, 2018

natebird left a comment

dillonwelch Oct 12, 2018

dillonwelch Oct 12, 2018

Refactor Password Expireable #45

Refactor Password Expireable #45

Conversation

olbrich commented Jun 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 23, 2018 • edited Loading

Pull Request Test Coverage Report for Build 149

💛 - Coveralls

bjensen commented Jul 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olbrich commented Oct 9, 2018

natebird left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 23, 2018 •

edited

Loading