Skip to content
This repository has been archived by the owner on Apr 10, 2024. It is now read-only.

Implement authentication for usernames/passwords using adal #3

Closed
wants to merge 7 commits into from

Conversation

brettcannon
Copy link
Member

No description provided.

def __init__(self, username, password, **kwargs):
super(AdalUserPassCredentials, self).__init__(**kwargs)
self.username = username
self.password = password
Copy link
Member

Choose a reason for hiding this comment

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

This implies that credentials are not tested at the instance creation, but at the instance usage. This is different from current UserPassCredentials behavior.
I think we might want to keep the current behavior, but I'm open to discussion (@annatisch)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.


"""Base class for adal-derived authentication."""

def __init__(self, client_id="04b07795-8ddb-461a-bbee-02f9e1bf7b46",
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to have this id in a constant XPLAT_APP_ID since it's not recommend anymore to use it:
https://github.com/AzureAD/azure-activedirectory-library-for-python#about-client_id-and-resource-arguments

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@annatisch
Copy link
Member

Hi @brettcannon, @lmazuel

The changes look good.
My only concern is that there's currently no support for refresh tokens.
Maybe while we're still supporting the old classes and this being an interim solution it might not be such a big deal.
Once we do a larger scale merging of the ADAL vNext lib into the pipeline this will likely go away.

In order to integrate the refresh token functionality we would need to use an oauth-requestslib Session as opposed to a standard requests Session.

Alternatively, maybe ADAL has refresh token handling built in - and we can access it be doing a token acquisition at the time of creating the Session? I'm not sure - I'll take a look at ADAL.

@lmazuel
Copy link
Member

lmazuel commented Nov 30, 2016

Close since integrated in #8
@brettcannon, feel free to talk if I should have not closed this one :)

@lmazuel lmazuel closed this Nov 30, 2016
@brettcannon
Copy link
Member Author

Nope, closing was the right thing to do.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants