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

Use Rails session store options in table tasks and migrations #157

Merged
merged 5 commits into from
Nov 11, 2024

Conversation

mullermp
Copy link
Contributor

@mullermp mullermp commented Nov 6, 2024

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. When DynamoDbStore is loaded, options are searched in the YAML configuration files or explicitly through code. Action Dispatch will store the provided options in session_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.

@mullermp
Copy link
Contributor Author

mullermp commented Nov 6, 2024

@thomaswitt

@mullermp
Copy link
Contributor Author

mullermp commented Nov 7, 2024

@thomaswitt Did these changes work for you?

@thomaswitt
Copy link

@mullermp I can confirm that ENV works now:

$ grep aws- Gemfile
gem 'aws-sdk-rails' , github: 'aws/aws-sdk-rails', branch: 'sessionstore-config-load'
gem 'aws-sessionstore-dynamodb' , github: 'aws/aws-sessionstore-dynamodb-ruby', branch: 'env-yaml'
$ export DYNAMO_DB_SESSION_TABLE_NAME='your-table-name'
$ bundle exec rails console
Loading development environment (Rails 7.2.2)
>> Rails.application.config.session_store
=> ActionDispatch::Session::CookieStore
?> Rails.application.config.session_store :dynamo_db_store,
?> name: "test-sessions_#{Rails.env}",
?> key: 'test-session',
>> secret_key: Rails.application.credentials.dig(:secret_key_base)
=>
{:name=>"test-sessions_development",
 :key=>"test-session",
 :secret_key=> "XXX"}
>> Aws::SessionStore::DynamoDB::Table.create_table
I, [2024-11-08T11:09:52.361606 #34563]  INFO -- : Table your-table-name created, waiting for activation...
I, [2024-11-08T11:09:52.369849 #34563]  INFO -- : Table your-table-name is now ready to use.
=> true

So you see the ENV takes precedence and works. Without the ENV, still a sessions table is created, so the arguments to Rails.application.config.session_store :dynamo_db_store are still being ignored.

Is ENV now the way to go?

@mullermp
Copy link
Contributor Author

mullermp commented Nov 8, 2024

The configuration should still work. Your keys should be table_name and table_key.

@mullermp
Copy link
Contributor Author

mullermp commented Nov 8, 2024

Additional keys like key are used in rack here https://www.rubydoc.info/gems/rack/2.0.1/Rack/Session/Abstract/Persisted

@thomaswitt
Copy link

The configuration should still work. Your keys should be table_name and table_key.

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.

?> Rails.application.config.session_store :dynamo_db_store,
?> table_name: "test-sessions_#{Rails.env}",
?> table_key: 'test-session',
>> secret_key: Rails.application.credentials.dig(:secret_key_base)
=>
{:table_name=>"test-sessions_development",
 :table_key=>"test-session",
 :secret_key=>
  "XXX"}
>> Aws::SessionStore::DynamoDB::Table.create_table
>>
I, [2024-11-08T15:23:07.012618 #16430]  INFO -- : Table your-table-name-test created, waiting for activation...
I, [2024-11-08T15:23:07.022746 #16430]  INFO -- : Table your-table-name-test is now ready to use.

@thomaswitt
Copy link

thomaswitt commented Nov 8, 2024

Additionally, when I don't set the ENV var, I get another error:

?> Rails.application.config.session_store :dynamo_db_store,
?> table_name: "my_test",
>> table_key: 'my_test'
=> {:table_name=>"my_test", :table_key=>"my_test"}
>> Aws::SessionStore::DynamoDB::Table.create_table
/Users/thomas/.rbenv/versions/3.3.6/lib/ruby/gems/3.3.0/gems/irb-1.14.1/lib/irb.rb:1260:in `full_message': Invalid table/index name.  Table/index names must be between 3 and 255 characters long, and may contain only the characters a-z, A-Z, 0-9, '_', '-', and '.' (Aws::DynamoDB::Errors::ValidationException)

Also doesn't work when removing the underscores.

@mullermp
Copy link
Contributor Author

mullermp commented Nov 8, 2024

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.

@mullermp
Copy link
Contributor Author

mullermp commented Nov 8, 2024

How are you using repl? Are you using rails console? Does this still occur when you run the dynamo_db:session_store:create task?

@mullermp
Copy link
Contributor Author

mullermp commented Nov 8, 2024

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.

@thomaswitt
Copy link

How are you using repl? Are you using rails console? Does this still occur when you run the dynamo_db:session_store:create task?

I am using bundle exec rails console … I can try the initalizer and report back.

@mullermp mullermp requested a review from alextwoods November 10, 2024 23:37
@mullermp
Copy link
Contributor Author

mullermp commented Nov 11, 2024

@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 Table.create_table, the Rails config is not used, because the gem is generic to Rack applications. You can pass options to that method if you'd like (even the same way I'm doing so in the rake task). If you don't pass options, ENV is used, and if ENV sets a config file, those values are used.

@thomaswitt
Copy link

thomaswitt commented Nov 11, 2024

@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.

@thomaswitt
Copy link

@mullermp Just as another heads up, as I tried this with some real life code I got an exception

undefined method `commit_csrf_token' for an instance of Rack::Request
Extracted source (around line #73):

    module SessionObject # :nodoc:
      def commit_session(req, res)
        req.commit_csrf_token
        super(req, res)
      end

 actionpack (7.2.2) lib/action_dispatch/middleware/session/abstract_store.rb:73:in `commit_session'
rack-session (2.0.0) lib/rack/session/abstract/id.rb:274:in `context'
rack-session (2.0.0) lib/rack/session/abstract/id.rb:266:in `call'
actionpack (7.2.2) lib/action_dispatch/middleware/cookies.rb:704:in `call'
actionpack (7.2.2) lib/action_dispatch/middleware/callbacks.rb:31:in `block in call'
activesupport (7.2.2) lib/active_support/callbacks.rb:101:in `run_callbacks'
actionpack (7.2.2) lib/action_dispatch/middleware/callbacks.rb:30:in `call'
actionpack (7.2.2) lib/action_dispatch/middleware/executor.rb:16:in `call'
actionpack (7.2.2) lib/action_dispatch/middleware/actionable_exceptions.rb:18:in `call'
actionpack (7.2.2) lib/action_dispatch/middleware/debug_exceptions.rb:31:in `call'
web-console (4.2.1) lib/web_console/middleware.rb:132:in `call_app'
web-console (4.2.1) lib/web_console/middleware.rb:28:in `block in call'
web-console (4.2.1) lib/web_console/middleware.rb:17:in `catch'
web-console (4.2.1) lib/web_console/middleware.rb:17:in `call'
actionpack (7.2.2) lib/action_dispatch/middleware/show_exceptions.rb:32:in `call'
railties (7.2.2) lib/rails/rack/logger.rb:41:in `call_app'
railties (7.2.2) lib/rails/rack/logger.rb:29:in `call'
actionpack (7.2.2) lib/action_dispatch/middleware/remote_ip.rb:96:in `call'
request_store (1.7.0) lib/request_store/middleware.rb:19:in `call'
actionpack (7.2.2) lib/action_dispatch/middleware/request_id.rb:33:in `call'
rack (3.1.8) lib/rack/method_override.rb:28:in `call'
rack (3.1.8) lib/rack/runtime.rb:24:in `call'
activesupport (7.2.2) lib/active_support/cache/strategy/local_cache_middleware.rb:29:in `call'
actionpack (7.2.2) lib/action_dispatch/middleware/server_timing.rb:61:in `block in call'
actionpack (7.2.2) lib/action_dispatch/middleware/server_timing.rb:26:in `collect_events'
actionpack (7.2.2) lib/action_dispatch/middleware/server_timing.rb:60:in `call'
actionpack (7.2.2) lib/action_dispatch/middleware/executor.rb:16:in `call'
actionpack (7.2.2) lib/action_dispatch/middleware/static.rb:27:in `call'
rack (3.1.8) lib/rack/sendfile.rb:114:in `call'
actionpack (7.2.2) lib/action_dispatch/middleware/host_authorization.rb:143:in `call'
rack-mini-profiler (3.3.1) lib/mini_profiler.rb:334:in `call'
railties (7.2.2) lib/rails/engine.rb:535:in `call'
puma (6.4.3) lib/puma/configuration.rb:272:in `call'
puma (6.4.3) lib/puma/request.rb:100:in `block in handle_request'
puma (6.4.3) lib/puma/thread_pool.rb:378:in `with_force_shutdown'
puma (6.4.3) lib/puma/request.rb:99:in `handle_request'
puma (6.4.3) lib/puma/server.rb:464:in `process_client'
puma (6.4.3) lib/puma/server.rb:245:in `block in run'
puma (6.4.3) lib/puma/thread_pool.rb:155:in `block in spawn_thread'

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

@mullermp
Copy link
Contributor Author

Yes, that is expected. As mentioned, Table.create_table is part of the sessionstore gem. It cannot load Rails configuration. If you set the config in your initializer, and run the provided rake task rake dynamo_db:session_store:create_table, it should load that configuration and use it.

@mullermp
Copy link
Contributor Author

mullermp commented Nov 11, 2024

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.

1b6559c

@@ -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)
Copy link
Contributor

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?)

Copy link
Contributor Author

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

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.

Gemfile Outdated Show resolved Hide resolved
@mullermp mullermp merged commit 4476f96 into main Nov 11, 2024
12 checks passed
@mullermp mullermp deleted the sessionstore-config-load branch November 11, 2024 17:19
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.

4 participants