Incorrectly documented SQLALCHEMY_CUSTOM_PASSWORD_STORE

[ottomata]

The docs say to do:

def example_lookup_password(url):
    secret = <<get password from external framework>>
    return 'secret'

SQLALCHEMY_CUSTOM_PASSWORD_STORE = example_lookup_password

But, superset sets this function as a instance variable on the Database model class, and calls it as such, with self:

conn.password = self.custom_password_store(conn)

When called with self, it is expected that the function take a self argument as the first parameter.

I fixed this temporarily in my function by adding a dummy 'self', but probably the code should not need this. :)

[mistercrunch]

@fabianmenges I think you contributed this originally. The function should probably be stamped in module scope instead of as a class attribute.

[fabianmenges]

Hm, interesting, we are using this slightly different, which is why I didn't run into it... e.g.:

class ExamplePasswordStore(object):
  ...
  def get_password(self, sqla_connection):
    key = self._gen_key(sqla_connection.__dict__)
    return self.password_lookup.get(key)

p_store = ExamplePasswordStore(...)
SQLALCHEMY_CUSTOM_PASSWORD_STORE = p_store.get_password

Not sure how this wasn't caught in the unit test.
https://github.com/apache/incubator-superset/blob/c84211ec449a6fc0e80f1e1b6df54cb4340ca3f7/tests/core_tests.py#L311

Anyways, two options. Either change the documentation or move the 'function' custom_password_store out of the Database class.

@ottomata are you using the self reference to the Database object inside the password store? Otherwise I might just move the function out. @mistercrunch, any preference?

[fabianmenges]

Oh, I just realized this is probably because this field is on a SQLAlchemy model and we probably have some weird interaction here. I should probably move this function out of the Database class.

[ottomata]

Yeah, I’m not using `self` at all. I only put it in there to make it work. I think a custom function shouldn’t need reference to a class instance, but should be standalone. +1 to moving it out of the Database class. :)

…

On Thu, Dec 14, 2017 at 10:16 AM, fabianmenges ***@***.***> wrote: Oh, I just realized this is probably because this field is on a SQLAlchemy model and we probably have some weird interaction here. I should probably move this function out of the Database class. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4043 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABjPxkJrBHGElykFSZBUVfRyKlFXdR8qks5tATvHgaJpZM4Q9_xh> .

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.