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.

Sign up for GitHub

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

Jump to bottom

Change has_secure_token default to on: :initialize #48912

Merged
merged 1 commit into from
Sep 1, 2023
Merged

Change has_secure_token default to on: :initialize #48912

merged 1 commit into from
Sep 1, 2023

Conversation

seanpdoyle
Copy link
Contributor

Follow-up to #47420

Motivation / Background

With the changes made in #47420, has_secure_token declarations can be configured to execute in an after_initialize callback. This commit proposed a new Rails 7.1 default: generate all has_secure_token values when their corresponding models are initialized.

Detail

To preserve pre-7.1 behavior, applications can set config.active_record.generate_secure_token_on = :create.

By default, generate the value when the model is initialized:

class User < ApplicationRecord
  has_secure_token
end

record = User.new
record.token # => "fwZcXX6SkJBJRogzMdciS7wf"

With config.active_record.generate_secure_token_on = :create, generate the
value when the model is created:

# config/application.rb
config.active_record.generate_secure_token_on = :create

# app/models/user.rb
class User < ApplicationRecord
  has_secure_token on: :create
end

record = User.new
record.token # => nil
record.save!
record.token # => "fwZcXX6SkJBJRogzMdciS7wf"

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@seanpdoyle seanpdoyle mentioned this pull request Aug 9, 2023
Merged
4 tasks
@seanpdoyle seanpdoyle force-pushed the has-secure-token-default-on-initialize branch from 623f701 to 28d810b Compare August 9, 2023 14:28
skipkayhil
skipkayhil reviewed Aug 9, 2023
View reviewed changes
activerecord/lib/active_record.rb Outdated Show resolved Hide resolved
activerecord/lib/active_record/railtie.rb Outdated Show resolved Hide resolved
guides/source/configuring.md Show resolved Hide resolved
...ib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt Show resolved Hide resolved
guides/source/configuring.md Outdated Show resolved Hide resolved
@seanpdoyle seanpdoyle force-pushed the has-secure-token-default-on-initialize branch 2 times, most recently from 2132388 to 37572c3 Compare August 9, 2023 15:02
@seanpdoyle seanpdoyle force-pushed the has-secure-token-default-on-initialize branch from 37572c3 to 12c6e20 Compare August 9, 2023 15:18
@seanpdoyle seanpdoyle force-pushed the has-secure-token-default-on-initialize branch 2 times, most recently from efd3d9f to 9dc9990 Compare August 9, 2023 19:17
@seanpdoyle seanpdoyle force-pushed the has-secure-token-default-on-initialize branch 4 times, most recently from 136c459 to 5b0468c Compare August 11, 2023 15:28
@seanpdoyle seanpdoyle force-pushed the has-secure-token-default-on-initialize branch from 5b0468c to 2497f28 Compare August 22, 2023 12:56
@seanpdoyle
Copy link
Contributor Author

@rafaelfranca @skipkayhil are there any other issues with this diff that need addressing?

@seanpdoyle
Copy link
Contributor Author

@rafaelfranca since it involves a breaking change with new defaults, does this feel worthwhile to include in the 7.1 release?

@seanpdoyle seanpdoyle force-pushed the has-secure-token-default-on-initialize branch 3 times, most recently from 1201abc to e2851f1 Compare August 23, 2023 17:41
@seanpdoyle seanpdoyle force-pushed the has-secure-token-default-on-initialize branch from e2851f1 to d53f1f7 Compare August 23, 2023 19:21
skipkayhil
skipkayhil approved these changes Aug 23, 2023
View reviewed changes
Copy link
Member

@skipkayhil skipkayhil left a comment

Choose a reason for hiding this comment

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

It looks good to me, just need a committer to review 👍

@seanpdoyle seanpdoyle force-pushed the has-secure-token-default-on-initialize branch 3 times, most recently from aa90125 to e5f7dbf Compare August 29, 2023 17:18
@seanpdoyle
Copy link
Contributor Author

@rafaelfranca if you're able, could you re-review these changes?

@seanpdoyle seanpdoyle force-pushed the has-secure-token-default-on-initialize branch 3 times, most recently from 9f51aa1 to b5c310b Compare September 1, 2023 15:56
@seanpdoyle @skipkayhil @rafaelfranca
Change has_secure_token default to on: :initialize
e85a3ec 
Follow-up to [rails#47420][]

With the changes made in [rails#47420][], `has_secure_token` declarations can
be configured to execute in an `after_initialize` callback. This commit
proposed a new Rails 7.1 default: generate all `has_secure_token` values
when their corresponding models are initialized.

To preserve pre-7.1 behavior, applications can set
`config.active_record.generate_secure_token_on = :create`.

By default, generate the value when the model is initialized:

```ruby
class User < ApplicationRecord
  has_secure_token
end

record = User.new
record.token # => "fwZcXX6SkJBJRogzMdciS7wf"
```

With `config.active_record.generate_secure_token_on = :create`, generate
the value when the model is created:

```ruby
 # config/application.rb
config.active_record.generate_secure_token_on = :create

 # app/models/user.rb
class User < ApplicationRecord
  has_secure_token on: :create
end

record = User.new
record.token # => nil
record.save!
record.token # => "fwZcXX6SkJBJRogzMdciS7wf"
```

[rails#47420]: rails#47420

Co-authored-by: Hartley McGuire <skipkayhil@gmail.com>
@rafaelfranca rafaelfranca force-pushed the has-secure-token-default-on-initialize branch from b5c310b to e85a3ec Compare September 1, 2023 20:17
@rafaelfranca rafaelfranca merged commit a21c40b into rails:main Sep 1, 2023
4 checks passed
@seanpdoyle seanpdoyle deleted the has-secure-token-default-on-initialize branch September 1, 2023 20:32
@morgoth morgoth mentioned this pull request Sep 4, 2023
Closed
kamipo added a commit that referenced this pull request Sep 5, 2023
@kamipo
Fix a test added in #48912 to work on a model with on: :initialize
2a73ef8 
Since default behavior is still `on: :create`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skipkayhil skipkayhil skipkayhil approved these changes

@rafaelfranca rafaelfranca Awaiting requested review from rafaelfranca

Assignees
No one assigned
Labels
activerecord docs railties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@seanpdoyle @rafaelfranca @skipkayhil