-
Notifications
You must be signed in to change notification settings - Fork 62
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
Use Rails session store options in table tasks and migrations #157
Conversation
@thomaswitt Did these changes work for you? |
@mullermp I can confirm that ENV works now:
So you see the ENV takes precedence and works. Without the ENV, still a sessions table is created, so the arguments to Is ENV now the way to go? |
The configuration should still work. Your keys should be table_name and table_key. |
Additional keys like key are used in rack here https://www.rubydoc.info/gems/rack/2.0.1/Rack/Session/Abstract/Persisted |
Also with table_name and table_key, that doesnt work. Only the table name from ENV is taken into consideration. Any data supplied to the Rails.application.config.session_store :dynamo_db_store is ignored, regardless using table_name or name as keys.
|
Additionally, when I don't set the ENV var, I get another error:
Also doesn't work when removing the underscores. |
Ok, that is weird. I explicitly tested this and I had it working. I used a brand new rails app (sample-app). Maybe there's something missing from the rails configuration side. |
How are you using repl? Are you using rails console? Does this still occur when you run the dynamo_db:session_store:create task? |
One thing you can also try is setting this configuration in an initializer. I think what may happen is rails is already initializing and changing of those keys don't matter after that is done. The rake task and console will load the rails environment. |
I am using bundle exec rails console … I can try the initalizer and report back. |
@thomaswitt Did you try with the initializer? I was able to make it work so I'm just confirming with you. The Rails config is loaded and used when using the rake task (no SQL) or db migrate (active record installed). Otherwise, if you are just calling |
@mullermp I can confirm that the initialization via session_store doesn't have ANY effect on Aws::SessionStore::DynamoDB::Table.create_table. I solved it by creating an initializer like: # config/initializers/session_store.rb
DYNAMO_SESSION_TABLE = {
table_name: "app_sessions_#{Rails.env}",
table_key: 'hk',
max_age: 7 * 3600 * 24,
max_stale: 3600 * 5,
}
Rails.application.config.session_store :dynamodb_store,
**DYNAMO_SESSION_TABLE.merge(
secret_key: Rails.application.credentials.dig(:secret_key_base),
) Then I can call Aws::SessionStore::DynamoDB::Table.create_table(DYNAMO_SESSION_TABLE), which works correctly. In order to have a simple rask task to do the job I wrote: # lib/tasks/dynamo_db.rake
namespace :dynamo_db do
namespace :sessions do
desc 'Create DynamoDB Sessions Table'
task create_db: :environment do
Aws::SessionStore::DynamoDB::Table.create_table(DYNAMO_SESSION_TABLE)
end
desc 'Delete DynamoDB Sessions Table'
task delete_db: :environment do
Aws::SessionStore::DynamoDB::Table.delete_table(DYNAMO_SESSION_TABLE)
end
end
end At least it works, but I would definitely not call it foolproof. I highly recommend to document this. |
@mullermp Just as another heads up, as I tried this with some real life code I got an exception
I don't know if that's a general problem or a problem in my code, would need to investigate. Definitely doesn't work right out of the box by just switching the auth provider |
Yes, that is expected. As mentioned, |
I have seen the commit csrf token issue during development. I think the issue is with Rails 7.2 and the rack middleware that's currently published in aws-sdk-rails. I believe the staged changes I have will fix that. |
@@ -2,10 +2,12 @@ | |||
|
|||
class <%= name.camelize %> < ActiveRecord::Migration[<%= migration_version %>] | |||
def up | |||
Aws::SessionStore::DynamoDB::Table.create_table | |||
options = Rails.application.config.session_options | |||
Aws::SessionStore::DynamoDB::Table.create_table(options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably dumb question - does this need to be idempotent? (Ie check if table already exists/update?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create_table in this gem is already idempotent, sorta. It uses options to construct create table options and will rescue ResourceInUseException (DDB validates). https://github.com/aws/aws-sessionstore-dynamodb-ruby/blob/main/lib/aws/session_store/dynamo_db/table.rb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate that this is idempotent so I can easily include in code which gets executed at every bin/dev and/or deploy.
Fixes concerns in #139
When running the rake tasks for create/delete/clean, and when performing database migrations, the rails environment is loaded, and the
DynamoDbStore
class (ActionDispatch::Session::AbstractStore
) is loaded. WhenDynamoDbStore
is loaded, options are searched in the YAML configuration files or explicitly through code. Action Dispatch will store the provided options insession_options
on the config. This change will retrieve those session storage options and provide them as options to the create/delete/clean tasks/migrations. Additionally, we should also search for a config file path using ENV. This change relies on changing ENV precedence.