36

As per this tweet by Jason Punyon, I was kind of wondering if any of the SO devs could elaborate a little on why exactly this method is so terrible/hard to refactor?

A few choice words about HandleSuccess from the #stackoverflow chat rooms.

I do not know what the HandleSuccess method actually is, but from what I can read in the tweet/pic, I assume it is a part of the Stack Overflow code base.

  • 16
    Questions asking for code must demonstrate a minimal understanding of the problem being solved. Include attempted solutions, why they didn't work, and the expected results. See also: Stack Overflow question checklist................ wait a second, this isn't right – Richard Tingle Nov 14 '13 at 17:58
  • 14
    i.imgur.com/eFK7Ilj.png – balpha Nov 14 '13 at 17:58
  • 2
    @RichardTingle From the line about flags, I'd assume it's a function that handles marking successful flags. wild shot in the dark – animuson Nov 14 '13 at 17:59
  • That must explain why it's so difficult to get helpful/decline actions on individual flags, instead of all the flags on the post at the same time. – Robert Harvey Nov 14 '13 at 18:00
  • Could you quote the tweet? Not sure if it illuminates anything, but I'm curious and on a restricted network. – jball Nov 14 '13 at 18:02
51

It's the method in UserController.Authentication.cs that handles a successful OpenId authentication. As such, it contains all the brittleness that is the handling of various edge cases around OpenId that have been discovered over the years. In addition, session creation and – because of things like converting an unregistered to a registered acount – user management are initiated from here. And because user creation may also require network account creation, and a network account needs a site user, there are some nice race conditions to be worried about as well.

HandleSuccess isn't actually as horrible as its reputation, but still any kind of having to touch that method is usually a "drawing the short straw" kind of thing. Nobody wants to be the person who broke login.

YOU CAN'T HANDLE THE SUCCESS!

  • 36
    Don't forget it handles its own success, or fail, by calling itself via a provider callback after a negotiation, but only if a merge didn't happen, or if there was a match and not a queue, but not on a new account only with the same IP range with...shoot me. – Nick Craver Nov 14 '13 at 18:10
  • 33
    So it's "One Of THOSE Functions" -- dealing with a problem domain so large as to be nigh-incomprehensible, riddled with special cases to handle other people's brokenness, and absolutely critical to the functioning of the environment. Everyone wants it rewritten to be simple and elegant, but all the people smart enough to do such a rewrite are also smart enough to leave it the hell alone lest they let the magic smoke out :-) – voretaq7 Nov 14 '13 at 18:11
  • @voretaq7 I'd say that's spot-on. – balpha Nov 14 '13 at 18:11
  • 40
    To get into the mindset of HandleSuccess, realize that there is no HandleFailure. – Kevin Montrose Nov 14 '13 at 18:29
  • 3
    Aug-28-2009, ParseSuccessfulAuthentication was renamed to HandleSuccess, a day that shall live in infamy forever. – Jarrod Dixon Nov 15 '13 at 0:17
  • 1
    @voretaq7 Like Date functions? Makes sense. – Manishearth Nov 15 '13 at 7:56
  • You guys should replace it with a bunch of pigeons clacking away at keyboards and handling authentication – Manishearth Nov 15 '13 at 7:58
  • 1
    @KevinMontrose Authentication CAN'T fail. Now if you'll excuse me there are some raving psychotic escapees from the local insane asylum who look like they really need to use a Q&A website for something... – voretaq7 Nov 15 '13 at 8:04
  • Is HandleSuccess still a thing with the global auth? – Braiam Sep 21 '16 at 2:13
  • @Braiam Yes, it just has to run a little less often :) – balpha Sep 21 '16 at 5:04
12

Wha.. ? What happened...

I tried to grok the 200 lines of code that signifies an actionresult which somehow starts at stage 2 and contains the word if 54 times. After having read it 3 times I uh,

Everyone deals with success in their own way.

What makes it kind of complicated though is the basic process of authenticating efficiently against multiple providers all the while accounting for a slough of strange behaviors and attempted exploitations (turns out that programmers like to game systems).

Perhaps the user wanted to use an openid provider, but didn't have an account with them. Well it is too late. They were already redirected. So at that point the user is making a new account and then expects to magically return all signed in using their newly created account.

But hey, that doesn't always happen. Maybe they entered the correct credentials for the provider and the rest of the exchange side for the user needs to be examined. Is this openid even associated with someone? Or, does someone already have a different openid associated with them but logged in with this one for some reason while already logged in with another one?

If they did have the correct openid account, and no account with SE, maybe they want to create one? Or they don't have to. What if they try to create one but it exists? Better hold on to that openid authentication through this whole process. Definitely don't want to start over... that would be awkward! That is strange, this user was merged with another user but tried to log in from their original openid.

Dang. Did all that and they weren't even using https. They should probably start over. Finally, got through it all again? Then you went afk for two days and clicked submit? Did your cookie expire? Hm. Oh well, lets send the user back.. Wait, where were they again?

11
  1. Wrap all of the desired behaviors of HandleSuccess in unit tests
  2. Rewrite HandleSuccess from the ground up.
  3. ??
  4. Pass all of the unit tests.
  5. Profit.
  • 19
    ...The problem is usually the programmers all commit suicide about halfway through step 1 – voretaq7 Nov 14 '13 at 18:03
  • You forgot the magic step ("???") between 3 and 4. You are right - before the unit tests is better. – chue x Nov 14 '13 at 18:04
  • 1
    You can't actually test it that well, it's an OpenID handler that must interact with external providers - including the quirky ones we don't and can't have accounts to. – Nick Craver Nov 14 '13 at 18:05
  • 1
    That's what mocks are for. Should only take you a year or so to write the necessary ones. :) – Robert Harvey Nov 14 '13 at 18:05
  • 22
    what are unit tests? – Kasra Rahjerdi Nov 14 '13 at 18:15
  • 14
    I find the programs with 0% unit test coverage pass the units test with the greatest reliability – Richard Tingle Nov 14 '13 at 18:18
  • 3
    [insert gratuitous Microsoft joke here] – Robert Harvey Nov 14 '13 at 18:19
  • 2
    @KasraRahjerdi I'm guessing they have to do with tests? In that case, the answer is "c". – Cole Johnson Nov 14 '13 at 18:35

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .