403/forbidden and cancelling

[laurelnaiad]

This change adds support for 403/forbidden. It adds response.data to allow the backend to indicate reasons for 403s (and theoretically for 401s though that doesn't make as much sense).

It also renames the loginConfirmed function to confirmLogin to make it clearer that you are indicating confirmation to the service and adds cancelLogin. cancelLogin clears the buffer so that requests aren't stacked up if the user doesn't log in.

I have a pull request for the gh-pages that hacks up the demo app to demonstrate these features. I'll make a pull request for that as well, though you may not want to pull it in en-masse because I was somewhat sloppy with the HTML/CSS since I don't do photoshop. Also, I forgot to edit the readme file but I imagine that if you want to merge this you'd like to do the documentation yourself, anyway.

Thanks -- this is a great little component!

[witoldsz]

Hi, I like to way you added support for 403's and possible payload. I don't get the idea behind the 'loginConfirmed' to 'confirmLogin' rename, though. "Login confirmed" message (in this case the message is the method name) tells the service what happened already, while "confirm login" seems to indicate that the service is supposed to actually confirm anything - while it actually waits for us for the information when user login is confirmed.

[laurelnaiad]

Hi, Witold.. thanks for looking at this.

"loginConfirmed" sounded like it was the event, as opposed to something the application should call, which then actually does raise an event... but it's not a big deal to me. If I re-do the PR after restoring the original name, would you merge it? Anything else you'd like to see? Thanks!

(oops, I didn't mean to close the PR, but if you want I can resubmit it without the name change).

[witoldsz]

Hi, I can see your point about changing the method name. I don't see anything wrong with method names like events, after all, Alan Kay said, OOP is about messaging. I think your observation is correct: we are invoking a method loginConfirmed and this method is raising the event and this seems odd.

Now I think this: your version is not correct because the service has actually nothing to do with confirming the login. It is supposed to be notified after login has been confirmed. On the other side, as you said, my version lacks logic.

At the end, since both names are not perfect, I would stay with the old one to keep backward compatibility. I do not like changing the existing API from not-perfect into another not-perfect :)

I also wanted to ask you about the second method: cancelLogin.

 /**
  * call this function to indicate that authentication should not proceed.
  * All deferred requests will be cancelled.
  * @param data an optional argument to pass on to $broadcast.
  */
 cancelLogin: function(data) { [...] }

You are not cancelling the promises, you are rejecting them, so I would reword the comment to:

All deferred requests will be rejected.

Or I would consider doing something else: actually cancelling (abandoning?) those requests? What do you think? Can you provide a real use case for this? I am asking, because doing promise.reject() will trigger error callbacks everywhere across the application. What is more, you are rejecting without providing a reason, so how are errors callbacks supposed to figure out what happened? Maybe it would be better just to clear the buffer without rejecting? To be honest, that was my first though when I saw httpBuffer.clearAll(); - the name is misleading.

[laurelnaiad]

I hear you... the cancelling came about because I needed a way to stop the original request from being retried if the user didn't want to do a login (or a relogin). I'm open to alternatives. I do think the code that invoked the original request (which is now being cancelled due to not (successfully) logging in) should be supplied with a reason that the request isn't being completed, so rejecting the promise seems appropriate. How about a standardized error object -- "ErrorHttpNotAuthorized", or something like that -- should be returned in the rejection?

[laurelnaiad]

sorry... something else -- you say it will trigger error callbacks across the application -- does this mean that other code -- code that isn't involved with the action that's being disallowed -- is going to suffer rejections? I would have thought the only things in the buffer are things that are being disallowed due to permissions.

[laurelnaiad]

(As to "clearAll", I named it that because my first choice, "clear", is a reserved word -- no particular attachment to the function name.)

Now I think this: your version is not correct because the service has actually nothing to do with confirming the login. It is supposed to be notified after login has been confirmed. On the other side, as you said, my version lacks logic.

I was thinking about this after your initial response. There are two things happening in this function, regardless of what it's called.

1) the service is being asked to retry the buffered request(s). 2) an event is being broadcast to let any listeners know that someone has just logged in.

The event may not really be necessary -- the login process is (currently) completely outside the scope of this component and is already responsible on its own for figuring that someone just logged in, and if interested, who just logged in... should it also be responsible for any events that indicate the login? Perhaps the component should just expose retryRequests and cancelRequests (or whatever it may be called), and stay out of the events business (other than the one it uses to prompt the login process)?

Something is bothering me, though... I can't decide whether I think it would make sense for this component to become involved in session management in a larger sense. Half of me thinks no, half thinks yes. Broadcasting the login-confirmed event is kind of like the tip of an iceberg... should the component intercept try to intercept login responses and let an application-specified handler function parse them for information (such as who just logged in)? I can't figure out how I feel about that.

[laurelnaiad]

@witoldsz,

Just wanted to see if we can revive this. What would it take to get these features in?

Or I would consider doing something else: actually cancelling (abandoning?) those requests? What do you think? Can you provide a real use case for this? I am asking, because doing promise.reject() will trigger error callbacks everywhere across the application. What is more, you are rejecting without providing a reason, so how are errors callbacks supposed to figure out what happened? Maybe it would be better just to clear the buffer without rejecting? To be honest, that was my first though when I saw httpBuffer.clearAll(); - the name is misleading.

How about if we change the name of clearAll and keep the rejections? The reason I would want the rejections is that that is exactly what has happened... the bufferred requests aren't going to be allowed because of authentication/authorization, so rejection seems appropriate. I'd be happy to change the name and modify the PR. I'll also propose a rejection reason-passing mechanism through the PR.

I don't get the idea behind the 'loginConfirmed' to 'confirmLogin' rename, though. "Login confirmed" message (in this case the message is the method name) tells the service what happened already, while "confirm login" seems to indicate that the service is supposed to actually confirm anything - while it actually waits for us for the information when user login is confirmed.

I'm fine with keeping your function name and I can restore it in a new commit to the PR.

I'll put comments in the PR to indicate what I'm planning to do and then do another commit to see if you'll merge it :)

[witoldsz]

Hi there, I was thinking a little bit about the actual case provided by Stu:

the cancelling came about because I needed a way to stop the original request from being retried if the user didn't want to do a login

I am having trouble explaining to myself the reason to actually reject the buffered requests if user decides not to login.

The idea behind this project is to free the users of $http (and derivatives) to be bothered by 401 errors. In my previous application, we were applying the "head-up-display" messages when something were wrong with particular requests. Example: controller: $http(....).then(hud('operation-success'), hud('operation-failed')); , where hud is a service which returns a callback function displaying a message with specified key (message markups are in template, with specified identities).

The nice thing is that we do not have to check for '401' in the error callback, because it is not going to reach this point when using "angular-http-auth". Now, you are trying to invoke an action, login window pops up, you choose not to log-in and suddenly error massages appear on screen, because controller's $http invocation catch the error.

I think, when user decides to abandon the login process, we should abandon the buffered requests instead of rejecting them, shouldn't we?

[laurelnaiad]

My thinking is that rejecting them does give the app code that initiated the request the information on why it's not going to happen. Locally I've implemented the rejection reason that we discussed inline. I can just commit my stuff for the purposes of discussion. For now, please excuse the function name changes -- I can change them back if you want to merge, and if not, I'll just keep them the way they are. :)

[witoldsz]

So, are you really bothering your controllers with rejects to indicate the user wants to abandon the login process? Do the controllers care? What do you actually do after asking the module to reject the queue? Do you handle this rejection in all the controllers? I mean all the controllers, because one cannot guess which controllers can be affected by the rejections, because user can loose theirs session any time.

[laurelnaiad]

What to do depends on the context of what the user was trying to do, but there's a pretty big difference between a post failing because the REST api just crashed, vs. failing because of invalid data being posted, vs. failing because authorization didn't happen, vs. getting no feedback at all (i.e. if the request was just dropped without notice).

[laurelnaiad]

Just for the sake of completeness -- I'm actually using a sessionManager service to handle authentication, but it does indeed pass back the reason to the controller (in the promise rejection). It's entirely up to the controller to decide what to do about it... of course, the controller is free to ignore the information altogether.

Edit: so yes, I'm bothering the controllers -- I don't want any controller that is asking for an exchange with the REST api to go about ignoring the success or failure of the request, and I do want the controller to make a smart move if there's a failure, and the only way to make a smart move is to know why the failure happened.

Edit 2: also, since $http is used to fetch templates when provided to the router, you can have navigation that fails due to authorization as well as AJAX, which is a bit of a different use case, but also a case where you'll want to know why something isn't happening. It's not a 404 and it's not a 500 error.. its a 401/403 that, if the user can't or won't login, has to come back in some form to the app.

[witoldsz]

OK, but you did not answer my question: what do you actually do when rejecting the requests? You said you want your controllers to actively participate when user decides to abandon the login procedure. I am just trying to catch the real use-case, not the, kind of, manifest: "Truth for everyone, each party interacting with $http has right to know what happened". It is hard to for me to imagine to prepare my application for login abandon action in... like hundred places, spreading the error callbacks for 'abandon' all over the application. I guess I would just redirect user to some common place or maybe even reload the page?

The bottom line is: do you want rejections "just because" or do you have good reason to use that rejections. My case, as described earlier, is that such a rejections could cause unintended HUD messages, because I never check for 401's in application, excluding one place where I handle login.

[laurelnaiad]

I don't know what a HUD message is, but if you're not checking the rejections then I would submit that it doesn't matter if they're sent.

I guess maybe we're seeing this in different lights. It seems really basic to me that the code that initiated the request that's being denied should know that it's being denied due to authorization failure.

One typically does different things based on why things are rejected -- that's why we have HTTP status codes to begin with. If the user enters data that cannot be saved because it would create an inconsistency with another user's data (e.g. optimistic locking) then would you want the application to know the difference between that and an authorization-based rejection? How about between a server meltdown/500 and an authorization-based rejection? Or do you just kind of take all failures and treat them identically?

I'll leave it at that and let you decide how you want to proceed.

[witoldsz]

Hi again, I think we are talking about different things. You still haven't answered my question: what are you doing with rejections caused by user abandoning login. I cannot agree with the statement:

It seems really basic to me that the code that initiated the request that's being denied should know that it's being denied due to authorization failure.

There are thing that are supposed to be handled "on-the-spot" and things that are supposed to be handled elsewhere, application-wide.

Anyway: I think there is space to support support both options: cancel and reject the buffered requests. No need to force one solution only.

[laurelnaiad]

There are thing that are supposed to be handled "on-the-spot" and things that are supposed to be handled elsewhere, application-wide.

I can agree with that statement. Rejecting the request seems like a no-brainer, giving the app developer the option of handling it at the place where it was initiated. The login-cancelled event is great for application-wide handling.

what are you doing with rejections caused by user abandoning login.

When a user tries to do something that they aren't allowed to do, you tell them they aren't allowed to do it. If they're navigating to something they can't see, you can show them an "unauthorized" message (in the context of where they were) and let them go "back". You don't do that on a 500 error and you don't do that if you just get an empty response from the api.

For me, the case is pretty simple -- if there were no http-auth-interceptor you'd be getting a 401 or 403 from the server in your $http rejection. So why should it be any different after the interceptor has done its work and the user still hasn't logged in? The interceptor is great for centralizing the interjection of a login opportunity and hanging onto the request(s), but it shouldn't force the app into losing the context of the actual 401/403 information.

So it occurs to me now that perhaps the ideal for the interceptor is that, if you cancel it, the app code should in fact get a rejection that looks exactly like the one that originally prompted the interception? Just a thought.

If it doesn't seem right to you, by all means don't do it -- I don't have much skin in the game. Just wanted to contribute where I could.

[witoldsz]

Cherry picked: 1730092 Also, cancel action can either abandon or reject the buffer.