removed Configuration class #6682

AndrewChubatiuk · 2024-01-02T13:55:26Z

What type of PR is this?

removed not used Configuration class

Description

removed Configuration class, which was used for unencrypted options, which were already removed

How is this tested?

Unit tests (pytest, jest)
E2E Tests (Cypress)
Manually
N/A

masayuki038 · 2024-01-04T07:55:51Z

Thanks for extracting this update from #6671.

I asked some questions and you answered:

Since "encrypted_options" is already encrypted, why did you decide to encrypt "options" as well?
-> encrypted old unencrypted options to get rid of Configuration class, which was in a source code just for this purpose. there are no unencrypted options in a current model
Since this update is not related to SQLAlchemy version upgrade, can you make it a separate PR?
-> (Already done)
Is backward compatibility considered? Will this update require existing Redash users to re-register their data sources?
-> this is needed for users who for some reason are still using redash version with unencrypted options

codecov · 2024-01-04T07:59:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (30e7392) 63.38% compared to head (7aaac7c) 63.37%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6682      +/-   ##
==========================================
- Coverage   63.38%   63.37%   -0.01%     
==========================================
  Files         162      162              
  Lines       13176    13170       -6     
  Branches     1819     1819              
==========================================
- Hits         8351     8347       -4     
+ Misses       4534     4532       -2     
  Partials      291      291

Files	Coverage Δ
redash/models/__init__.py	`91.95% <ø> (ø)`
redash/models/types.py	`85.96% <100.00%> (+1.83%)`	⬆️

masayuki038

I would like to ask a question after understanding the above comments.

Is it correct to encrypt the "options" field because you would like to delete the Configuration class?

From my point of view, if there are two fields, "encrypted_options" and "options", it would be natural for the "options" field to be unencrypted. Therefore, I think it is better to keep Configuration class...

AndrewChubatiuk · 2024-01-04T11:54:06Z

there is only options property, which is encrypted_options column in a database. there are no two types of properties options and encrypted_options
https://github.com/getredash/redash/blob/69df17fe8e7eacdf597088789142350af8e21aad/redash/models/__init__.py#L113

masayuki038 · 2024-01-05T10:25:39Z

Thanks for your explanation. I misunderstood. The "options" field has been already removed.

As a personal view, I think it's a bit of an overkill to remove the class used in past migration files. Instead of deleting the Configuration class, I think it would be better to indicate that it is deprecated.

Do anyone have opinions?

AndrewChubatiuk · 2024-01-15T15:34:00Z

@guidopetri @eradman could you please review it as well?

eradman

This change looks good to me

justinclift · 2024-01-18T03:01:27Z

Looks to be failing some of the CI tests now:

redash/models/types.py:20:16: F821 Undefined name `TypeDecorator`
redash/models/types.py:21:12: F821 Undefined name `db`

@AndrewChubatiuk Are you ok to fix those so we can merge this? 😄

AndrewChubatiuk · 2024-01-18T06:02:10Z

@justinclift Done

justinclift · 2024-01-18T07:41:26Z

@AndrewChubatiuk Thanks. Weirdly enough, there still seem to be things failing. 😦

Are you ok to investigate?

justinclift · 2024-01-18T07:43:00Z

I'm running the failed tests again, just in case it was some kind of temporary infrastructure weirdness. Pretty sure that's not the case though, but re-running the tests again to double check isn't going to hurt. 😄

justinclift · 2024-01-18T07:43:50Z

The re-tests also failed in the same spot. Oh well. 😉

AndrewChubatiuk · 2024-01-18T09:24:51Z

@justinclift sorry, there was a typo, ti should be fixed now

justinclift · 2024-01-18T09:55:47Z

package.json

@@ -34,7 +34,7 @@
    "url": "git+https://github.com/getredash/redash.git"
  },
  "engines": {
-    "node": ">14.16.0 <17.0.0",
+    "node": ">14.16.0 <19.0.0",


Just noticed this. Is this change needed in this PR?

nope, it was needed only for a local purposes, I have nodejs 18 on my system, reverted this change

@justinclift it's now green

Thanks for keeping an eye on it, I'll merge it now. 😄

justinclift

Carrying over @eradman's approval of this PR. 😄

zachliu · 2024-06-18T21:23:09Z

I think this is wrong, at least if your database is at or before the e5c7a4e2df4d version and if your "Alert Destinations" aren't empty.

By changing

sa.Column("options", ConfigurationContainer.as_mutable(Configuration)),

into

sa.Column(
    "options", 
    ConfigurationContainer.as_mutable(
        EncryptedConfiguration(
            sa.Text, settings.DATASOURCE_SECRET_KEY, FernetEngine
        )
    ),
),

you are asking the alembic database migration to treat the existing column options as an encrypted column. As a result, when the migration process tries to copy the information from the old options column to the new encrypted_options column,

conn.execute(
    notification_destinations.update()
        .where(notification_destinations.c.id == dest.id)
        .values(encrypted_options=dest.options)
)

it starts decrypting the plain strings and leads to a very confusing error traceback:

[2024-06-18 19:09:48,559][PID:435][INFO][alembic.runtime.migration] Running upgrade e5c7a4e2df4d -> d7d747033183, encrypt alert destinations
Traceback (most recent call last):
  File "/app/manage.py", line 9, in <module>
    manager()
  File "/usr/local/lib/python3.8/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/usr/local/lib/python3.8/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/local/lib/python3.8/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/local/lib/python3.8/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/usr/local/lib/python3.8/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/click/decorators.py", line 26, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/flask/cli.py", line 357, in decorator
    return __ctx.invoke(f, *args, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/flask_migrate/cli.py", line 134, in upgrade
    _upgrade(directory, revision, sql, tag, x_arg)
  File "/usr/local/lib/python3.8/site-packages/flask_migrate/__init__.py", line 95, in wrapped
    f(*args, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/flask_migrate/__init__.py", line 280, in upgrade
    command.upgrade(config, revision, sql=sql, tag=tag)
  File "/usr/local/lib/python3.8/site-packages/alembic/command.py", line 403, in upgrade
    script.run_env()
  File "/usr/local/lib/python3.8/site-packages/alembic/script/base.py", line 583, in run_env
    util.load_python_file(self.dir, "env.py")
  File "/usr/local/lib/python3.8/site-packages/alembic/util/pyfiles.py", line 95, in load_python_file
    module = load_module_py(module_id, path)
  File "/usr/local/lib/python3.8/site-packages/alembic/util/pyfiles.py", line 113, in load_module_py
    spec.loader.exec_module(module)  # type: ignore
  File "<frozen importlib._bootstrap_external>", line 843, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "migrations/env.py", line 93, in <module>
    run_migrations_online()
  File "migrations/env.py", line 85, in run_migrations_online
    context.run_migrations()
  File "<string>", line 8, in run_migrations
  File "/usr/local/lib/python3.8/site-packages/alembic/runtime/environment.py", line 948, in run_migrations
    self.get_context().run_migrations(**kw)
  File "/usr/local/lib/python3.8/site-packages/alembic/runtime/migration.py", line 627, in run_migrations
    step.migration_fn(**kw)
  File "/app/migrations/versions/d7d747033183_encrypt_alert_destinations.py", line 60, in upgrade
    .values(encrypted_options=dest.options)
  File "/usr/local/lib/python3.8/site-packages/sqlalchemy/sql/type_api.py", line 1278, in process
    return process_value(impl_processor(value), dialect)
  File "/usr/local/lib/python3.8/site-packages/sqlalchemy/sql/sqltypes.py", line 946, in process
    value = bytes(value)
TypeError: string argument without an encoding

This error has led me on a merry chase, in a very deep 🐰 🕳️
it has nothing to do with kvesteri/sqlalchemy-utils#366 or kvesteri/sqlalchemy-utils#425 or kvesteri/sqlalchemy-utils#444 or even sqlalchemy/sqlalchemy#5073.

If someone is on the same boat as me, do NOT try to "fix" it by adding the encoding, otherwise you'll see

[2024-06-18 18:52:35,035][PID:837][INFO][alembic.runtime.migration] Running upgrade e5c7a4e2df4d -> d7d747033183, encrypt alert destinations
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/cryptography/fernet.py", line 117, in _get_unverified_token_data
    data = base64.urlsafe_b64decode(token)
  File "/usr/local/lib/python3.8/base64.py", line 133, in urlsafe_b64decode
    return b64decode(s)
  File "/usr/local/lib/python3.8/base64.py", line 87, in b64decode
    return binascii.a2b_base64(s)
binascii.Error: Invalid base64-encoded string: number of data characters (25) cannot be 1 more than a multiple of 4

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/app/manage.py", line 9, in <module>
    manager()
  File "/usr/local/lib/python3.8/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/usr/local/lib/python3.8/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/local/lib/python3.8/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/local/lib/python3.8/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/usr/local/lib/python3.8/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/click/decorators.py", line 26, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/flask/cli.py", line 357, in decorator
    return __ctx.invoke(f, *args, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/flask_migrate/cli.py", line 134, in upgrade
    _upgrade(directory, revision, sql, tag, x_arg)
  File "/usr/local/lib/python3.8/site-packages/flask_migrate/__init__.py", line 95, in wrapped
    f(*args, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/flask_migrate/__init__.py", line 280, in upgrade
    command.upgrade(config, revision, sql=sql, tag=tag)
  File "/usr/local/lib/python3.8/site-packages/alembic/command.py", line 403, in upgrade
    script.run_env()
  File "/usr/local/lib/python3.8/site-packages/alembic/script/base.py", line 583, in run_env
    util.load_python_file(self.dir, "env.py")
  File "/usr/local/lib/python3.8/site-packages/alembic/util/pyfiles.py", line 95, in load_python_file
    module = load_module_py(module_id, path)
  File "/usr/local/lib/python3.8/site-packages/alembic/util/pyfiles.py", line 113, in load_module_py
    spec.loader.exec_module(module)  # type: ignore
  File "<frozen importlib._bootstrap_external>", line 843, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "migrations/env.py", line 93, in <module>
    run_migrations_online()
  File "migrations/env.py", line 85, in run_migrations_online
    context.run_migrations()
  File "<string>", line 8, in run_migrations
  File "/usr/local/lib/python3.8/site-packages/alembic/runtime/environment.py", line 948, in run_migrations
    self.get_context().run_migrations(**kw)
  File "/usr/local/lib/python3.8/site-packages/alembic/runtime/migration.py", line 627, in run_migrations
    step.migration_fn(**kw)
  File "/app/migrations/versions/d7d747033183_encrypt_alert_destinations.py", line 60, in upgrade
    .values(encrypted_options=dest.options)
  File "/usr/local/lib/python3.8/site-packages/sqlalchemy/sql/type_api.py", line 1283, in process
    return process_value(value, dialect)
  File "/app/redash/models/types.py", line 17, in process_result_value
    super(EncryptedConfiguration, self).process_result_value(value, dialect)
  File "/home/redash/.local/lib/python3.8/site-packages/sqlalchemy_utils/types/encrypted/encrypted_type.py", line 414, in process_result_value
    decrypted_value = self.engine.decrypt(value)
  File "/home/redash/.local/lib/python3.8/site-packages/sqlalchemy_utils/types/encrypted/encrypted_type.py", line 218, in decrypt
    decrypted = self.fernet.decrypt(value.encode())
  File "/usr/local/lib/python3.8/site-packages/cryptography/fernet.py", line 86, in decrypt
    timestamp, data = Fernet._get_unverified_token_data(token)
  File "/usr/local/lib/python3.8/site-packages/cryptography/fernet.py", line 119, in _get_unverified_token_data
    raise InvalidToken
cryptography.fernet.InvalidToken

I print out the "value" and realize it's trying to decrypt plain strings

token = '{"addresses": "me@domain.com"}'    # value from the "options" column of the notification_destinations table
base64.urlsafe_b64decode(token)

this directly causes binascii.Error: Invalid base64-encoded string: number of data characters (25) cannot be 1 more than a multiple of 4

I ended up reverting this PR in my implementation of redash and everything is ok so far.

This reverts commit 97db492. Based on information in getredash#6954 (comment)

AndrewChubatiuk changed the title ~~Removed configuration~~ Removed Configuration class Jan 2, 2024

AndrewChubatiuk mentioned this pull request Jan 2, 2024

Sqlalchemy 2.0 #6671

Closed

10 tasks

AndrewChubatiuk force-pushed the removed-configuration branch from eba0847 to 69df17f Compare January 3, 2024 08:20

AndrewChubatiuk changed the title ~~Removed Configuration class~~ removed Configuration class Jan 3, 2024

masayuki038 reviewed Jan 4, 2024

View reviewed changes

eradman previously approved these changes Jan 16, 2024

View reviewed changes

AndrewChubatiuk force-pushed the removed-configuration branch from 69df17f to 0902cc9 Compare January 16, 2024 20:29

AndrewChubatiuk dismissed eradman’s stale review via 7cebdef January 17, 2024 21:18

AndrewChubatiuk force-pushed the removed-configuration branch from 0902cc9 to 7cebdef Compare January 17, 2024 21:18

AndrewChubatiuk force-pushed the removed-configuration branch from c191be0 to aca0e2f Compare January 18, 2024 06:01

AndrewChubatiuk force-pushed the removed-configuration branch 2 times, most recently from a995733 to 9a248a5 Compare January 18, 2024 09:23

justinclift reviewed Jan 18, 2024

View reviewed changes

AndrewChubatiuk force-pushed the removed-configuration branch from 9a248a5 to ed2075a Compare January 18, 2024 10:00

removed unused configuration class

7aaac7c

AndrewChubatiuk force-pushed the removed-configuration branch from ed2075a to 7aaac7c Compare January 18, 2024 10:01

justinclift approved these changes Jan 18, 2024

View reviewed changes

justinclift merged commit 97db492 into getredash:master Jan 18, 2024
15 checks passed

AndrewChubatiuk deleted the removed-configuration branch January 18, 2024 11:55

zachliu mentioned this pull request Jun 19, 2024

DB upgrade from 8 -> preview fails #6954

Closed

eradman added a commit to eradman/redash that referenced this pull request Jul 19, 2024

Revert "Removed unused configuration class (getredash#6682)"

1476a99

This reverts commit 97db492. Based on information in getredash#6954 (comment)

justinclift pushed a commit that referenced this pull request Aug 1, 2024

Revert "Removed unused configuration class (#6682)" (#7071)

5257e39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

removed Configuration class #6682

removed Configuration class #6682

AndrewChubatiuk commented Jan 2, 2024

masayuki038 commented Jan 4, 2024

codecov bot commented Jan 4, 2024 •

edited

Loading

masayuki038 left a comment •

edited

Loading

AndrewChubatiuk commented Jan 4, 2024 •

edited

Loading

masayuki038 commented Jan 5, 2024 •

edited

Loading

AndrewChubatiuk commented Jan 15, 2024

eradman left a comment

justinclift commented Jan 18, 2024

AndrewChubatiuk commented Jan 18, 2024

justinclift commented Jan 18, 2024

justinclift commented Jan 18, 2024

justinclift commented Jan 18, 2024

AndrewChubatiuk commented Jan 18, 2024

justinclift Jan 18, 2024

AndrewChubatiuk Jan 18, 2024

AndrewChubatiuk Jan 18, 2024

justinclift Jan 18, 2024

justinclift left a comment

zachliu commented Jun 18, 2024 •

edited

Loading

removed Configuration class #6682

removed Configuration class #6682

Conversation

AndrewChubatiuk commented Jan 2, 2024

What type of PR is this?

Description

How is this tested?

masayuki038 commented Jan 4, 2024

codecov bot commented Jan 4, 2024 • edited Loading

Codecov Report

masayuki038 left a comment • edited Loading

Choose a reason for hiding this comment

AndrewChubatiuk commented Jan 4, 2024 • edited Loading

masayuki038 commented Jan 5, 2024 • edited Loading

AndrewChubatiuk commented Jan 15, 2024

eradman left a comment

Choose a reason for hiding this comment

justinclift commented Jan 18, 2024

AndrewChubatiuk commented Jan 18, 2024

justinclift commented Jan 18, 2024

justinclift commented Jan 18, 2024

justinclift commented Jan 18, 2024

AndrewChubatiuk commented Jan 18, 2024

justinclift Jan 18, 2024

Choose a reason for hiding this comment

AndrewChubatiuk Jan 18, 2024

Choose a reason for hiding this comment

AndrewChubatiuk Jan 18, 2024

Choose a reason for hiding this comment

justinclift Jan 18, 2024

Choose a reason for hiding this comment

justinclift left a comment

Choose a reason for hiding this comment

zachliu commented Jun 18, 2024 • edited Loading

codecov bot commented Jan 4, 2024 •

edited

Loading

masayuki038 left a comment •

edited

Loading

AndrewChubatiuk commented Jan 4, 2024 •

edited

Loading

masayuki038 commented Jan 5, 2024 •

edited

Loading

zachliu commented Jun 18, 2024 •

edited

Loading