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

Respect --force option for config/master.key #31957

Merged
merged 1 commit into from
Feb 11, 2018

Conversation

claudiob
Copy link
Member

This is similar to #30700 which ensures the --quiet option of rails new is respected by the MasterKeyGenerator (missing from #30067).

Before this commit, running rails new app --force would still prompt the user what to do with the conflict in config/master.key:

              …
   identical  config/locales/en.yml
    conflict  config/master.key
Overwrite /Users/claudiob/Desktop/pizza/config/master.key? (enter "h" for help) [Ynaqdh]

After this commit, config/master.key is overwritten:

           …
identical  config/locales/en.yml
    force  config/master.key
   append  .gitignore

The newly added test generates an app and then generates it again with --force. Without this commit, the test would just wait forever for user input.

This is similar to rails#30700 which ensures the `--quiet` option of `rails new`
is respected by the `MasterKeyGenerator` (missing from rails#30067).

Before this commit, running `rails new app --force` would still prompt the
user what to do with the conflict in `config/master.key`:

```
              …
   identical  config/locales/en.yml
    conflict  config/master.key
Overwrite /Users/claudiob/Desktop/pizza/config/master.key? (enter "h" for help) [Ynaqdh]
```

After this commit, `config/master.key` is overwritten:

```
           …
identical  config/locales/en.yml
    force  config/master.key
   append  .gitignore
```

The newly added test generates an app and then generates it again with
`--force`. Without this commit, the test would just wait forever for user
input.
@claudiob
Copy link
Member Author

My suggestion would be to backport this to 5.2 as well.

@y-yagi y-yagi merged commit 360d9bd into rails:master Feb 11, 2018
y-yagi added a commit that referenced this pull request Feb 11, 2018
Respect --force option for config/master.key
@y-yagi
Copy link
Member

y-yagi commented Feb 11, 2018

Backported 74976e8

@claudiob claudiob deleted the force-master-key branch February 12, 2018 00:37
@claudiob
Copy link
Member Author

Mmmh @y-yagi now that I think about it, I see another issue related to this code.

On one hand, it's true that --force should be respected.
On the other hand, if we replace the master key (which is not under git control), we won't be able to decrypt config/credentials.yml.enc anymore.

I can see two solutions here:

  1. Using --force does not replace config/master.key if it's already present. Does not even ask for it, simply leaves it there

or

  1. Using --force also regenerates config/credentials.yml.enc. Currently this does not happen because this code introduced in f7e3c68 does not respect the --force option:
def add_credentials_file_silently(template = nil)
  unless credentials.content_path.exist?
    credentials.write(credentials_template)
  end
end

What's your opinion?

@y-yagi
Copy link
Member

y-yagi commented Feb 12, 2018

Thanks. I think the first solution is good.
It seems to be sufficient if add a file existence check to add_key_file_silently like add_key_file, what about it?
https://github.com/rails/rails/blob/cfcb92f9eaf78daefe21335bcabf813842c0ab07/railties/lib/rails/generators/rails/encryption_key_file/encryption_key_file_generator.rb#L28..L30

@claudiob
Copy link
Member Author

@y-yagi That sounds good. Go for it! 🏃

claudiob added a commit to claudiob/rails that referenced this pull request Feb 13, 2018
See rails#31957 (comment)

The purpose of `--force` is not to have any prompt whether a file should
be kept or overwritten. In general, all existing files should be overwritten.
However, `config/master.key` is special because it is git-ignored, and
overwriting it will cause the app not to run (since there won't be a way
to decrypt the credentials).

As a result, it's probably better to keep the existing config/master.key.
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.

None yet

2 participants