From fc38e5637365758cbee98b48416a3af5fedc93b1 Mon Sep 17 00:00:00 2001 From: Michael Jerger Date: Tue, 14 May 2024 08:24:31 +0200 Subject: [PATCH] enhance test & fix reviews --- .deadcode-out | 4 +++ models/forgefed/federationhost.go | 6 ++-- models/forgefed/federationhost_repository.go | 8 ++--- models/forgefed/federationhost_test.go | 25 +++++++++++++- models/forgefed/nodeinfo.go | 2 +- models/forgefed/nodeinfo_test.go | 7 ++-- models/forgejo_migrations/migrate.go | 2 ++ models/forgejo_migrations/v15.go | 33 +++++++++++++++++++ modules/activitypub/client.go | 2 +- modules/forgefed/actor.go | 8 ++--- modules/forgefed/actor_test.go | 4 ++- .../api_activitypub_repository_test.go | 20 +++++++++-- 12 files changed, 101 insertions(+), 20 deletions(-) create mode 100644 models/forgejo_migrations/v15.go diff --git a/.deadcode-out b/.deadcode-out index 723b35791..9845b885a 100644 --- a/.deadcode-out +++ b/.deadcode-out @@ -131,6 +131,7 @@ package "code.gitea.io/gitea/models/user" func (ErrUserInactive).Unwrap func IsErrExternalLoginUserAlreadyExist func IsErrExternalLoginUserNotExist + func NewFederatedUser func IsErrUserSettingIsNotExist func GetUserAllSettings func DeleteUserSetting @@ -319,6 +320,9 @@ package "code.gitea.io/gitea/modules/translation" package "code.gitea.io/gitea/modules/util/filebuffer" func CreateFromReader +package "code.gitea.io/gitea/modules/validation" + func IsErrNotValid + package "code.gitea.io/gitea/modules/web" func RouteMock func RouteMockReset diff --git a/models/forgefed/federationhost.go b/models/forgefed/federationhost.go index eb1183a2a..b60c0c39c 100644 --- a/models/forgefed/federationhost.go +++ b/models/forgefed/federationhost.go @@ -19,11 +19,11 @@ type FederationHost struct { HostFqdn string `xorm:"host_fqdn UNIQUE INDEX VARCHAR(255) NOT NULL"` NodeInfo NodeInfo `xorm:"extends NOT NULL"` LatestActivity time.Time `xorm:"NOT NULL"` - Create timeutil.TimeStamp `xorm:"created"` + Created timeutil.TimeStamp `xorm:"created"` Updated timeutil.TimeStamp `xorm:"updated"` } -// Factory function for PersonID. Created struct is asserted to be valid +// Factory function for FederationHost. Created struct is asserted to be valid. func NewFederationHost(nodeInfo NodeInfo, hostFqdn string) (FederationHost, error) { result := FederationHost{ HostFqdn: strings.ToLower(hostFqdn), @@ -45,7 +45,7 @@ func (host FederationHost) Validate() []string { result = append(result, fmt.Sprintf("HostFqdn has to be lower case but was: %v", host.HostFqdn)) } if !host.LatestActivity.IsZero() && host.LatestActivity.After(time.Now().Add(10*time.Minute)) { - result = append(result, fmt.Sprintf("Latest Activity may not be far futurer: %v", host.LatestActivity)) + result = append(result, fmt.Sprintf("Latest Activity cannot be in the far future: %v", host.LatestActivity)) } return result diff --git a/models/forgefed/federationhost_repository.go b/models/forgefed/federationhost_repository.go index b4e72b0ce..03d8741c5 100644 --- a/models/forgefed/federationhost_repository.go +++ b/models/forgefed/federationhost_repository.go @@ -25,7 +25,7 @@ func GetFederationHost(ctx context.Context, ID int64) (*FederationHost, error) { return nil, fmt.Errorf("FederationInfo record %v does not exist", ID) } if res, err := validation.IsValid(host); !res { - return nil, fmt.Errorf("FederationInfo is not valid: %v", err) + return nil, err } return host, nil } @@ -39,14 +39,14 @@ func FindFederationHostByFqdn(ctx context.Context, fqdn string) (*FederationHost return nil, nil } if res, err := validation.IsValid(host); !res { - return nil, fmt.Errorf("FederationInfo is not valid: %v", err) + return nil, err } return host, nil } func CreateFederationHost(ctx context.Context, host *FederationHost) error { if res, err := validation.IsValid(host); !res { - return fmt.Errorf("FederationInfo is not valid: %v", err) + return err } _, err := db.GetEngine(ctx).Insert(host) return err @@ -54,7 +54,7 @@ func CreateFederationHost(ctx context.Context, host *FederationHost) error { func UpdateFederationHost(ctx context.Context, host *FederationHost) error { if res, err := validation.IsValid(host); !res { - return fmt.Errorf("FederationInfo is not valid: %v", err) + return err } _, err := db.GetEngine(ctx).ID(host.ID).Update(host) return err diff --git a/models/forgefed/federationhost_test.go b/models/forgefed/federationhost_test.go index 04f941d93..ea5494c6e 100644 --- a/models/forgefed/federationhost_test.go +++ b/models/forgefed/federationhost_test.go @@ -4,6 +4,7 @@ package forgefed import ( + "strings" "testing" "time" @@ -22,13 +23,35 @@ func Test_FederationHostValidation(t *testing.T) { t.Errorf("sut should be valid but was %q", err) } + sut = FederationHost{ + HostFqdn: "", + NodeInfo: NodeInfo{ + SoftwareName: "forgejo", + }, + LatestActivity: time.Now(), + } + if res, _ := validation.IsValid(sut); res { + t.Errorf("sut should be invalid: HostFqdn empty") + } + + sut = FederationHost{ + HostFqdn: strings.Repeat("fill", 64), + NodeInfo: NodeInfo{ + SoftwareName: "forgejo", + }, + LatestActivity: time.Now(), + } + if res, _ := validation.IsValid(sut); res { + t.Errorf("sut should be invalid: HostFqdn too long (len=256)") + } + sut = FederationHost{ HostFqdn: "host.do.main", NodeInfo: NodeInfo{}, LatestActivity: time.Now(), } if res, _ := validation.IsValid(sut); res { - t.Errorf("sut should be invalid") + t.Errorf("sut should be invalid: NodeInfo invalid") } sut = FederationHost{ diff --git a/models/forgefed/nodeinfo.go b/models/forgefed/nodeinfo.go index bb5657063..66d2eca7a 100644 --- a/models/forgefed/nodeinfo.go +++ b/models/forgefed/nodeinfo.go @@ -33,7 +33,7 @@ type NodeInfoWellKnown struct { Href string } -// Factory function for PersonID. Created struct is asserted to be valid +// Factory function for NodeInfoWellKnown. Created struct is asserted to be valid. func NewNodeInfoWellKnown(body []byte) (NodeInfoWellKnown, error) { result, err := NodeInfoWellKnownUnmarshalJSON(body) if err != nil { diff --git a/models/forgefed/nodeinfo_test.go b/models/forgefed/nodeinfo_test.go index ba1bd90be..4c73bb44d 100644 --- a/models/forgefed/nodeinfo_test.go +++ b/models/forgefed/nodeinfo_test.go @@ -6,6 +6,7 @@ package forgefed import ( "fmt" "reflect" + "strings" "testing" "code.gitea.io/gitea/modules/validation" @@ -52,12 +53,14 @@ func Test_NodeInfoWellKnownValidate(t *testing.T) { } sut = NodeInfoWellKnown{Href: "./federated-repo.prod.meissa.de/api/v1/nodeinfo"} - if _, err := validation.IsValid(sut); err.Error() != "Href has to be absolute\nValue is not contained in allowed values [http https]" { + _, err := validation.IsValid(sut) + if !validation.IsErrNotValid(err) && strings.Contains(err.Error(), "Href has to be absolute\nValue is not contained in allowed values [http https]") { t.Errorf("validation error expected but was: %v\n", err) } sut = NodeInfoWellKnown{Href: "https://federated-repo.prod.meissa.de/api/v1/nodeinfo?alert=1"} - if _, err := validation.IsValid(sut); err.Error() != "Href may not contain query" { + _, err = validation.IsValid(sut) + if !validation.IsErrNotValid(err) && strings.Contains(err.Error(), "Href has to be absolute\nValue is not contained in allowed values [http https]") { t.Errorf("sut should be valid, %v, %v", sut, err) } } diff --git a/models/forgejo_migrations/migrate.go b/models/forgejo_migrations/migrate.go index 3b6da7414..fc5a46016 100644 --- a/models/forgejo_migrations/migrate.go +++ b/models/forgejo_migrations/migrate.go @@ -66,6 +66,8 @@ var migrations = []*Migration{ NewMigration("Add `hide_archive_links` column to `release` table", AddHideArchiveLinksToRelease), // v14 -> v15 NewMigration("Remove Gitea-specific columns from the repository and badge tables", RemoveGiteaSpecificColumnsFromRepositoryAndBadge), + // v15 -> v16 + NewMigration("Create the `federation_host` table", CreateFederationHostTable), } // GetCurrentDBVersion returns the current Forgejo database version. diff --git a/models/forgejo_migrations/v15.go b/models/forgejo_migrations/v15.go new file mode 100644 index 000000000..d7ed19ca7 --- /dev/null +++ b/models/forgejo_migrations/v15.go @@ -0,0 +1,33 @@ +// Copyright 2024 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package forgejo_migrations //nolint:revive + +import ( + "time" + + "code.gitea.io/gitea/modules/timeutil" + + "xorm.io/xorm" +) + +type ( + SoftwareNameType string +) + +type NodeInfo struct { + SoftwareName SoftwareNameType +} + +type FederationHost struct { + ID int64 `xorm:"pk autoincr"` + HostFqdn string `xorm:"host_fqdn UNIQUE INDEX VARCHAR(255) NOT NULL"` + NodeInfo NodeInfo `xorm:"extends NOT NULL"` + LatestActivity time.Time `xorm:"NOT NULL"` + Created timeutil.TimeStamp `xorm:"created"` + Updated timeutil.TimeStamp `xorm:"updated"` +} + +func CreateFederationHostTable(x *xorm.Engine) error { + return x.Sync(new(FederationHost)) +} diff --git a/modules/activitypub/client.go b/modules/activitypub/client.go index 4962ad79a..d47990430 100644 --- a/modules/activitypub/client.go +++ b/modules/activitypub/client.go @@ -129,7 +129,7 @@ func (c *Client) Post(b []byte, to string) (resp *http.Response, err error) { } // Create an http GET request with forgejo/gitea specific headers -func (c *Client) Get(to string) (resp *http.Response, err error) { // ToDo: we might not need the b parameter +func (c *Client) Get(to string) (resp *http.Response, err error) { var req *http.Request emptyBody := []byte{0} if req, err = c.NewRequest(http.MethodGet, emptyBody, to); err != nil { diff --git a/modules/forgefed/actor.go b/modules/forgefed/actor.go index a34abc207..29d6f15d8 100644 --- a/modules/forgefed/actor.go +++ b/modules/forgefed/actor.go @@ -32,8 +32,8 @@ func NewActorID(uri string) (ActorID, error) { return ActorID{}, err } - if valid, outcome := validation.IsValid(result); !valid { - return ActorID{}, outcome + if valid, err := validation.IsValid(result); !valid { + return ActorID{}, err } return result, nil @@ -83,8 +83,8 @@ func NewPersonID(uri, source string) (PersonID, error) { // validate Person specific path personID := PersonID{result} - if valid, outcome := validation.IsValid(personID); !valid { - return PersonID{}, outcome + if valid, err := validation.IsValid(personID); !valid { + return PersonID{}, err } return personID, nil diff --git a/modules/forgefed/actor_test.go b/modules/forgefed/actor_test.go index 9a1dbd4c3..a3c01eceb 100644 --- a/modules/forgefed/actor_test.go +++ b/modules/forgefed/actor_test.go @@ -92,7 +92,9 @@ func TestPersonIdValidation(t *testing.T) { sut.Host = "an.other.host" sut.Port = "" sut.UnvalidatedInput = "https://an.other.host/path/1" - if _, err := validation.IsValid(sut); err.Error() != "path: \"path\" has to be a person specific api path" { + + _, err := validation.IsValid(sut) + if validation.IsErrNotValid(err) && strings.Contains(err.Error(), "path: \"path\" has to be a person specific api path\n") { t.Errorf("validation error expected but was: %v\n", err) } diff --git a/tests/integration/api_activitypub_repository_test.go b/tests/integration/api_activitypub_repository_test.go index e237ffeb1..67b18dac5 100644 --- a/tests/integration/api_activitypub_repository_test.go +++ b/tests/integration/api_activitypub_repository_test.go @@ -91,6 +91,22 @@ func TestActivityPubRepositoryInboxValid(t *testing.T) { `"openRegistrations":true,"usage":{"users":{"total":14,"activeHalfyear":2}},"metadata":{}}`) fmt.Fprint(res, responseBody) }) + federatedRoutes.HandleFunc("/api/v1/activitypub/user-id/2", + func(res http.ResponseWriter, req *http.Request) { + // curl -H "Accept: application/json" https://federated-repo.prod.meissa.de/api/v1/activitypub/user-id/2 + responseBody := fmt.Sprintf(`{"@context":["https://www.w3.org/ns/activitystreams","https://w3id.org/security/v1"],` + + `"id":"https://federated-repo.prod.meissa.de/api/v1/activitypub/user-id/2","type":"Person",` + + `"icon":{"type":"Image","mediaType":"image/png","url":"https://federated-repo.prod.meissa.de/avatars/1bb05d9a5f6675ed0272af9ea193063c"},` + + `"url":"https://federated-repo.prod.meissa.de/stargoose1","inbox":"https://federated-repo.prod.meissa.de/api/v1/activitypub/user-id/2/inbox",` + + `"outbox":"https://federated-repo.prod.meissa.de/api/v1/activitypub/user-id/2/outbox","preferredUsername":"stargoose1",` + + `"publicKey":{"id":"https://federated-repo.prod.meissa.de/api/v1/activitypub/user-id/2#main-key","owner":"https://federated-repo.prod.meissa.de/api/v1/activitypub/user-id/2",` + + `"publicKeyPem":"-----BEGIN PUBLIC KEY-----\nMIIBojANBgkqhkiG9w0BAQEFAAOCAY8AMIIBigKCAYEA18H5s7N6ItZUAh9tneII\nIuZdTTa3cZlLa/9ejWAHTkcp3WLW+/zbsumlMrWYfBy2/yTm56qasWt38iY4D6ul\n` + + `CPiwhAqX3REvVq8tM79a2CEqZn9ka6vuXoDgBg/sBf/BUWqf7orkjUXwk/U0Egjf\nk5jcurF4vqf1u+rlAHH37dvSBaDjNj6Qnj4OP12bjfaY/yvs7+jue/eNXFHjzN4E\n` + + `T2H4B/yeKTJ4UuAwTlLaNbZJul2baLlHelJPAsxiYaziVuV5P+IGWckY6RSerRaZ\nAkc4mmGGtjAyfN9aewe+lNVfwS7ElFx546PlLgdQgjmeSwLX8FWxbPE5A/PmaXCs\n` + + `nx+nou+3dD7NluULLtdd7K+2x02trObKXCAzmi5/Dc+yKTzpFqEz+hLNCz7TImP/\ncK//NV9Q+X67J9O27baH9R9ZF4zMw8rv2Pg0WLSw1z7lLXwlgIsDapeMCsrxkVO4\n` + + `LXX5AQ1xQNtlssnVoUBqBrvZsX2jUUKUocvZqMGuE4hfAgMBAAE=\n-----END PUBLIC KEY-----\n"}}`) + fmt.Fprint(res, responseBody) + }) federatedRoutes.HandleFunc("/", func(res http.ResponseWriter, req *http.Request) { t.Errorf("Unhandled request: %q", req.URL.EscapedPath()) @@ -126,9 +142,7 @@ func TestActivityPubRepositoryInboxValid(t *testing.T) { assert.NoError(t, err) assert.Equal(t, http.StatusNoContent, resp.StatusCode) - federationHost := unittest.AssertExistsAndLoadBean(t, &forgefed.FederationHost{ID: 1}) - assert.Equal(t, "127.0.0.1", federationHost.HostFqdn) - + unittest.AssertExistsAndLoadBean(t, &forgefed.FederationHost{HostFqdn: "127.0.0.1"}) }) }