diff --git a/CHANGELOG.md b/CHANGELOG.md index c2fd53a6..efe7402e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ - Regenerated JSVM types to include methods from structs with single generic parameter. +- Fixed `RateLimitRule.Audience` code comment ([#7098](https://github.com/pocketbase/pocketbase/pull/7098); thanks @iustin05). + +- Try to forward Apple OAuth2 POST redirect user's name so that it can be returned (and eventually assigned) with the success response of the all-in-one auth call ([#7090](https://github.com/pocketbase/pocketbase/issues/7090)). + ## v0.29.2 diff --git a/apis/record_auth_with_oauth2.go b/apis/record_auth_with_oauth2.go index cdab3bc8..cbc5f400 100644 --- a/apis/record_auth_with_oauth2.go +++ b/apis/record_auth_with_oauth2.go @@ -15,6 +15,7 @@ import ( validation "github.com/go-ozzo/ozzo-validation/v4" "github.com/pocketbase/dbx" "github.com/pocketbase/pocketbase/core" + "github.com/pocketbase/pocketbase/tools/auth" "github.com/pocketbase/pocketbase/tools/dbutils" "github.com/pocketbase/pocketbase/tools/filesystem" "golang.org/x/oauth2" @@ -90,6 +91,17 @@ func recordAuthWithOAuth2(e *core.RequestEvent) error { return firstApiError(err, e.BadRequestError("Failed to fetch OAuth2 user.", err)) } + // Apple currently returns the user's name only as part of the first redirect data response + // so we try to assign the [apis.oauth2SubscriptionRedirect] forwarded name. + if form.Provider == auth.NameApple && authUser.Name == "" { + nameKey := oauth2RedirectAppleNameStoreKeyPrefix + form.Code + name, ok := e.App.Store().Get(nameKey).(string) + if ok { + e.App.Store().Remove(nameKey) + authUser.Name = name + } + } + var authRecord *core.Record // check for existing relation with the auth collection diff --git a/apis/record_auth_with_oauth2_redirect.go b/apis/record_auth_with_oauth2_redirect.go index f5bde653..7115ce56 100644 --- a/apis/record_auth_with_oauth2_redirect.go +++ b/apis/record_auth_with_oauth2_redirect.go @@ -3,21 +3,27 @@ package apis import ( "encoding/json" "net/http" + "strings" + "time" "github.com/pocketbase/pocketbase/core" "github.com/pocketbase/pocketbase/tools/subscriptions" ) const ( - oauth2SubscriptionTopic string = "@oauth2" - oauth2RedirectFailurePath string = "../_/#/auth/oauth2-redirect-failure" - oauth2RedirectSuccessPath string = "../_/#/auth/oauth2-redirect-success" + oauth2SubscriptionTopic string = "@oauth2" + oauth2RedirectFailurePath string = "../_/#/auth/oauth2-redirect-failure" + oauth2RedirectSuccessPath string = "../_/#/auth/oauth2-redirect-success" + oauth2RedirectAppleNameStoreKeyPrefix string = "@redirect_name_" ) type oauth2RedirectData struct { State string `form:"state" json:"state"` Code string `form:"code" json:"code"` Error string `form:"error" json:"error,omitempty"` + + // returned by Apple only + AppleUser string `form:"user" json:"-"` } func oauth2SubscriptionRedirect(e *core.RequestEvent) error { @@ -52,6 +58,19 @@ func oauth2SubscriptionRedirect(e *core.RequestEvent) error { } defer client.Unsubscribe(oauth2SubscriptionTopic) + // see https://github.com/pocketbase/pocketbase/issues/7090 + if data.AppleUser != "" && data.Error == "" && data.Code != "" { + nameErr := parseAndStoreAppleRedirectName( + e.App, + oauth2RedirectAppleNameStoreKeyPrefix+data.Code, + data.AppleUser, + ) + if nameErr != nil { + // non-critical error + e.App.Logger().Debug("Failed to parse and load Apple Redirect name data", "error", nameErr) + } + } + encodedData, err := json.Marshal(data) if err != nil { e.App.Logger().Debug("Failed to marshalize OAuth2 redirect data", "error", err) @@ -72,3 +91,51 @@ func oauth2SubscriptionRedirect(e *core.RequestEvent) error { return e.Redirect(redirectStatusCode, oauth2RedirectSuccessPath) } + +// parseAndStoreAppleRedirectName extracts the first and last name +// from serializedNameData and temporary store them in the app.Store. +// +// This is hacky workaround to forward safely and seamlessly the Apple +// redirect user's name back to the OAuth2 auth handler. +// +// Note that currently Apple is the only provider that behaves like this and +// for now it is unnecessary to check whether the redirect is coming from Apple or not. +// +// Ideally this shouldn't be needed and will be removed in the future +// once Apple adds a dedicated userinfo endpoint. +func parseAndStoreAppleRedirectName(app core.App, nameKey string, serializedNameData string) error { + if serializedNameData == "" { + return nil + } + + // https://developer.apple.com/documentation/signinwithapple/incorporating-sign-in-with-apple-into-other-platforms#Handle-the-response + extracted := struct { + Name struct { + FirstName string `json:"firstName"` + LastName string `json:"lastName"` + } `json:"name"` + }{} + if err := json.Unmarshal([]byte(serializedNameData), &extracted); err != nil { + return err + } + + fullName := extracted.Name.FirstName + " " + extracted.Name.LastName + + // truncate just in case to prevent storing large strings in memory + if len(fullName) > 150 { + fullName = fullName[:150] + } + + fullName = strings.TrimSpace(fullName) + if fullName == "" { + return nil + } + + // store (and remove) + app.Store().Set(nameKey, fullName) + time.AfterFunc(90*time.Second, func() { + app.Store().Remove(nameKey) + }) + + return nil +} diff --git a/apis/record_auth_with_oauth2_redirect_test.go b/apis/record_auth_with_oauth2_redirect_test.go index f8cb06ce..5a09af30 100644 --- a/apis/record_auth_with_oauth2_redirect_test.go +++ b/apis/record_auth_with_oauth2_redirect_test.go @@ -3,6 +3,7 @@ package apis_test import ( "context" "net/http" + "net/url" "strings" "testing" "time" @@ -266,6 +267,41 @@ func TestRecordAuthWithOAuth2Redirect(t *testing.T) { } }, }, + { + Name: "(POST) Apple user's name json", + Method: http.MethodPost, + URL: "/api/oauth2-redirect", + Body: strings.NewReader(url.Values{ + "code": []string{"123"}, + "state": []string{clientStubs[8]["c3"].Id()}, + "user": []string{ + `{"name":{"firstName":"aaa","lastName":"` + strings.Repeat("b", 200) + `"}}`, + }, + }.Encode()), + Headers: map[string]string{ + "content-type": "application/x-www-form-urlencoded", + }, + BeforeTestFunc: beforeTestFunc(clientStubs[8], map[string][]string{ + "c3": {`"state":"` + clientStubs[8]["c3"].Id(), `"code":"123"`}, + }), + ExpectedStatus: http.StatusSeeOther, + ExpectedEvents: map[string]int{"*": 0}, + AfterTestFunc: func(t testing.TB, app *tests.TestApp, res *http.Response) { + app.Store().Get("cancelFunc").(context.CancelFunc)() + + checkSuccessRedirect(t, app, res) + + if clientStubs[8]["c3"].HasSubscription("@oauth2") { + t.Fatalf("Expected oauth2 subscription to be removed") + } + + storedName, _ := app.Store().Get("@redirect_name_123").(string) + expectedName := "aaa " + strings.Repeat("b", 146) + if storedName != expectedName { + t.Fatalf("Expected stored name\n%q\ngot\n%q", expectedName, storedName) + } + }, + }, } for _, scenario := range scenarios { diff --git a/apis/record_auth_with_oauth2_test.go b/apis/record_auth_with_oauth2_test.go index 0e2a385c..b1625fbf 100644 --- a/apis/record_auth_with_oauth2_test.go +++ b/apis/record_auth_with_oauth2_test.go @@ -1641,6 +1641,104 @@ func TestRecordAuthWithOAuth2(t *testing.T) { ExpectedContent: []string{"TX_ERROR"}, }, + // Apple AuthUser.Name assign checks + // ----------------------------------------------------------- + { + Name: "store name with Apple provider", + Method: http.MethodPost, + URL: "/api/collections/users/auth-with-oauth2", + Body: strings.NewReader(`{ + "provider": "apple", + "code":"test_code", + "redirectURL": "https://example.com" + }`), + BeforeTestFunc: func(t testing.TB, app *tests.TestApp, e *core.ServeEvent) { + users, err := app.FindCollectionByNameOrId("users") + if err != nil { + t.Fatal(err) + } + + // register the test provider + auth.Providers[auth.NameApple] = func() auth.Provider { + return &oauth2MockProvider{ + AuthUser: &auth.AuthUser{Id: "test_id"}, + Token: &oauth2.Token{AccessToken: "abc"}, + } + } + + app.Store().Set("@redirect_name_test_code", "test_store_name") + + // add the test provider in the collection + users.MFA.Enabled = false + users.OAuth2.Enabled = true + users.OAuth2.Providers = []core.OAuth2ProviderConfig{{ + Name: auth.NameApple, + ClientId: "123", + ClientSecret: "456", + }} + if err := app.Save(users); err != nil { + t.Fatal(err) + } + }, + ExpectedStatus: 200, + ExpectedContent: []string{ + `"meta":{`, + `"name":"test_store_name"`, + }, + AfterTestFunc: func(t testing.TB, app *tests.TestApp, res *http.Response) { + if app.Store().Has("@redirect_name_test_code") { + t.Fatal("Expected @redirect_name_test_code store key to be removed") + } + }, + }, + { + Name: "store name with non-Apple provider", + Method: http.MethodPost, + URL: "/api/collections/users/auth-with-oauth2", + Body: strings.NewReader(`{ + "provider": "test", + "code":"test_code", + "redirectURL": "https://example.com" + }`), + BeforeTestFunc: func(t testing.TB, app *tests.TestApp, e *core.ServeEvent) { + users, err := app.FindCollectionByNameOrId("users") + if err != nil { + t.Fatal(err) + } + + // register the test provider + auth.Providers["test"] = func() auth.Provider { + return &oauth2MockProvider{ + AuthUser: &auth.AuthUser{Id: "test_id"}, + Token: &oauth2.Token{AccessToken: "abc"}, + } + } + + app.Store().Set("@redirect_name_test_code", "test_store_name") + + // add the test provider in the collection + users.MFA.Enabled = false + users.OAuth2.Enabled = true + users.OAuth2.Providers = []core.OAuth2ProviderConfig{{ + Name: "test", + ClientId: "123", + ClientSecret: "456", + }} + if err := app.Save(users); err != nil { + t.Fatal(err) + } + }, + ExpectedStatus: 200, + NotExpectedContent: []string{ + `"name":"test_store_name"`, + }, + AfterTestFunc: func(t testing.TB, app *tests.TestApp, res *http.Response) { + if !app.Store().Has("@redirect_name_test_code") { + t.Fatal("Expected @redirect_name_test_code store key to NOT be deleted") + } + }, + }, + // rate limit checks // ----------------------------------------------------------- { diff --git a/tools/auth/apple.go b/tools/auth/apple.go index fc514d36..9783c2f2 100644 --- a/tools/auth/apple.go +++ b/tools/auth/apple.go @@ -4,7 +4,6 @@ import ( "context" "encoding/json" "errors" - "strings" "github.com/golang-jwt/jwt/v5" "github.com/pocketbase/pocketbase/tools/types" @@ -23,7 +22,7 @@ const NameApple string = "apple" // Apple allows authentication via Apple OAuth2. // -// [OIDC differences]: https://bitbucket.org/openid/connect/src/master/How-Sign-in-with-Apple-differs-from-OpenID-Connect.md +// OIDC differences: https://bitbucket.org/openid/connect/src/master/How-Sign-in-with-Apple-differs-from-OpenID-Connect.md. type Apple struct { BaseProvider @@ -47,7 +46,7 @@ func NewAppleProvider() *Apple { // FetchAuthUser returns an AuthUser instance based on the provided token. // -// API reference: https://developer.apple.com/documentation/sign_in_with_apple/tokenresponse. +// API reference: https://developer.apple.com/documentation/signinwithapple/authenticating-users-with-sign-in-with-apple#Retrieve-the-users-information-from-Apple-ID-servers. func (p *Apple) FetchAuthUser(token *oauth2.Token) (*AuthUser, error) { data, err := p.FetchRawUserInfo(token) if err != nil { @@ -60,16 +59,13 @@ func (p *Apple) FetchAuthUser(token *oauth2.Token) (*AuthUser, error) { } extracted := struct { - Id string `json:"sub"` - Name string `json:"name"` - Email string `json:"email"` EmailVerified any `json:"email_verified"` // could be string or bool - User struct { - Name struct { - FirstName string `json:"firstName"` - LastName string `json:"lastName"` - } `json:"name"` - } `json:"user"` + Email string `json:"email"` + Id string `json:"sub"` + + // not returned at the time of writing and it is usually + // manually populated in apis.recordAuthWithOAuth2 + Name string `json:"name"` }{} if err := json.Unmarshal(data, &extracted); err != nil { return nil, err @@ -89,17 +85,13 @@ func (p *Apple) FetchAuthUser(token *oauth2.Token) (*AuthUser, error) { user.Email = extracted.Email } - if user.Name == "" { - user.Name = strings.TrimSpace(extracted.User.Name.FirstName + " " + extracted.User.Name.LastName) - } - return user, nil } // FetchRawUserInfo implements Provider.FetchRawUserInfo interface. // -// Apple doesn't have a UserInfo endpoint and claims about users -// are instead included in the "id_token" (https://openid.net/specs/openid-connect-core-1_0.html#id_tokenExample) +// Note that Apple doesn't have a UserInfo endpoint and claims about +// the users are included in the id_token (without the name - see #7090). func (p *Apple) FetchRawUserInfo(token *oauth2.Token) ([]byte, error) { idToken, _ := token.Extra("id_token").(string) @@ -108,18 +100,6 @@ func (p *Apple) FetchRawUserInfo(token *oauth2.Token) ([]byte, error) { return nil, err } - // Apple only returns the user object the first time the user authorizes the app - // https://developer.apple.com/documentation/sign_in_with_apple/sign_in_with_apple_js/configuring_your_webpage_for_sign_in_with_apple#3331292 - rawUser, _ := token.Extra("user").(string) - if rawUser != "" { - user := map[string]any{} - err = json.Unmarshal([]byte(rawUser), &user) - if err != nil { - return nil, err - } - claims["user"] = user - } - return json.Marshal(claims) }