Open nbarbettini opened 6 years ago
@nbarbettini just to clarify, are you expecting the developer to place this line in their application:
signIn.token.parseTokensFromUrl(OktaSignIn.DefaultTokenHandler);
? That is what I would like to see, because it's explicit.
The current PR #494 tries to do it automatically, such that doing this:
signIn.token.parseTokensFromUrl();
Has the magic side affect of running the default token handler. I don't think this magic is clear and should be avoided.
@robertjd - Can you tell me more about the benefit of binding this to the root object as opposed to keeping it internal?
I'd imagine that this method would only be used once, so binding it to the OktaSignIn
object seems a bit odd to me.
With many of the changes coming into the Widget, it seems like we're moving into a world where we're handling many of the well-known use cases for developers. If i'm understanding correctly, the goal of this type of change is to cleanup the code required for basic OIDC use:
var signIn = new OktaSignIn({ /* config */ });
if (!signIn.token.hasTokensInUrl()) {
signIn.renderEl({el: '#widget-container'});
} else {
signIn.token.parseTokensFromUrl(
function success(res) {
// Add the token to tokenManager to automatically renew the token when needed
signIn.tokenManager.add('id_token', res[0]);
signIn.tokenManager.add('access_token', res[1]);
},
function error(err) {
console.log('handle error', err);
}
);
}
// Becomes
var signIn = new OktaSignIn({ /* config */ });
if (!signIn.token.hasTokensInUrl()) {
signIn.renderEl({el: '#widget-container'});
} else {
signIn.token.parseTokensFromUrl();
}
The concern is about making it clear what's going on, the method name of parseTokensFromUrl
doesn't sound like a method that would also be magically store tokens for me. That's why signIn.token.parseTokensFromUrl(OktaSignIn.DefaultTokenHandler);
is clearer: you're passing the output of one function into another.
An alternative is a new method that does both, something like signin.parseAndStoreTokens()
@jmelberg-okta You're right, the goal is indeed to clean up the code for the basic use case 🙂 I agree with @robertjd that we should move towards less magic, more explicit. My thought was that parseTokensFromUrl
already takes a handler, so we can provide a default handler (that the developer explicitly opts in to) without having to change any of the existing interface or behavior of parseTokensFromUrl
.
Adding DefaultTokenHandler
as a public member of OktaSignIn.prototype
is only one way to achieve that. It might be a pattern more common in languages other than JS. I like the pattern, but I'm not tied to it. The important thing here is that there is an easy way to explicitly provide a default behavior to parseTokensFromUrl
in ~one line, and it's also nice to wrap up that behavior in a way that can be reused down the line.
Background
When the Widget is used to sign in users to a frontend application, tokens are returned from Okta in the fragment and this code is used to grab them:
This code rarely needs to change. To reduce the code a developer needs to write (and reduce the chance of copypasta errors), I propose adding a built-in handler.
DefaultTokenHandler
Define
OktaSignIn.prototype.DefaultTokenHandler()
with this behavior:tokenManager.add('access_token')
tokenManager.add('id_token')
Changes to
parseTokensFromUrl
If the second parameter of the function (
error
) is omitted, use the default error handler added in #466 (logging to the console).Example