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.

Sign up for GitHub

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

On each execute_get call, check age of auth token, reinitialize HubRestApi object if >= 1.5 hours. #118

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

On each execute_get call, check age of auth token, reinitialize HubRestApi object if >= 1.5 hours. #118

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
92a57ca
Currently, the rest api bearer token is only valid for 2h.
evansa-synopsys Nov 6, 2020
ea0a704
Merge branch 'master' into reauthenticate-with-black-duck-before-toke…
evansa-synopsys Nov 20, 2020
03ead3e
Remove prints of token refresh information. Make refresh_token a keyw…
evansa-synopsys Nov 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 53 additions & 18 deletions blackduck/HubRestApi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@

'''
import logging
import os
import time
import requests
import json
from operator import itemgetter
Expand Down Expand Up @@ -86,26 +88,32 @@ class HubInstance(object):

# TODO: What to do about the config file for thread-safety, concurrency
configfile = ".restconfig.json"

global root_dir
# For refresh to find the .restconfig.json file upon reinitialization,
# client script must be executed in the same directory
root_dir = os.getcwd()
refresh_token = False
def __init__(self, *args, **kwargs):
# Config needs to be an instance variable for thread-safety, concurrent use of HubInstance()
self.config = {}

try:
self.config['baseurl'] = args[0]
api_token = kwargs.get('api_token', False)
if api_token:
self.config['api_token'] = api_token
else:
self.config['username'] = args[1]
self.config['password'] = args[2]
self.config['insecure'] = kwargs.get('insecure', False)
self.config['debug'] = kwargs.get('debug', False)

if kwargs.get('write_config_flag', True):
self.write_config()
except Exception:
self.read_config()
self.read_config()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why skip the try/except block?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that try/except block is checking if the .restconfig exists, if it doesn't check the args and write it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a keyword argument named refresh_token that the client script can pass on instantiation. Refreshing the token is now off by default.

self.refresh_token = kwargs.get('refresh_token')
if not kwargs.get('refresh_token', False):
try:
self.config['baseurl'] = args[0]
api_token = kwargs.get('api_token', False)
if api_token:
self.config['api_token'] = api_token
else:
self.config['username'] = args[1]
self.config['password'] = args[2]
self.config['insecure'] = kwargs.get('insecure', False)
self.config['debug'] = kwargs.get('debug', False)

if kwargs.get('write_config_flag', True):
self.write_config()
except Exception:
self.read_config()

if self.config['insecure']:
requests.packages.urllib3.disable_warnings()
Expand All @@ -123,6 +131,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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the hardcoded part. Now, it works under the assumption that .restconfig.json is in the current working directory.

with open('.restconfig.json','r') as f:
self.config = json.load(f)
except:
Expand Down Expand Up @@ -151,6 +162,8 @@ def get_auth_token(self):
except json.decoder.JSONDecodeError as e:
logger.exception("Authentication failure, could not obtain bearer token")
raise Exception("Failed to obtain bearer token, check for valid authentication token")
# set token expiration to 1.5h
self.access_token_expiration = time.time() + 5200
return (bearer_token, csrf_token, None)
else:
authendpoint="/j_spring_security_check"
Expand All @@ -162,6 +175,8 @@ def get_auth_token(self):
response = session.post(url, credentials, verify= not self.config['insecure'])
cookie = response.headers['Set-Cookie']
token = cookie[cookie.index('=')+1:cookie.index(';')]
# set token expiration to 1.5h
self.access_token_expiration = time.time() + 5200
return (token, None, cookie)

def _get_hub_rest_api_version_info(self):
Expand Down Expand Up @@ -231,7 +246,27 @@ def get_limit_paramstring(self, limit):

def get_apibase(self):
return self.config['baseurl'] + "/api"


def reauthenticate(self, *args, **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this really isn't a good idea. It would be far better to rerun auth rather than explicitly reinitialising the class.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator

@OffBy0x01 OffBy0x01 Jan 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

try:
kwargs.update({"refresh_token":self.refresh_token})
self.__init__(self, *args, **kwargs)
except Exception as e:
print("There was an error when refreshing the token: {}".format(e))
return None
else:
return self

class Decorators():
@staticmethod
def refresh_token(decorated):
def wrapper(hub_api_object, *args, **kwargs):
if time.time() > hub_api_object.access_token_expiration and hub_api_object.refresh_token:
hub_api_object.reauthenticate(hub_api_object, *args, **kwargs)
return decorated(hub_api_object, *args, **kwargs)

return wrapper

###
#
# Role stuff
Expand Down