39

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.

Improve this question
5
  • 17
    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, 2013 at 17:58
  • 15
    i.imgur.com/eFK7Ilj.png
    – balpha StaffMod
    Nov 14, 2013 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 StaffMod
    Nov 14, 2013 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.
    – user102937
    Nov 14, 2013 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, 2013 at 18:02

3 Answers 3

Reset to default
54

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!

Improve this answer
10
  • 37
    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, 2013 at 18:10
  • 35
    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, 2013 at 18:11
  • @voretaq7 I'd say that's spot-on.
    – balpha StaffMod
    Nov 14, 2013 at 18:11
  • 42
    To get into the mindset of HandleSuccess, realize that there is no HandleFailure.
    – Kevin Montrose
    Nov 14, 2013 at 18:29
  • 4
    Aug-28-2009, ParseSuccessfulAuthentication was renamed to HandleSuccess, a day that shall live in infamy forever.
    – Jarrod Dixon
    Nov 15, 2013 at 0:17
  • 1
    @voretaq7 Like Date functions? Makes sense.
    – Manishearth
    Nov 15, 2013 at 7:56
  • You guys should replace it with a bunch of pigeons clacking away at keyboards and handling authentication
    – Manishearth
    Nov 15, 2013 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, 2013 at 8:04
  • Is HandleSuccess still a thing with the global auth?
    – Braiam
    Sep 21, 2016 at 2:13
  • @Braiam Yes, it just has to run a little less often :)
    – balpha StaffMod
    Sep 21, 2016 at 5:04
13

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?

Improve this answer
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.
Improve this answer
8
  • 19
    ...The problem is usually the programmers all commit suicide about halfway through step 1
    – voretaq7
    Nov 14, 2013 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, 2013 at 18:04
  • 2
    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, 2013 at 18:05
  • 1
    That's what mocks are for. Should only take you a year or so to write the necessary ones. :)
    – user102937
    Nov 14, 2013 at 18:05
  • 23
    what are unit tests?
    – Kasra Rahjerdi
    Nov 14, 2013 at 18:15
  • 15
    I find the programs with 0% unit test coverage pass the units test with the greatest reliability
    – Richard Tingle
    Nov 14, 2013 at 18:18
  • 3
    [insert gratuitous Microsoft joke here]
    – user102937
    Nov 14, 2013 at 18:19
  • 2
    @KasraRahjerdi I'm guessing they have to do with tests? In that case, the answer is "c".
    – Cole Tobin
    Nov 14, 2013 at 18:35

You must log in to answer this question.

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