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

Handle authentication token #42

Open
avranju opened this issue Oct 30, 2014 · 6 comments
Open

Handle authentication token #42

avranju opened this issue Oct 30, 2014 · 6 comments
Assignees

Comments

@avranju
Copy link
Contributor

avranju commented Oct 30, 2014

Currently the API does not handle the case when a call fails as a result of an expired access token. The error is passed on to the caller as is. It might be nicer if the client objects provided a way for the caller to supply a callback that is invoked automatically whenever the API needs a fresh access token and also automatically retry requests on the caller's behalf. The .NET AD Graph client for instance let's you do the following:

var adclient = new ActiveDirectoryClient(new Uri("https://login.windows.net/common/"), async () =>
{
    var context = new AuthenticationContext("https://login.windows.net/common/");
    var authParams = new AuthorizationParameters(PromptBehavior.Auto, this);
    var result = await context.AcquireTokenAsync("https://graph.windows.net/",
          "<<CLIENT_ID>>",
          new Uri("<<REDIRECT_URI"),
          authParams);
    return result.AccessToken;
});

var apps = await adclient.Applications.ExecuteAsync();

The idea is to have the API invoke the callback supplied by the client whenever it needs a new access token. To get an idea of how much work this is otherwise, follow the code path from here.

@marcote
Copy link
Contributor

marcote commented Oct 31, 2014

@avranju . I like what you propose, not sure how this will play with our intent of doing our constructor as generic as possible. Remember we're relying on another libraries (ADAL) the OAuth authentication workflow.
I will discuss with the team and keep you posted. This is great feedback, thank you.

@avranju
Copy link
Contributor Author

avranju commented Nov 1, 2014

I see what you mean. Perhaps another approach could be to extend our Credentials interface to include a method that will be invoked whenever the API encounters an Authentication_ExpiredToken error.

public interface Credentials {
    void prepareRequest(Request request);
    void refreshCredentials();
}

The idea is for the relevant API to retry the request a second time after it has invoked refreshCredentials and error out only if the second attempt fails as well. But this interface change is of course a breaking change. Not sure if it makes sense to create a descendant interface with just the refreshCredentials method.

@marcote
Copy link
Contributor

marcote commented Nov 6, 2014

@avranju I will contact the ADAL team on this matter to see what we can do.

@joshgav
Copy link
Contributor

joshgav commented Nov 11, 2014

We are looking into dynamically calling AuthenticationContext.acquireToken() or .acquireTokenSilent() for each prepareRequest() invocation. ADAL has been written to automatically try local cache, then refresh token, and finally a new authentication web view, which could use a cookie or actually prompt the user. (NB: Silent says to throw an error if web view is required.)

@avranju
Copy link
Contributor Author

avranju commented Nov 11, 2014

Right now, it is the responsibility of the object implementing the Credentials interface to perform the authentication and provide the required HTTP header for the request. Is the idea to change the API to have the API do the authentication on the caller's behalf?

-----Original Message-----
From: "Josh Gavant" [email protected]
Sent: ‎11/‎11/‎2014 9:27 AM
To: "OfficeDev/Office-365-SDK-for-Android" [email protected]
Cc: "Rajasekharan Vengalil" [email protected]
Subject: Re: [Office-365-SDK-for-Android] Handle authentication token (#42)

We are looking into dynamically calling AuthenticationContext.acquireToken() or .acquireTokenSilent() for each prepareRequest() invocation. ADAL has been written to automatically try local cache, then refresh token, and finally a new authentication web view, which could use a cookie or actually prompt the user. (NB: Silent says to throw an error if web view is required.)

Reply to this email directly or view it on GitHub.=

@joshgav
Copy link
Contributor

joshgav commented Nov 11, 2014

No, this change would be integrated into an implementation of the Credentials interface, e.g. AADCredentials. We'll likely include the implementation in the DefaultDependencyResolver.

@joshgav joshgav modified the milestones: v0.10.0, Immediate Nov 11, 2014
@joshgav joshgav removed this from the Immediate milestone May 19, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants