-
Notifications
You must be signed in to change notification settings - Fork 15
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
Device flow #17
base: main
Are you sure you want to change the base?
Device flow #17
Conversation
Sorry, I'm not following the point of this PR? |
It's for issue 14. The changes made to simple_ado allow for the use of device flow authentication (or more generically bearer auth), and a sample / test of using it end to end. |
auth_helper.py
Outdated
|
||
import json5 | ||
import sys | ||
import requests |
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 is unused.
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.
Will address
@@ -0,0 +1,68 @@ | |||
import os |
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 file should be in the simple_ado
package. It should probably be something like device_flow_helper
to be as explicit as possible.
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.
I'm fine with that change. Would we consider leaving it as auth_helper as it could potentially be the base for other authentication flows as well?
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.
I don't hugely mind the name, but I don't think it should be exposed to the users either way. I'd rather see two constructors for ADOClient, one which takes a PAT, one which takes a Tenant ID, App ID, etc. and calls into this class.
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.
Can you take the pull request as is and then I can look to make that change for the future?
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.
I don't think that's the right move. Temporary fixes like this shouldn't happen. If we merge and someone ends up depending on it, then fixing it would break things for them. We should do it right from the start.
auth_helper.py
Outdated
import msal | ||
|
||
class AuthHelper: | ||
tenantId = os.environ["tenant_id"] |
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 is setting class variables, not instance variables. These should also be taken in the constructor.
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.
Resolved.
|
||
def main(): | ||
logger = logging.getLogger("test") | ||
project_id = os.environ["SIMPLE_ADO_PROJECT_ID"] |
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.
Where are these coming from?
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.
Do you mean where are the os environment variables coming from? I'm not following.
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.
Correct. I'd like to see a test that's reproducible.
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.
Hmm... ok, so the challenge is I'm not sure what I can share there. All of my tests are using values I've setup for internal usage at Microsoft. Getting this to work for project/repo/org etc. should be easy enough, but the authentication layer requires appId and a variety of other values that I'm not sure can be publicly provided. Let me look into that... are you by chance Microsoft-based? We could discuss this in a teams chat and I could show you there. Then we can figure out how we want to address it for the public.
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.
Ok, I think I can do this, based on conversations I've had internally, nothing of these are considered secrets. If you can approve PR, I can re-pull, move this all to a test case, and add it there.
# callback=handle_progress | ||
#zip = git_client.download_zip(output_path=output, repository_id=repo["id"], branch=branch, project_id=project_id,callback=callback) | ||
zip = git_client.download_zip(output_path=output, repository_id=repo["id"], branch=branch, project_id=project_id) | ||
except ADOHTTPException as e: |
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.
In a test you want to re-throw these exceptions (or don't even catch them)
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.
Will address.
device_flow_test.py
Outdated
repo = git_client.get_repository(repository_id=repo_id, project_id=project_id) | ||
branch = repo["defaultBranch"] | ||
branch = branch.split("/")[-1] | ||
#callback=None |
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.
Remove unused code.
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.
Resolved.
token = ah.adoAuthenticate() | ||
|
||
print("** Setting up ADOHTTPClient with " + tenant) | ||
http = simple_ado.http_client.ADOHTTPClient(tenant=tenant, |
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.
We shouldn't be constructing anything other than ADOClient
for public use. It's fine for tests, but we need to be able to ensure it works in real world situations.
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.
I can commit to working on that in next iteration if that's ok... I can move this whole thing to a test case in your tests folder and address it there.
@@ -21,22 +21,22 @@ | |||
class ADOBranchPermission(enum.IntEnum): |
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.
These changes should be undone.
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.
Believe these came from running the style checks.
if "access_token" in result: | ||
print("Found access token.") | ||
return result | ||
else: |
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.
The if
returns a result, so you don't need the else. You can just straight up raise the AuthError
print("** Setting up ADOHTTPClient with " + tenant) | ||
http = simple_ado.http_client.ADOHTTPClient(tenant=tenant, | ||
credentials=token, | ||
user_agent="test", |
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.
Something like simple_ado/tests
would be fine.
Attempts to resolve issue opened to support device flow authentication path.