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

connect() will not forward username if api_key is used instead of password #75

Open
alextanski opened this issue Feb 9, 2022 · 13 comments

Comments

@alextanski
Copy link

I am on version 0.5.1 and when connecting via api-key I've realized that the username will not get passed over to the session. This will cause issues e.g. in scrunch when trying to take editorship using get_mutable_dataset(..., editor=True).

import pycrunch
print pycrunch.__version__
api_key_conn= pycrunch.connect('alexander.buchhammer@yougov.de', api_key=api_key)
pw_conn = pycrunch.connect('alexander.buchhammer@yougov.de', pw=pw)
print 'api-key:', api_key_conn.session.email
print 'password:', pw_conn.session.email

will print:

0.5.1
api-key: None
password: alexander.buchhammer@yougov.de
@jjdelc
Copy link
Contributor

jjdelc commented Feb 9, 2022

That session.email value is stored from the credentials used to authenticate. When you sign in via API key, you will find that key under api_key_conn.session.token and will print out the key you used.

If you want to obtain the authenticated user email, should be obtained from the API like this:

api_key_conn.user["body"]["email"]

@alextanski
Copy link
Author

alextanski commented Feb 10, 2022

Ah, right - I see. But shouldn't we copy that value for consistency and compatibility with old-authentication based code, e.g. like stated above in scrunch?:
https://github.com/Crunch-io/scrunch/blob/master/scrunch/mutable_dataset.py#L23 where it will call session.email. Might be easier adjust this directly here in pycrunch than to look through all dependent (production!) code.

@alextanski
Copy link
Author

alextanski commented Feb 24, 2022

Hey @jjdelc - I have updated pycrunch to the latest version (0.5.3) as that one had #76 released. I've also updated to the latest scrunch (0.11.0). This version will now throw an AttributeError when connecting via api_key and then trying to derive the session/user email in scrunch:

     21     ds = MutableDataset(shoji_ds)
     22     if editor is True:
---> 23         ds.change_editor(root.session.email)
     24     return ds
     25 

C:\Users\alt\AppData\Local\conda\conda\envs\dev\lib\site-packages\pycrunch\elements.pyc in email(self)
    324         )
    325         if self.__email is None:
--> 326             self.__email = self.user["body"]["email"]
    327         return self.__email
    328 

AttributeError: 'ScrunchSession' object has no attribute 'user'

I am raising this over here as the error comes from pycrunch. Did I miss anything, maybe the versions/releases not yet maching?

@jjdelc
Copy link
Contributor

jjdelc commented Feb 24, 2022

Released Scrunch 0.11.1 and Pycrunch 0.5.3 that work correctly around this.

@alextanski
Copy link
Author

alextanski commented Feb 24, 2022

Sorry to be a pain, scrunch 0.11.1 will return a different AttributeError, same code nows results in:

AttributeErrorTraceback (most recent call last)
<ipython-input-4-8001ee7c3f63> in <module>()
      1 connection = scrunch.connect(user='alexander.buchhammer@yougov.de', api_key=api_key)
----> 2 dataset = scrunch.get_mutable_dataset(dataset='be493c840fcb4e0e9fc689c8cc583827', connection=connection, editor=True)

C:\Users\alt\AppData\Local\conda\conda\envs\dev\lib\site-packages\scrunch\mutable_dataset.pyc in get_mutable_dataset(dataset, connection, editor, project)
     21     ds = MutableDataset(shoji_ds)
     22     if editor is True:
---> 23         authenticated_url = root.session.views["user_url"]
     24         ds.change_editor(authenticated_url)
     25     return ds

AttributeError: 'ScrunchSession' object has no attribute 'views'

(This now technically belongs to the scrunch repo though).

@jjdelc
Copy link
Contributor

jjdelc commented Feb 24, 2022

I found why my test skipped that check, in all of them the logged user was already the current editor not stepping in that path. I made a fix in master branch. Can you make sure it works of you with current master?

@alextanski
Copy link
Author

alextanski commented Feb 28, 2022

@jjdelc Thanks! Yep, that seems to work now with the latest scrunch Master. Double-checked password auth again as well, all good now I think. 👍

However, checking the pycrunch session, I think the email/username will still not get returned properly when using the api-key, although the code is supposed to catch that now, right (or maybe I've misunderstood if this should happen at all!)?:

print api_key_conn.session.email

AttributeErrorTraceback (most recent call last)
<ipython-input-29-c741d043621f> in <module>()
----> 1 api_key_conn.session.email

C:\Users\alt\AppData\Local\conda\conda\envs\dev\lib\site-packages\pycrunch\elements.pyc in email(self)
    324         )
    325         if self.__email is None:
--> 326             self.__email = self.user["body"]["email"]
    327         return self.__email
    328 

AttributeError: 'ElementSession' object has no attribute 'user'

@alextanski
Copy link
Author

@jjdelc Could push a new version tag to the scrunch repo, so I can fetch the Master's changes easily for the DP guys?

@jjdelc
Copy link
Contributor

jjdelc commented Mar 7, 2022

I'm debugging this in pycrunch, but Scrunch is not using session.email anywhere or is it?

@alextanski
Copy link
Author

alextanski commented Mar 8, 2022

@jjdelc Nope - I don't think it's anywhere in scrunch. I think in combination with the latest scrunch changes (once the master has a new version tag), we're safe when it comes to authentication via both password and api_key, but I just wondered if we should have the value being copied over for a) any unknown pieces of prod code that might use in the SSCs/CENXs and b) consistency of the interface.

@jjdelc
Copy link
Contributor

jjdelc commented Mar 9, 2022

@alextanski can you try this release? https://pypi.org/project/pycrunch/

@alextanski
Copy link
Author

@jjdelc 0.5.4 throws:

<ipython-input-11-f2717f55cb97> in <module>()
----> 1 import pycrunch

C:\Users\alt\AppData\Local\conda\conda\envs\dev\lib\site-packages\pycrunch\__init__.py in <module>()
    186 from six.moves import urllib
    187 
--> 188 from pycrunch import cubes
    189 from pycrunch import elements
    190 from pycrunch import shoji

C:\Users\alt\AppData\Local\conda\conda\envs\dev\lib\site-packages\pycrunch\cubes.py in <module>()
      3 import six
      4 
----> 5 from pycrunch import elements
      6 
      7 

C:\Users\alt\AppData\Local\conda\conda\envs\dev\lib\site-packages\pycrunch\elements.py in <module>()
     29 from requests.utils import get_environ_proxies
     30 
---> 31 from pycrunch import lemonpy
     32 from pycrunch.progress import DefaultProgressTracking
     33 from .version import __version__

ImportError: cannot import name lemonpy

@alextanski
Copy link
Author

@jjdelc Also, I think if this (the original pycrunch issue here) cannot be fixed without too much effort and bending backwards, we should first add a new release to scrunch (1.11.2 ?) that has the latest changes from Master to make both password and API key auth work at while we transition DP over to their API keys?

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

No branches or pull requests

2 participants