aws / aws-sdk-rails

Official repository for the aws-sdk-rails gem, which integrates the AWS SDK for Ruby with Ruby on Rails.
Other
596 stars 62 forks source link

DynamoDB Sessions don't work like documented #139

Closed thomaswitt closed 3 weeks ago

thomaswitt commented 2 months ago

The DynamoDB Sessions store seems to me rather poorly implemented and/or documented.

First, the Aws::SessionStore::DynamoDB::Table.create_table does seems to ignore basically all the options set in the Rails app, it always creates a table called sessions. Browsing swiftly through the code, I don't see any possibility how the Rails configuration would land up here: https://github.com/aws/aws-sessionstore-dynamodb-ruby/blob/b9d2ce50f86076bc4bfb7d90d8637080711dd487/lib/aws/session_store/dynamo_db/table.rb#L11

Second, the generate command creates a migration, which is rather useless if you use DynamoDB, as you don't have any migrations when not using ActiveRecord.

The configuration in the initialized also seems to be rather ignored.

Rails.application.config.session_store :dynamodb_store,
  key: '_myapp_session',
  table_name: "myapp_sessions_#{Rails.env}",
  max_age: 7 * 3600 * 24,
  max_stale: 3600 * 5,
  secret_key: Rails.application.credentials.dig(:secret_key_base)

Also I keep on getting the error undefined methodcommit_csrf_token' for an instance of Rack::Request` when using OmniAuth.

mullermp commented 2 months ago

Thanks for your feedback on this. Currently we don't have bandwidth to fix any of this. Do you have any recommendations on how to fix it or would you like to start a pull request?

thomaswitt commented 2 months ago

@mullermp Not really interested. That'd be a bigger code change than just a few lines. I didn't really do much digging on this issue so far as the code seems quite old (e.g. still asks for Read Capacity Units and doesn't support PAY_PER_REQUEST). I might build a custom solution for my project.

Personal comment: It is truly disappointing that a giant company like AWS apparently puts very little effort into Ruby/Rails support. Unfixed things like this, very un-ruby-esque APIs, etc …Ruby doesn't get much love from you guys and seems to be nowhere near a first class citizen there.

mullermp commented 2 months ago

I understand where you are coming from and I appreciate your sentiment. There are only so many of us here working on the Ruby products. I can assure you that me and the Ruby SDK team are passionate about Ruby and want to improve your experiences. We are working diligently on SDK features that cover almost 400 services and their APIs - it is not trivial. I do agree more investment needs to be done in 1P gems such as this and DDB session storage, etc, but we have to balance them with other company initiatives. We can carve out some time to look at DDB session storage (it was created before I joined the team!) and go from there.

thomaswitt commented 2 months ago

@mullermp I really highly appreciate your explanations and your efforts. Please let me know if I can be of any help. I think the session storage is a good idea, as everybody who wants to use DynamoDB is not interested in maintaining another database for sessions.

mullermp commented 1 month ago

I took a look into a few of these issues.

First, the Aws::SessionStore::DynamoDB::Table.create_table does seems to ignore basically all the options set in the Rails app

If you are referring to the session_store config snippet, I think this is by design (by Rails)? Migrations are run separate from rails initialization. Looking at create_table, it does take options, you can add your options to the create_table call in the migration for one time use. You could also use the config_file option which points to a yml file, I believe the generator will give you a skeleton template.

Second, the generate command creates a migration, which is rather useless if you use DynamoDB, as you don't have any migrations when not using ActiveRecord.

This is true - I think maybe a rake task for create/delete would be preferred, and we can take this as a feature request. ActiveRecord usage is separate from your session storage usage. I think at the time of this gem being created, aws-record did not exist, and noSQL wasn't as popular as it is now.

The configuration in the initialized also seems to be rather ignored.

Looking into this, I confirmed that the initializer is setting config and it's eventually piped through to SessionStore::DynamoDB::Configuration. Looking at SessionStore::DynamoDB::RackMiddleware it seems the only config that matters at runtime is locking, the secret key, and the error handler.

I think given how some of the config options are used (in and out of Rails execution), you should define your config in the dynamo_db_session_store.yml config that is provided on default generation, and use it per Rails.env (or globally). You can pass this config option to the create table, and it will be loaded when you run your application as well as when you clean up sessions with the rake task. The initializer would only be a default call to Rails.application.config.session_store :dynamodb_store

Also I keep on getting the error undefined method commit_csrf_token' for an instance of Rack::Request` when using OmniAuth. I don't see a reference to this method anywhere in these packages. It might be something unrelated? If you have a stack trace or more information I could look.

thomaswitt commented 2 weeks ago

@mullermp Thanks a lot for all the work. I was just trying the new session store out, and I ran into the problem that the configuration doesn't work (at least not as like documented - or I am doing/getting it wrong). I always get a table called sessions created with the default options:

?> 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=>"SUPERSECRET"}
>>       Aws::SessionStore::DynamoDB::Table.create_table
I, [2024-11-06T14:48:29.248255 #65506]  INFO -- : Table sessions created, waiting for activation...
I, [2024-11-06T14:48:29.255852 #65506]  INFO -- : Table sessions is now ready to use.
=> true
>> Aws::SessionStore::DynamoDB::Table.delete_table
I, [2024-11-06T14:48:32.773254 #65506]  INFO -- : Table sessions deleted.
=> true

?> 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=> "SUPERSECRET"}
>>       Aws::SessionStore::DynamoDB::Table.create_table
I, [2024-11-06T14:48:45.475762 #65506]  INFO -- : Table sessions created, waiting for activation...
I, [2024-11-06T14:48:45.480747 #65506]  INFO -- : Table sessions is now ready to use.
=> true
>> Aws::SessionStore::DynamoDB::Table.delete_table
I, [2024-11-06T14:48:48.954962 #65506]  INFO -- : Table sessions deleted.
=> true

The same is true when I put this configuration into config/initializers/session_store.rb or change data in config/dynamo_db_session_store.yml. It's always "sessions" which is created.

I also found an inconsistency in https://github.com/aws/aws-sdk-rails/blob/7245861a8c6666df552bff78c38ce370d54119c6/sample-app/config/initializers/session_store.rb where it says options = { key: '_sample_app_session' }. So I am not sure whether it's key or table_key. Doesn't work with both.

mullermp commented 2 weeks ago

Thanks for reaching back out. I released the new sessionstore gem which is major version 3, and aws-sdk-rails currently pins to major version 2. I have not released a new version of the rails gem yet. In your Gemfile, do you mind testing the experience by pulling the latest of both of these gems by setting the git option?

mullermp commented 2 weeks ago

Regarding the key vs table key, they are both valid. Table key is used in the session store. The key is a rack option that is passed through to rack abstract session. I believe there is a link to that on the readme in that gem.

thomaswitt commented 2 weeks ago

@mullermp I am currently using gem 'aws-sdk-rails', github: 'aws/aws-sdk-rails', branch: 'main'

Do I also need to include the session store gem like gem 'aws-sessionstore-dynamodb', github: 'aws/aws-sessionstore-dynamodb-ruby', branch: 'main' ?

mullermp commented 2 weeks ago

Ah ok. Sorry. I think I covered this partially in my response here https://github.com/aws/aws-sdk-rails/issues/139#issuecomment-2366880413.

Aws::SessionStore::DynamoDB::Table.create_table without options should not implicitly pull rails configuration because it is generic to rack. The values set in Rails.application.config.session_store :dynamo_db_store are passed to ActionDispatch::Session::DynamoDbStore which passed to Aws::SessionStore::DynamoDB::RackMiddleware for use in runtime and not creation/deletion/cleaning of the table. That said, table creation/deletion/cleaning, with rails, should respect configurations. ENV values should work right now. If you set DYNAMO_DB_SESSION_TABLE_NAME=<value> that should work. I think the rake task and the active record migration should possibly search and add the config file just like ActionDispatch::Session::DynamoDbStore does. This would be Aws::SessionStore::DynamoDB::Table.create_table(config_file: 'config/dynamo_db_session_store.yml') or the environment specific yml.

That said, I think I can make a quick change to improve that if that makes sense.

Edit - it seems passing config_file through as ENV complicates this, because it would never be used (the in-code takes precedence). Let me think through this.

thomaswitt commented 2 weeks ago

@mullermp Thanks – I'll await further instruction for my testing. Thanks for your time.