iTwin / imodels-clients

Monorepo for iModels API clients
MIT License
6 stars 1 forks source link

AuthorizationCallback is wrong format #57

Closed tcobbs-bentley closed 2 years ago

tcobbs-bentley commented 2 years ago

@austeja-bentley @calebmshafer

The AuthorizationCallback should probably be renamed to AccessTokenCallback, and return a Promise that contains the token in the same format as IModelApp.getAccessToken(). The rest of the iTwin API has changed over to this string-only format for token handling, instead of authorization.

Right now, in order to make this work with IModelApp.getAccessToken(), a user has to strip the "Bearer " prefix off of the accessToken value, then build an Authorization object out of the resulting string and "Bearer".

austeja-bentley commented 2 years ago

@tcobbs-bentley

The name AuthorizationCallback was chosen deliberately as iModels API can and will eventually support more ways to authenticate than with a Bearer access token. This client will also have to work with iTwin Stack so the structured callback result allows for better extensibility. The @itwin/imodels-client-management and @itwin/imodels-client-authoring packages are of course a part of the platform but they are intended to be used in more places than applications built using iTwin.js libraries so their interfaces were designed having that in mind.

What applications can do is build an adapter to their dependencies. The @itwin/imodels-access-frontend and @itwin/imodels-access-backend act as exactly that - adapters that bridge the iModels API client interface and the iTwin.js core libraries and they do indeed have that bit of code that parses the access token into scheme and token. I think it is pretty common to have dependencies that have an interface that does not match your needs exactly but as long as conforming to them does not cause too much trouble I feel like it's fine.

Does your application use the IModelApp class and also iModels API client directly (not via the @itwin/imodels-access-frontend and @itwin/imodels-access-backend packages)?

tcobbs-bentley commented 2 years ago

@austeja-bentley My app is actually a mobile sample for users to look at to see how to use iTwin on a mobile device (using mobile-specific packages to make that work). And yes, it uses IModelApp. Given that @itwin/imodels-client-management will likely be used a lot by people who are also using IModelApp, it seems to me that it should make it easier to make use of the token string returned by IModelApp.getAccessToken(). Right now, in order to use it, it has to strip off the "Bearer " prefix that is part of that string.

@calebmshafer What are your thoughts on this?

calebmshafer commented 2 years ago

I tend to agree with @tcobbs-bentley, as I just hit the same thing he did when updating the itwinjs-core repo to use the new clients.

The motivation behind moving towards a single string in the iTwin.js core code is to be agnostic to the type of auth that people would like to use and support. Most common workflows (OAuth 2.0, Basic and API key) can all be handled via a single string to interpret them, helping to reduce complication on the consumer of the API. It would be up to that consumer to use the appropriate implementation of the AuthorizationClient interface (for IModelApp/IModelHost) that supports the workflow needed, as @tcobbs-bentley mentions that interface only has a single method that returns only a string.

Now, I understand that the imodels client libraries has different requirements since it has to actually construct the requests, either by adding it to the auth header or as a query param in the case of API key. However, in all cases I think it makes sense for the @itwin/imodels-client-management to make it as simple as it is in the Frontend/Backend Hub Access clients are for the main cases we want to support.

Does that sound like it would work? @austeja-bentley @Animagne @robertas-kerpys

austeja-bentley commented 2 years ago

@tcobbs-bentley @calebmshafer This PR was merged to help applications that rely on IModelHost/IModelApp to communicate with the plain IModelsClient. With the conversion functions exposed calls to the plain client would look something like this:

const iModelsClient: IModelsClient = new IModelsClient();
const accessToken: string = await IModelHost.getAccessToken();
const briefcase: Briefcase = await iModelsClient.briefcases.acquire({
  authorization: AccessTokenAdapter.toAuthorizationCallback(accessToken),
  iModelId: "some id"
});

With this solution client applications do not have to implement the conversion functions themselves and IModelsClient interfaces remain extensible.

Please let me know if there are any concerns regarding this.