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

Added auth handler for release 3.0 #9

Merged
merged 4 commits into from Sep 12, 2019

Conversation

@eddwhite
Copy link
Contributor

commented Aug 19, 2019

No description provided.

@eddwhite

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019


def create_session_token(self, r, **kwargs):
prep = requests.Request(
method='POST', url=r.request.url.split('api/v1')[0] + 'api/v1/auth',

This comment has been minimized.

Copy link
@LouisBrunner

LouisBrunner Aug 20, 2019

Member

It would be nicer/easier to just inject the base URL in the constructor instead of doing splits

self.create_session_token(r, **kwargs)

else:
self.renew_session_token(r, **kwargs)

This comment has been minimized.

Copy link
@LouisBrunner

LouisBrunner Aug 20, 2019

Member

Renew needs to happen in the background, before a 401 because in that case your token is invalid and you need to create a new one.



def __call__(self, r):
r.headers['Authorization'] = 'Bearer {}'.format(self.session_token)

This comment has been minimized.

Copy link
@LouisBrunner

LouisBrunner Aug 20, 2019

Member

This pattern is repeated 3 times, it would be nicer to have it in one function

self.session_token = None
eva_error('renew auth request error', _r)

return _r

This comment has been minimized.

Copy link
@LouisBrunner

LouisBrunner Aug 20, 2019

Member

What is the point of returning if we don't use the value?

headers={'Authorization': 'Bearer {}'.format(self.session_token)},
).prepare()

_r = r.connection.send(prep, **kwargs)

This comment has been minimized.

Copy link
@LouisBrunner

LouisBrunner Aug 20, 2019

Member

_r is a confusing variable name, especially as Python has special meaning for underscore prefixes


elif _r.status_code != 204:
self.session_token = None
eva_error('renew auth request error', _r)

This comment has been minimized.

Copy link
@LouisBrunner

LouisBrunner Aug 20, 2019

Member

Does this crash properly the request trying to do auth?

# Retry original request with latest session token, in case it has changed
retry_req = r.request.copy()
retry_req.headers['Authorization'] = 'Bearer {}'.format(self.session_token)
return r.connection.send(retry_req, **kwargs)

This comment has been minimized.

Copy link
@LouisBrunner

LouisBrunner Aug 20, 2019

Member

What is the point of returning here?

This comment has been minimized.

Copy link
@eddwhite

eddwhite Aug 20, 2019

Author Contributor

Have to return a Response object here. This requests hook allows the response to be modified before the original request call returns

This comment has been minimized.

Copy link
@LouisBrunner

LouisBrunner Aug 20, 2019

Member

Ok, I couldn't find any documentation on this, so that's what I assumed.
I don't know if a comment is the right way to make this more explicit but it would be nice to make it more obvious somehow

@LouisBrunner LouisBrunner changed the base branch from master to v/3 Aug 20, 2019

Edward White
Fixed renewal of tokens, as waiting for a 401 is too late.
Now create a daemon thread that keeps renewing the current session
@Charlesworth
Copy link
Member

left a comment

2 PR wide comments:

  • Is there any way we can test the EvaAuth threading, I'm always a bit worried about closing threads, in this case if you are using an Eva and destruct it. EvaLocker has some tests that should be convertable
  • You may want to update the README if its now out of date and perhaps example code if its broken? I haven't checked the examples
timeout=self.request_timeout,
)


# AUTH
def _auth_renew_session(self):

This comment has been minimized.

Copy link
@Charlesworth

Charlesworth Sep 3, 2019

Member

doesn't need to be a hidden ("_auth_renew_session" -> "auth_renew_session") route


self.__eva._auth_renew_session()


class EvaHTTPClient:

This comment has been minimized.

Copy link
@Charlesworth

Charlesworth Sep 3, 2019

Member

could you add all of the now public auth routes (all detailed in docs/api/auth)

@@ -10,37 +11,101 @@
# TODO lots of sleeps in control_* de to the robot state being updated slowly after starting an action, can this be improved?


class EvaAuth(requests.auth.AuthBase):

This comment has been minimized.

Copy link
@Charlesworth

Charlesworth Sep 3, 2019

Member

not seen AuthBase, nice usage of the request lib

return r.connection.send(retry_req, **kwargs)


def __create_session_token(self, r, **kwargs):

This comment has been minimized.

Copy link
@Charlesworth

Charlesworth Sep 3, 2019

Member

Can we add this to the HTTP client as a public method



def __create_session_token(self, r, **kwargs):
prep = requests.Request(

This comment has been minimized.

Copy link
@Charlesworth

Charlesworth Sep 3, 2019

Member

should prep be create_token_request

data=json.dumps({'token': self.__eva.api_token}),
).prepare()

create_r = r.connection.send(prep, **kwargs)

This comment has been minimized.

Copy link
@Charlesworth

Charlesworth Sep 3, 2019

Member

perhaps this should also be create_token_response

create_r = r.connection.send(prep, **kwargs)

if create_r.status_code != 200:
eva_error('auth request error', create_r)

This comment has been minimized.

Copy link
@Charlesworth

Charlesworth Sep 3, 2019

Member

use EvaAuthError

self.__renew_period_mins = renew_period_mins
self.__cond = threading.Condition()
self.__thread = threading.Thread(target=self.__renew_loop, daemon=True)
self.__thread.start()

This comment has been minimized.

Copy link
@Charlesworth

Charlesworth Sep 3, 2019

Member

The thread is never joined or stopped. You will want to stop the thread on destruction or some other point

Edward White
No longer using threading due to garbage collection issues.
Not using authbase as too much interdependence with EvaHTTPClient
@eddwhite

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

No changes required to README or examples, as all changes are internal.
No longer using threading as no way to make sure the objects are collected by the garbage collector.
Tests added, but require a connection to an Eva to run

@eddwhite eddwhite force-pushed the eddwhite:update_tokens branch from cc173b8 to 902341f Sep 10, 2019

@Charlesworth
Copy link
Member

left a comment

nearly there, the token/managed arguements on the auth methods for both Eva and the http client need to be worked out, either made clearer or removed


@pytest.fixture(scope="module")
def eva():
e = Eva('172.16.16.2', 'b2a54715-96cf-4248-bc8e-2580bb1a3e37', request_timeout=10)

This comment has been minimized.

Copy link
@Charlesworth

Charlesworth Sep 10, 2019

Member

can you parameterize the ip and token for the tests?

raise ValueError('Session must be renewed before expiring (30 minutes)')


def api_call(self, method, path, payload=None, creating_session=False):

This comment has been minimized.

Copy link
@Charlesworth

Charlesworth Sep 10, 2019

Member

What is the difference between api_call and api_request? I'd consider renaming these and adding comments as its not very clear without reading the code

This comment has been minimized.

Copy link
@Charlesworth

Charlesworth Sep 10, 2019

Member

The creating_session argument is a work around that the user should not ever need to see, is there another way?

headers = {
'Authorization': 'API {}'.format(self.token),
}
if renew_period > 30*60:

This comment has been minimized.

Copy link
@Charlesworth

Charlesworth Sep 10, 2019

Member

check for negative value here as well

return requests.request(
method, 'http://{}/api/v1/{}'.format(self.host_ip, path),
data=payload, headers=headers,
timeout=self.request_timeout,
)


# AUTH
def auth_renew_session(self, token=None):

This comment has been minimized.

Copy link
@Charlesworth

Charlesworth Sep 10, 2019

Member

Either you'll want to rename token here, not sure to what, possibly nonClientToken or externalToken, and/or add some comments as to what supplying the token does, i.e. in poor English: no token means a client renew and token renews the supplied token without changing or affecting the client token.

This comment has been minimized.

Copy link
@Charlesworth

Charlesworth Sep 10, 2019

Member

Another option is splitting this into two methods, a renew_self and renew_token? Same with the other session methods

return r.json()['token']


def auth_invalidate_session(self, token=None):

This comment has been minimized.

Copy link
@Charlesworth

Charlesworth Sep 10, 2019

Member

Same as with auth_renew_session, better "token" name and possible comment

self.auth_create_session()
return self.api_request(method, path, payload)

if time.time() - self.__last_renew > self.renew_period:

This comment has been minimized.

Copy link
@Charlesworth

Charlesworth Sep 10, 2019

Member

Not a huge problem but this will trigger if you are unsuccessfully creating a session on an old client


if time.time() - self.__last_renew > self.renew_period:
self.__logger.debug('Automatically renewing session')
self.auth_renew_session()

This comment has been minimized.

Copy link
@Charlesworth

Charlesworth Sep 10, 2019

Member

Interesting case, if the original request succeeds but the renewal doesn't we would raise an exception. This is valid in the case that the session reaches 48 hours old it will be unable to be renewed. We may want to catch this as the original request is valid and successful, the user should know it went through

@@ -42,9 +42,25 @@ def __exit__(self, type, value, traceback):


# --------------------------------------------- HTTP HANDLERS ---------------------------------------------
def api_call(self, method, path, payload=None):
def api_call(self, method, path, payload=None, creating_session=False):

This comment has been minimized.

Copy link
@Charlesworth

Charlesworth Sep 10, 2019

Member

I would err on the side of removing the creating_session arg here, it would mean that as it is a session creation would trigger twice on a 401 but it greatly simplifies this function definition. worth a chat



# Auth
def auth_renew_session(self, token=None):

This comment has been minimized.

Copy link
@Charlesworth

Charlesworth Sep 10, 2019

Member

again, worth a chat for whether we should have a set of renew/create/invalidate methods for both self and external token. either that or make it clearer about what happens when you feed in the token arg

Edward White
Changes based on PR comments.
Ability to manage your own session tokens removed for simplicity.
Renewing and creation loop errors removed
@Charlesworth
Copy link
Member

left a comment

Thanks Edd, lgtm

@Charlesworth Charlesworth merged commit 90bec43 into automata-tech:v/3 Sep 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.