Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
oliverpool opened this issue Feb 10, 2022 · 4 comments
Closed

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

oliverpool opened this issue Feb 10, 2022 · 4 comments

Comments

@oliverpool
Copy link

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

authboss/oauth2/oauth2.go

Lines 235 to 247 in e62387f

user, err := storer.NewFromOAuth2(r.Context(), provider, details)
if err != nil {
return errors.Wrap(err, "failed to create oauth2 user from values")
}
user.PutOAuth2Provider(provider)
user.PutOAuth2AccessToken(token.AccessToken)
user.PutOAuth2Expiry(token.Expiry)
if len(token.RefreshToken) != 0 {
user.PutOAuth2RefreshToken(token.RefreshToken)
}
if err := storer.SaveOAuth2(r.Context(), user); err != nil {

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

@oliverpool
Copy link
Author

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)
}
  1. 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
Copy link
Author

related: #294

@aarondl
Copy link
Member

aarondl commented Feb 13, 2022

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
Copy link
Author

Experimented in #332

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants