patch bug in OAuth2 module

stephenafamo commented 4 years ago

In the OAuth2 module, the session PID is put like this:

authboss.PutSession(w, authboss.SessionKey, authboss.MakeOAuth2PID(provider, user.GetOAuth2UID()))

I don't think it's necessary. Since the OAuth2User interface already requires a GetPID method, I think we should use that directly and leave it up to the user to create the PID using the Helper function.

This could also cause an issue because Middleware2 requires authboss.SessionKey to be the user PID, and during this process, theGetPID method is called. If the user themselves are not using authboss.MakeOAuth2PID to create their PIDs, they will run into a problem because the End method in the OAuth2 modules assumes this must be the case.

I've also modified the mocks package to take this into account. So it does not return the email when the Oauth2 module tests ask for the PID.

stephenafamo commented 4 years ago

Can I get some feedback on this?

aarondl commented 4 years ago

Hi @stephenafamo. GetPID could be used by an application to show a user's e-mail or name or otherwise in a different authboss module. That module should not need to know that the PID is some combined value like facebook:a@a.com. That's to say it would require all modules that are built on the GetPID() to be aware that oauth exists and that their pids may come back in this odd joined way.

Unless I'm perhaps missing something (been a while since I've been deep in this code).

stephenafamo commented 4 years ago

In the Oauth2 module, when creating the user, the module ALWAYS uses the MakeOAuth2PID method to set the PID. However, the Oauth2User interface already requires a GetPID method which the user can implement to use the MakeOAuth2PID method if they want.

The current implementation means that if I was combining this with other modules, while other modules could use my custom GetPID method which may be returning the email or a UUID, the Oauth2 module would create it's own PID regardless of my implementation.

aarondl commented 4 years ago

Hm. How do we deal with the fact that this is a breaking change to users though? They must alter their GetPID() methods if this change was to be applied otherwise everything breaks for them, yes?

stephenafamo commented 4 years ago

So, I've reasoned about this, and I think it's non-breaking.

The GetPID() method is already required for the Oauth2User, the line I changed is used to set the session, and the same GetPID() method is used when retrieving the user.

I'm actually surprised that this has not caused any problems for anyone so far.

If someone worked around the inconsistency, by making their GetPID() method use the MakeOAuth2PID method for Oauth2 users, it will continue to work as expected.

aarondl commented 4 years ago

Ok, I really had to go and dig in and get back up to speed with all this because I'll be honest I just wasn't understanding everything.

Middleware2 uses the same old LoadCurrentUser/CurrentUser/CurrentUserID nonsense that everything else does. This means that it looks in the session data using SessionKey which pulls back something that a storer can figure out how to use.

The session is keyed the way it is (automatically) so GetPID() always does the same thing, returns something that the user could be shown that relates to their account that's not prefixed with some internal identifier. Even in OAuth2 this can quite often be an e-mail and that's useful to be able to show a user in a template or something.

The place that this all gets resolved with in a typical implementation is the storage layer: https://github.com/volatiletech/authboss-sample/blob/master/storer.go#L252 https://github.com/volatiletech/authboss-sample/blob/master/storer.go#L355

So you noted that the storage layer gets passed an "incorrect pid" but it seems that the decision was made to have the storage layer explicitly understand an OAuth2 PID. Does this change your perspective on the issue? Both as a bug/problem currently, and convincing you that it would indeed be a breaking change to those using the system as it is?

stephenafamo commented 4 years ago

You're correct about how it works.

Why I assumed this is a bug

This is undocumented behaviour.

Based on what is documented (see this), I would expect the Load method to receive a PID, and if I did not set the GetPID method to use an OAuth2PID, this will break, even if my own implementation of GetPID returns say the email of the OAuth2 user.

The change I'm suggesting removes this undocumented behaviour. The user can continue to use something like the Load() implementation in the authboss-sample, if they also explicitly use the MakeOAuth2PID() method like in the authboss-sample

What could break

Thinking about it some more, I can see how it will break for some users if they expect the Load() method to be sent an OAuth2PID even if they did not set it themselves.

I think this is relying on buggy behaviour and we should change it. However, if we are going to leave it as it is, I would suggest some documentation about this behaviour in the section about OAuth2 authentication.

aarondl commented 4 years ago

The breaking change really comes from the storer's expecting OAuth2 pids, and then suddenly not receiving them. And instead looking up users based on... email? Whatever it is the PID is currently returning, which for OAuth2 users will likely not be actually unique (which is why these weird provider:uid OAuth2 pids exist in the first place).

I am not sure this is "buggy" behavior. It seems logical to me. If you log in as an OAuth2 user your session key becomes: facebook:your@email.com, if you log in as a normal user your session key becomes: your@email.com. The automatic behavior here prevents collisions in general and something like GetPID() can continue to return only an e-mail address keeping the user implementation simple/dumb. The other thing about it is it allows a user to login as via OAuth2 or via a regular login depending on which channel they went down.

In order to do correct lookups the storer will always have to be implemented as it is. Without the pid given to it containing both pieces of information its possible that it looks up the incorrect thing. The question then becomes: Do we want this behavior to be implicit/automatic? Or explicit. Because every single use of OAuth2 requires this logic to be put into GetPID(), perhaps its better to simply not to avoid the duplication across every implementation of User?

The above is mostly just me thinking out loud. But the conclusions I've come to from it are:

Creating a different PID for /auth vs /oauth2 logins allows us to determine how a user logged in if they are both an OAuth user and a Password user.
This logic would need to be duplicated in all User() implementations instead of in the one place in the library it's currently in
The storers must remain unchanged, they must always deal with OAuth2 pids as a special case of Load and therefore this change doesn't save us from any sort of surprises there

As of this message I'm pretty unconvinced that this change is necessary. Particularly because of the loss of the feature of being able to differentiate the login type as well as it is a breaking change. I do agree that the documentation is underwhelming and we should correct it.

Do you think this is sufficient? https://github.com/volatiletech/authboss/compare/7cb9fa3..08fa0a6

stephenafamo commented 4 years ago

Creating a different PID for /auth vs /oauth2 logins allows us to determine how a user logged in if they are both an OAuth user and a Password user.

I assumed that the method IsOAuth2User() is meant to be used to make this differentiation.

Do you think this is sufficient? https://github.com/volatiletech/authboss/compare/7cb9fa3..08fa0a6

Yes this works. Probably the best way to avoid breaking anything while documenting what people should expect.

I can see how this helps. It's an easy way to know how the user was authenticated in the situations where multiple authentication methods are present.

I guess the choice is whether we want the users adding the extra logic in the Load() method or in the GetPID(). Since it is already in Load(), with documentation, there may be no real need to change it.

aarondl commented 4 years ago

@stephenafamo I'm convinced no matter what you'd need to have Load() understand OAuth2 pids. So all this change would do is complicate User implementations. You -must- be able to be given an OAuth2 pid to be able to correctly look up the user in the database. I think I've convinced myself this is not necessary.

Thank you for being patient with me while I remembered how my own library worked (haha) and thanks for bringing up the issue. It led to improvement either way. I just moved authboss to v3 with Go modules just so your aware. All this looking into it reminded me it needed to be done. Hope that helps and doesn't hinder you :)

stephenafamo commented 4 years ago

Love the library, I use it on all my projects 😁

Nice to see the v3 update and support for go modules. Let me go update my projects 👍

volatiletech / authboss

patch bug in OAuth2 module #294

Why I assumed this is a bug

What could break