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

Add MysqlAdapter transactions support #58

Merged
merged 3 commits into from
Jul 11, 2024
Merged

Conversation

joaopgmaria
Copy link
Contributor

@joaopgmaria joaopgmaria commented Jul 10, 2024

Description

This PR adds support for MySQL read-only transactions in sandbox mode.

@joaopgmaria joaopgmaria requested a review from a team as a code owner July 10, 2024 15:54
@joaopgmaria joaopgmaria changed the title [GDSXM-118] Add MysqlAdapter transactions support Add MysqlAdapter transactions support Jul 10, 2024
Copy link
Contributor

@erikkessler1 erikkessler1 left a comment

Choose a reason for hiding this comment

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

We should update the README to mention that it supports MySQL too:

This gem makes Rails console sessions less dangerous in specified environments by warning, color-coding, and auto-sandboxing PostgreSQL connections. In the future we'd like to extend this to make other external connections read-only too (e.g. disable job queueing, non-GET HTTP requests, etc.)

Also, we should probably update these integration tests to include support for MySQL: https://github.com/salsify/safer_rails_console/blob/master/spec/integration/patches/sandbox_spec.rb

I'd imagine the easiest way to do this would be to add a separate database in the existing apps that has a mysql adapter: https://github.com/salsify/safer_rails_console/blob/master/spec/internal/rails_7_1/config/database.yml and then add a model that uses that MySQL database.

Copy link
Contributor

@gremerritt gremerritt left a comment

Choose a reason for hiding this comment

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

LGTM once we add some tests.

@joaopgmaria joaopgmaria force-pushed the feature/add_mysql_support branch 15 times, most recently from e361e8d to 06b26bc Compare July 11, 2024 01:23
@joaopgmaria
Copy link
Contributor Author

@gremerritt I've reverted the changes to keep backward compatibility.
@erikkessler1 Tests are in but I am still working out one that fails despite not allowing edits to the db.

Converting this to draft pending resolution

@joaopgmaria joaopgmaria marked this pull request as draft July 11, 2024 01:27
@joaopgmaria joaopgmaria force-pushed the feature/add_mysql_support branch 2 times, most recently from ab64683 to 9c18e79 Compare July 11, 2024 12:32
@joaopgmaria joaopgmaria marked this pull request as ready for review July 11, 2024 12:37
@joaopgmaria joaopgmaria force-pushed the feature/add_mysql_support branch from 9c18e79 to 990893c Compare July 11, 2024 12:38
@joaopgmaria
Copy link
Contributor Author

Should be ready to review @erikkessler1 @gremerritt

@erikkessler1 I had to go with different environments (per adapter) instead of adapter-specific models.
For some reason, the adapter constant was not defined at startup, hence no patches were applied and thus, the failing MySQL test

Copy link
Contributor

@erikkessler1 erikkessler1 left a comment

Choose a reason for hiding this comment

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

A couple last thoughts, but looking good

Comment on lines +32 to +33
# Not possible to change a running transaction to read-only in MySQL
# https://dev.mysql.com/doc/refman/8.4/en/set-transaction.html
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we throw an error or provide some other indication that something has escaped the sandbox if we detect there is already an open transaction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. I'm having a hard time thinking of scenarios where this would even be possible.
Not sure if this is different in Postgres but in Mysql, since this is a new session, there shouldn't be any transactions running for this session.

There could, however, be running transactions for other session that, even if we could, we wouldn't want to make read-only. For this reason, throwing an error or informing the user doesn't make much sense to me.

I may be missing some scenarios though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not to sure either. Unfortunately, I wasn't able to glean anything from PR that originally added it for the Postgres version.

@joaopgmaria joaopgmaria merged commit 364d42f into master Jul 11, 2024
16 checks passed
@joaopgmaria joaopgmaria deleted the feature/add_mysql_support branch July 11, 2024 14:49
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.

3 participants