On each execute_get call, check age of auth token, reinitialize HubRestApi object if >= 1.5 hours. #118
Conversation
If a HubRestApi object is used by a client script in the examples directory to create a custom report for a large set of components, the report could run longer than 2h using the same token. Anything requested after 2h will get a 401 response from Black Duck Hub. This enhancement will solve that potential problem since there is no way around it on the Black Duck Hub side. On each execute_get call, check to see if the current HubRestApi object has existed for less than 1.5h, if older than 1.5h, reinitialize it with a new bearer token.
| def __init__(self, *args, **kwargs): | ||
| # Config needs to be an instance variable for thread-safety, concurrent use of HubInstance() | ||
| self.config = {} | ||
|
|
||
| self.read_config() |
There was a problem hiding this comment.
why skip the try/except block?
There was a problem hiding this comment.
I think that try/except block is checking if the .restconfig exists, if it doesn't check the args and write it.
There was a problem hiding this comment.
I added a keyword argument named refresh_token that the client script can pass on instantiation. Refreshing the token is now off by default.
| @@ -123,6 +128,9 @@ def __init__(self, *args, **kwargs): | |||
|
|
|||
| def read_config(self): | |||
| try: | |||
| #always return to the examples directory to read the config | |||
| if not os.getcwd().endswith("examples"): | |||
| os.chdir(root_dir) | |||
There was a problem hiding this comment.
could this potentially create a problem if you were calling this from another application that uses the blackduck library, rather than from the examples directory?
There was a problem hiding this comment.
Hi, yes, this is a potential problem that I've been hoping to make progress on before merging. The root of the problem is that .restconfig is located in examples on the first HubRestApi object init. When a refresh actually does occur, .restconfig no longer exists in context at the time HubRestApi reinitializes itself, so I reset back to examples to ensure that .restconfig is available. There has to be a better approach, it's just not clear to me. Any ideas?
There was a problem hiding this comment.
I believe the .restonfig.json is written to (or read from) the current working directory. It is not hard-coded to anything. So, if I invoke a program in /Users/gsnyder/Projects/sage for instance, the .restonfig.json needs to be in the /Users/gsnyder/Projects/sage directory for it to be read. Or, if I was initializing the HubInstance using parameters, then the .restconfig.json would be written to that directory.
Also, the config is saved in memory to self.config, so do we really need to read anything again?
There was a problem hiding this comment.
I removed the hardcoded part. Now, it works under the assumption that .restconfig.json is in the current working directory.
blackduck/HubRestApi.py
Outdated
| length = len(response.text) | ||
| new_token = response.text | ||
| token_tail = new_token[length - 50: length] | ||
| print ("New token requested, using restconfig api token: {}".format(token_tail)) |
There was a problem hiding this comment.
We probably shouldn't be printing out the user's api token, same thing for the password below
There was a problem hiding this comment.
Agreed, I'll go ahead and remove them. My wording is not clear. The actual text is from the bearer token which is available from the response. Not the private Black Duck api token from rest config.
There was a problem hiding this comment.
And if you are going to be printing the token, don't do so using print - make it a call to logger.debug(...) or logger.info(...), so it may at least be disabled by tweaking the log level.
…ord argument for the constructor and leave it off by default.
|
Hi @jackeekaplan, with the most recent changes added to the branch, is it ok to merge? Or would you want to take a second look? I am certainly looking for feedback. Thanks! |
| @@ -231,7 +248,26 @@ def get_limit_paramstring(self, limit): | |||
|
|
|||
| def get_apibase(self): | |||
| return self.config['baseurl'] + "/api" | |||
|
|
|||
|
|
|||
| def reauthenticate(self, *args, **kwargs): | |||
There was a problem hiding this comment.
Sorry, this really isn't a good idea. It would be far better to rerun auth rather than explicitly reinitialising the class.
There was a problem hiding this comment.
I tried rerunning the call to authenticate in my first attempt. I couldn't get it to work, I'll revisit.
For my own understanding, what is the concern about reinitializing the class upon token expiration?
Thanks for taking a look!
There was a problem hiding this comment.
It may have unexpected side effects down the line. Generally speaking in python the only time you should need to manually call __init__(...) is when initializing a parent class i.e. super().__init__(). I wouldn't enforce this sort of change at the library level.
blackduck/HubRestApi.py
Outdated
| length = len(response.text) | ||
| new_token = response.text | ||
| token_tail = new_token[length - 50: length] | ||
| print ("New token requested, using restconfig api token: {}".format(token_tail)) |
There was a problem hiding this comment.
And if you are going to be printing the token, don't do so using print - make it a call to logger.debug(...) or logger.info(...), so it may at least be disabled by tweaking the log level.
Currently, the rest api bearer token is only valid for 2h.
If a HubRestApi object is used by a client script in the examples directory to create a custom report for a large set of components, the report could run longer than 2h using the same token.
Anything requested after 2h will get a 401 response from Black Duck Hub.
This enhancement will solve that potential problem since there is no way around it on the Black Duck Hub side.
On each execute_get call, check to see if the current HubRestApi object has existed for less than 1.5h, if older than 1.5h, reinitialize it with a new bearer token.