volatiletech / authboss

The boss of http auth.
MIT License
3.79k stars 204 forks source link

oauth2: user.OAuth2UID not set before calling SaveOAuth2 #333

Closed oliverpool closed 2 years ago

oliverpool commented 2 years ago

Is there any reason why user.OAuth2UID is not set before calling SaveOAuth2 ? (just like provider, accessToken, Expiry and RefreshToken)

https://github.com/volatiletech/authboss/blob/e62387f74f1fd01cce883130882065b5bb701c8a/oauth2/oauth2.go#L235-L247

This can be handled by the developer in NewFromOAuth2, but it feels kind of odd.

oliverpool commented 2 years ago

Thinking more about this, maybe the usage of Oauth2 module could be simplified (and made more flexible).

  1. merge NewFromOAuth2 and SaveOAuth2 from OAuth2ServerStorer into one method to find or create a user:
    
    type OAuth2Result struct {
    Provider     string
    AccessToken  string
    Expiry       time.Time
    RefreshToken string
    }

type OAuth2ServerStorer interface { FindOrCreateOAuth2User(ctx context.Context, result OAuth2Result, details map[string]string) (User, error) }


2. drop `authboss.OAuth2User` and simply use `GetPID` everywhere.

This would be a fairly large (breaking) change, but it would make it easier to implement oauth2 support.

I guess there are still other steps needed to allow the usage of multiple OAuth2 providers to log the same user (I think this is currently not possible).
For instance a `AddOAuth2ProviderToUser()` to add a oauth2 provider to an already existing (and logged-in) user.
oliverpool commented 2 years ago

related: https://github.com/volatiletech/authboss/pull/294

aarondl commented 2 years ago

The reason was to give the developer control over the OAuth2ID. Because we don't understand their provider or how their storage system might work we didn't want to enforce specifics.

This is called out here:

  1. The special casing of the ServerStorer implementation's Load() function to deal properly with incoming OAuth2 pids. See authboss.ParseOAuth2PID as a way to do this.

The initial idea behind Authboss's oauth2 was actually not to separate the identity of oauth users, but be able to (if desired) combine them with the existing user/password style. Hence this extra flexibility and lack of prescription.

oliverpool commented 2 years ago

Experimented in #332