-
Notifications
You must be signed in to change notification settings - Fork 12
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
Access Token authentication #11
base: main
Are you sure you want to change the base?
Conversation
Access Token authentication was added. -a or QTOKEN can be used for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update! This is long overdue, it's great to see.
if ww.creds["QTOKEN"]: | ||
ww.rc = RestClient(random.choice(ww.ips), ww.ww.creds.get("QPORT", REST_PORT), Credentials(ww.creds["QTOKEN"])) | ||
else: | ||
ww.rc.login(ww.creds["QUSER"], ww.creds["QPASS"]) | ||
# log_it("re-initialized Qumulo rest client for worker") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this a function or method so it doesn't need to be copy-pasted
if self.creds["QTOKEN"]: | ||
rc = RestClient( | ||
self.creds["QHOST"], self.creds.get("QPORT", REST_PORT), Credentials(self.creds["QTOKEN"]) | ||
) | ||
else: | ||
rc = RestClient(self.creds["QHOST"], self.creds.get("QPORT", REST_PORT)) | ||
rc.login(self.creds["QUSER"], self.creds["QPASS"]) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this could be a function too; accept the creds object and produce a RestClient
parser.add_argument( | ||
"-p", help="Qumulo API password", default=os.getenv("QPASS") or "admin" | ||
"-p", help="Qumulo API password", default=os.getenv("QPASS") or "'Admin123'" | ||
) | ||
parser.add_argument( | ||
"-a", help="Qumulo access token", default=os.getenv("QTOKEN") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make these mutually exclusive (https://docs.python.org/3/library/argparse.html#mutual-exclusion) since you have the RestClient init prefer the token over the password. Which is reasonable, but I could see confusion if someone gives the script a valid password + invalid token, and it falls to connect
Better to make that situation impossible
if ww.creds["QTOKEN"]: | ||
ww.rc = RestClient(random.choice(ww.ips), ww.ww.creds.get("QPORT", REST_PORT), Credentials(ww.creds["QTOKEN"])) | ||
else: | ||
ww.rc.login(ww.creds["QUSER"], ww.creds["QPASS"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be solved when moving to a function, but let's make sure that none of the lines are longer than 100 characters
Access Token authentication was added. -a or QTOKEN can be used for that.