-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Question: Authentication flow #14
Comments
First, I dont recommend calling service directly within sagas, it'd be better to use declarative calls. It makes possible testing all the operational logic inside the generator as explained in the declarative effects section So I'll first refactor localStorage calls into some isolated service // Side effects Services
function getAuthToken() {
return JSON.parse(localStorage.getItem('authToken'))
}
function setAuthToken(token) {
localStorage.setItem('authToken', JSON.stringify(token))
}
function removeAuthToken() {
localStorage.removeItem('authToken')
} Another remark is regarding action watching flow. It'd be easier if you exploit the Structured programming benefits offered by generators. I'll illustrate with a simple example, suppose our flow is just this simple sequence
Instead of doing something like while(true) {
const action = take([SIGN_IN, SIGN_OUT])
if(action.type === SIGN_IN)
const token = yield call(authorize)
yield call(setAuthToken) // save to local storage
yield put(authSuccess, token)
else
yield call(removeAuthToken)
yield put(signout)
}
} You can exploit the fact that SIGN_IN and SIGN_OUT fire always in sequence and never concurrently and offload the flow control burden (where are we in the program right now ?) to the underlying generator runtime (I simplify to illustrate the concept, I ll introduce concurrency next) function* authFlowSaga() {
while(true) {
// first expect a SIGN_IN
const {credentials} = yield take(SIGN_IN)
const token = yield call(authorize, credentials)
// followed by a SIGN_OUT
yield take(SIGN_OUT)
yield call(signout)
}
}
// reusable subroutines. Avoid duplicating code inside the main Saga
function* authorize(credentialsOrToken) {
// call the remote authorization service
const token = yield call(authService, credentialsOrToken)
yield call(setAuthToken, token) // save to local storage
yield put(authSuccess, token) // notify the store
return token
}
function* signout(error) {
yield call(removeAuthToken) // remove the token from localStorage
yield put( actions.signout(error) ) // notify th store
} Before introducing concurrency, let's first introduce the refresh cycles, when the token expires w'll send again a request to the server to get a new token. so our sequence will become now
REFRESH* means many refreshes i.e. a loop in the generator We're still forgetting concurrency to keep things simple and progress step by step function* authFlowSaga() {
while(true) {
const {credentials} = yield take(SIGN_IN)
let token = yield call(authorize, credentials)
// refresh authorization tokens on expiration
while(true) {
yield wait(token.expires_in)
token = call(authorize, token)
}
yield take(SIGN_OUT)
yield call(signout)
}
} But we've an issue there, the refresh loop executes forever, because there is no breaking condition. If the user signed out between 2 refreshes we've to break the loop. So the breaking condition is the SIGN_OUT action. Also the SIGN_OUT action is concurrent to the next expiration delay, So we have to introduce a function* authFlowSaga() {
while(true) {
const {credentials} = yield take(SIGN_IN)
let token = yield call(authorize, credentials)
let userSignedOut
while( !userSignedOut ) {
const {expired} = yield race({
expired : wait(token.expires_in),
signout : take(SIGN_OUT)
})
// token expired first
if(expired)
token = yield call(authorize, token)
// user signed out before token expiration
else {
userSignedOut = true // breaks the loop and wait for SIGN_IN again
yield call(signout)
}
}
}
} But there are 2 othe issues, first the So first, we've to refactor our function* authorize(credentialsOrToken) {
const {response} = yield race({
response: call(authService, credentialsOrToken),
signout : take(SIGN_OUT)
})
// server responded (with Success) before user signed out
if(response && response.token) {
yield call(setAuthToken, response.token) // save to local storage
yield put(authSuccess, response.token)
return response.token
}
// user signed out before server response OR server responded first but with error
else {
yield call(signout, response ? response.error : 'User signed out')
return null
}
} Now if a remote authorization fails, we signout the user and return null as token. So we've also to refactor our main Saga to take into account the failure (null return value) function* authFlowSaga() {
while(true) {
const {credentials} = yield take(SIGN_IN)
let token = yield call(authorize, credentials)
// authorization failed, wait the next signing
if(!token)
continue
let userSignedOut
while( !userSignedOut ) {
const {expired} = yield race({
expired : wait(token.expires_in),
signout : take(SIGN_OUT)
})
// token expired first
if(expired) {
token = yield call(authorize, token)
// authorization failed, either by the server or the user signout
if(!token) {
userSignedOut = true // breaks the loop
yield call(signout)
}
}
// user signed out before token expiration
else {
userSignedOut = true // breaks the loop
yield call(signout)
}
}
}
} Now, we can think of the left requirement. What if there is a token already in local storage ? We'll simply skip the function* authFlowSaga() {
let token = yield call(getAuthToken) // retreive from local storage
token.expires_in = 0
while(true) {
if(!token) {
let {credentials} = yield take(SIGN_IN)
token = yield call(authorize, credentials)
}
// ... rest pf code unchanged
} So IMO we should follow as much as we can the following
As I said, the main benefit of using Generators is that it allows to leverage the power of Structured Programming and routine/subroutine approach. do you think that humans could write such complex programs using only goto jumps ? |
Wow, thank you for detailed answer, it was extremely helpful! One thing that still bothers me a bit is that we need to take |
@yelouafi what an answer 😄 this could make a good blog (medium) post I think to show the strength of Sagas |
Further development strengthened my concern: function* authorize() {
let token;
while (true) {
token = yield call(authService, token)
yield call(setAuthToken, token)
yield put(authSuccess, token)
yield call(delay, token.expires_in);
}
}
function* authFlowSaga() {
while (true) {
yield take(SIGN_IN);
const authLoopTask = yield fork(authorize);
yield take(SIGN_OUT);
authLoopTask.cancel();
}
} |
@yelouafi that's a nice example :) You should put it in a readme maybe and create an example with a fake auth service that failes 20% of the time :) I think you do not handle request failures and bad credentials cases on initial authentication. |
Yup. Fixed, thanks |
cancellation is on my todo list. And what you said makes quite sens. But We've to consider this: We can't allow arbitrary task cancellation, something ala linux kill process. Consider for example this simple subtask function* fetchPosts() {
yield put( startFetchRequest() )
const posts = yield call(fetchApi, '/posts')
yield put( fethRequestSuccess(posts) )
} What happens if you cancel this task while it's still waiting for the api call to resolve ? Imagine if your reducer sets some So task cancellation, if implemented, should give a chance to the cancelled task to do its cleanup to let the store in a consistent state. The best I can think of actually is similar to Threads behavior on Java. If a task is cancelled, we throw a special exception on it to give it chance to handle its cancellation in a proper way function* fetchPosts() {
try {
yield put( startFetchRequest() )
const posts = yield call(fetchApi, '/posts')
yield put( fethRequestSuccess(posts) )
} catch(error) {
if(error instanceof SagaCancellationError)
yield put( fetchRequestFailure(error) )
}
} Cancellation may be useful, but we need clear semantics for it. I m interested in any ideas. |
Yes, throwing special error was what I thought about. I can work on PR with |
@aikoven you may take a looke at task-cancel branch its incomplete (use a generic error) but it seems to work (Need more tests though). |
@aikoven Of course you are welcome to provide a PR with this feature |
I checked
|
Submitted PR #17 |
Complete flow with cancelation: function* authorize(refresh) {
try {
const token = yield call(auth.authorize, refresh);
yield call(auth.storeToken, token);
yield put(authorizeSuccess(token));
return token;
} catch (e) {
yield call(auth.storeToken, null);
yield put(authorizeFailure(e));
return null;
}
}
function* authorizeLoop(token) {
try {
while (true) {
const refresh = token != null;
token = yield call(authorize, refresh);
if (token == null)
return;
yield call(delay, token.expires_in);
}
} catch (e) {
if (e instanceof InterruptedError)
return;
throw e;
}
}
function* authentication() {
const storedToken = yield call(auth.getStoredToken);
while (true) {
if (!storedToken)
yield take(SIGN_IN);
const authLoopTask = yield fork(authorizeLoop, storedToken);
const {signOutAction} = yield race({
signOutAction: take(SIGN_OUT),
authLoop: join(authLoopTask)
});
if (signOutAction) {
authLoopTask.cancel(new InterruptedError(SIGN_OUT));
yield call(auth.storeToken, null);
}
}
} |
I am loving where this all is heading! For me, constantly "racing" the SIGN_OUT take is problematic for the simple reason that it is shortsighted - perhaps there will be some other action should cancel this in the future, and then you have to add an item to each of the races. Having forked "processes" be generally cancellable is the only way to go long-term IMO. I see the advantages of using the exception system for this: it does the 'correct' thing and immediately stops what is going on, and allows you to handle the cancellation in any way you see fit. However, it does seem like an abuse of the exception system - someone "canceling" a behavior is not exceptional behavior at all, and I shy away from using exceptions for non-exceptional behavior for semantic reasons. The obvious (though admittedly horrible) alternative is a more "opt-in" system where you check against some state variable whether or not the current thread has been canceled ( |
AFAIK The behavior of |
Oh, I am sure that it currently is, and that it probably should stay that way. Adding another important keyword to the mix is likely to do more harm than good with this framework! The |
@dts Could you please add more detail on your point against exceptions? |
We're arguing about a very small point here, so I would like to say that the exception mechanism is the best I've seen - I am merely trying to point out what I see as a weakness that we may be able to improve. With that out of the way, I would argue that any event that is triggered by a user action is inherently "non-exceptional". As for the mechanics, the default behavior of the exception mechanism is to drop everything semi-immediately, cleanup must be explicit with a try/catch/finally syntax. If there are asynchronous tasks that need to be performed in order to safely cancel something, that's additionally complicated (I assume it's possible, though). |
There may be cases where cancellation would be useful, but I don't think it is needed here. Sign-in and sign-out are triggered by real world events. Consider a UI containing Sign In and Sign Out buttons. Only one of these buttons would be enabled at any time. Sure, refresh could be in progress when sign_out occurs, but each refresh attempt could be time limited, so the delay to sign out would be un-noticeable. As @yelouafi said, "progressively introduce more requirements". I think this is a step too far. |
With automatic cancellation of function* authorize(refresh) {
try {
const token = yield call(auth.authorize, refresh);
yield call(auth.storeToken, token);
yield put(authorizeSuccess(token));
return token;
} catch (e) {
yield call(auth.storeToken, null);
yield put(authorizeFailure(e));
return null;
}
}
function* authorizeLoop(token) {
while (true) {
const refresh = token != null;
token = yield call(authorize, refresh);
if (token == null)
return;
yield call(delay, token.expires_in);
}
}
function* authentication() {
const storedToken = yield call(auth.getStoredToken);
while (true) {
if (!storedToken)
yield take(SIGN_IN);
const {signOutAction} = yield race({
signOutAction: take(SIGN_OUT),
authLoop: call(authorizeLoop, storedToken)
});
if (signOutAction) {
yield call(auth.storeToken, null);
}
}
} |
@aikoven nice! i do have a follow up question. Could you elaborate how this function works? cause in my flow it's just a service which uses fetch and returns the token. But my problem is that i need the token something like: but then i need access to my store and because auth.authorize is just some dumb javascript service it hasn't access to the store. I could pass in the credentials or the 'old' token but then i think i break the and i quote "So I'll first refactor localStorage calls into some isolated service" rule. So if you could elaborate how that part works i would be grateful, sorry for the beginners question. |
@Marinolinderhof In my project I used Google auth library that stored I guess the best solution for your case is to use |
@yelouafi this was amazing, it really helped me understand how saga might be used for async login requests. One question though - I'm a little confused about some of the
In this block, it makes sense that a function is called to save the token in localStorage, but what is the
Thank you in advance! |
|
thanks for the response - I actually meant these two specific references:
The format of the rest of the code suggests that Thanks. |
@aikoven This is a really nice example. I'm struggling to see how you combine other API requests with this though. For example, refresh token has expired, a request to an authorized resource is about to be made. How would you delay that request until the refresh token is done in here? Unless every other saga that hits the api has to check the expiry of the auth token? 🤔 |
@nouveller A common path is to start refreshing token before it expires, so that your current requests (and token refresh request as well) have time to finish. |
@aikoven That makes sense, keep an eye on your expiry time then trigger a refresh 5 minutes before or something. I suppose the only edge case to that may be the app goes into the background, token expires, app goes into the foreground and then you've got to do a refresh before any other api call. When I was using Unless there's an obvious solution to the edge case I mentioned above, maybe there's a nice way to wrap each saga so that it can wait until the expiry of the token is far enough in the future it won't interrupt a refresh. Thanks for your thoughts though either way. |
I'd go with a generic generator for all API calls that first gets the token from Redux store using |
@aikoven Sounds perfect, thanks for your input 👍 |
@nouveller that's what I'm worried about too. Having a delay on |
I think the loop is still valid, I have mine so that it checks every few
seconds if the token has expires and then refreshes it. I've not seen any
performance problems because of it, so far.
I've yet to implement the other requests however. But having a single saga
to handle other requests as suggested maybe the way to go. As it will need
to check the expiry, and then wait for the refresh before continuing. It
also need to set the new auth token into the request too.
The downside here is you loose the nice single sagas per request. Though
you could have a "helper" saga that was run before each request to check
the expiry.
…On Sat, 25 Feb 2017 at 06:35, Chappo ***@***.***> wrote:
@nouveller <https://github.com/nouveller> that's what I'm worried about
too. Having a delay on token.expires_in then the thread goes to sleep as
the app is not focused, when you re-open it the delay is still running but
is going to fire late. I haven't ran tests to see if this is the case yet.
@aikoven <https://github.com/aikoven> is on the money with having the
generic data saga and having it check the token expiry each time, I might
need to abandon the authorizeLoop for this approach.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AArhqNZIsGRbGfQOVWe0N7X7xEhY9wNbks5rf8upgaJpZM4G6rUL>
.
|
that's nice, i do have a follow up question, see the below code:
|
Yes, otherwise ur loop will skip take sign in |
thanks, if not set the null, the sign out action will be failure, refresh token is still valid. |
I throttle most of my api calls and put them through a specialized throttle function. I'm not queuing the calls (other than throttle-wise), because I don't care if I miss a call due to Internet outage or invalid token. These "missed" calls will be handled by another saga upon "INTERNET_ON" or "RECEIVED_TOKEN". I hope it helps someone. Please chime in if you think there is some wrong thinking in this.
|
Hi, I made an auth flow library (token based), which consists of 2 libs: https://github.com/alvelig/redux-saga-auth https://github.com/alvelig/redux-saga-api-call-routines It makes use of redux-saga-routines for compatibility with redux-form. Any comments are appreciated. |
@aikoven Could you post the complete auth flow somewhere? It will be great. |
@marcelaraujo It's there already: #14 (comment) |
@aikoven It was incomplete because you didn't handle the credentials on |
@zcmgyu
Usage:
|
Hi! sorry to revive the thread again. I have been trying your suggested approach to refresh a token if an API call is made that comes back with 401. I created wrapper saga function on fetch and it's all working fine except for one thing.
This will go through the API caller, make the call, and if it fails, it will refresh the token and make the call again with the new token. Awesome! Here's the code I came up with:
I hope it's not too bad. Any suggestions on how I may address that? |
Sorry, I don't have time to dive into your code. Instead I'll post my take on this. The below code serves me well in my app. The function If a task won't be forked due to no network or no token, this task will be retried automatically due to the extended pattern. I had problem with function extendPattern(pattern, actions) {
if (!Array.isArray(pattern)) pattern = [pattern];
if (!Array.isArray(actions)) actions = [actions];
actions.forEach((action) => {
if (!pattern.includes(action)) pattern.push(action);
});
return pattern;
}
export function* takeEveryIfOnlineAndTokenIsValid(pattern, task, ...args) {
//Add two triggers to the pattern in order to auto-resume if the action gets discarded due to network or token problems
pattern = extendPattern(pattern, [AUTH_GET_USER_TOKEN_SUCCESS, NETWORK_INTERNET_ON]);
while (true) {
const action = yield take(pattern);
const isOnline = yield select(selectNetworkStatus);
// const isOnline = yield call(NetInfo.isConnected.fetch);
const isAccessTokenValid = yield select(selectCheckAccessToken);
if (isOnline && isAccessTokenValid) {
yield fork(task, ...args, action);
} else {
console.log(
`Action ${action.type} discarded due to no network or invalid token (isOnline: ${isOnline}, isAccessTokenValid: ${isAccessTokenValid})`
);
if (DeviceInfo.isEmulator()) {
//The iOS simulator does not cope well with irregular Internet connections.
//We need to manually probe for internet if the simulator thinks that we are offline
if (!isOnline) yield put(networkSimulatorProbeAction());
}
}
}
} |
Here is a export function* throttleIfOnlineAndTokenIsValid(ms, pattern, task, ...args) {
pattern = extendPattern(pattern, [AUTH_GET_USER_TOKEN_SUCCESS, NETWORK_INTERNET_ON]);
const throttleChannel = yield actionChannel(pattern, buffers.sliding(1));
while (true) {
const action = yield take(throttleChannel);
const isOnline = yield select(selectNetworkStatus);
const isAccessTokenValid = yield select(selectCheckAccessToken);
if (isOnline && isAccessTokenValid) {
yield fork(task, ...args, action);
} else {
if (DeviceInfo.isEmulator()) {
//The iOS simulator does not cope well with irregular Internet connections.
//We need to manually probe for internet if the simulator thinks that we are offline
if (!isOnline) yield put(networkSimulatorProbeAction());
}
}
yield call(delay, ms);
}
} |
I'm trying to implement user authentication with following requirements:
Here's my attempt:
This code works well, but I wonder if there is more elegant solution for handling refresh. Currently it's too difficult to track
refreshDelay
effect. I thought about extracting refresh to separate saga that waits forAUTH_SUCCESS
action and then sets up a delay, but in this case I won't be able to cancel scheduled refresh if e.g. user signs out.The text was updated successfully, but these errors were encountered: