From 72e527a47300cba70c0ab9600f5c061545d675dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Gryglicki?= Date: Tue, 6 Oct 2026 10:45:15 +0200 Subject: [PATCH 1/4] M3 org import: 403->unregistered + all-403 guard, registrable-domain matching, suggested_account and acs_roles columns, merged duplicate sets, post-register read-back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Łukasz Gryglicki Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot) Assisted by [Claude](https://claude.ai) --- cla-backend-go/cmd/org_import/README.md | 50 +-- cla-backend-go/cmd/org_import/main.go | 6 +- .../orgimport/account_forms_test.go | 2 +- cla-backend-go/orgimport/adapters.go | 23 ++ cla-backend-go/orgimport/audit.go | 144 +++++++-- cla-backend-go/orgimport/decisions.go | 36 ++- cla-backend-go/orgimport/decisions_test.go | 35 ++ cla-backend-go/orgimport/followup_test.go | 302 +++++++++++++++++ cla-backend-go/orgimport/manual.go | 1 + cla-backend-go/orgimport/orgimport.go | 51 +-- cla-backend-go/orgimport/report.go | 4 +- cla-backend-go/orgimport/report_test.go | 1 + cla-backend-go/orgimport/run.go | 95 +++++- cla-backend-go/orgimport/run_test.go | 86 ++++- cla-backend-go/orgimport/steps.go | 1 + cla-backend-go/orgimport/suggest.go | 305 ++++++++++++++++++ cla-backend-go/v2/member-service/client.go | 5 +- .../v2/member-service/client_test.go | 19 ++ 18 files changed, 1086 insertions(+), 80 deletions(-) create mode 100644 cla-backend-go/orgimport/followup_test.go create mode 100644 cla-backend-go/orgimport/suggest.go diff --git a/cla-backend-go/cmd/org_import/README.md b/cla-backend-go/cmd/org_import/README.md index 0abeab035..b95ca42e6 100644 --- a/cla-backend-go/cmd/org_import/README.md +++ b/cla-backend-go/cmd/org_import/README.md @@ -62,8 +62,9 @@ Safety rules: on the lfx-api resource server (the audience above) — without it the token request fails with `authorization failed (403): Client "…" is not authorized to access resource server` (current state on dev); (2) the client must be in the `global_org_admin` team (member-service Heimdall ruleset) for `POST /b2b_orgs` and have `auditor` for `GET /b2b_orgs/{id}` (dry-run liveness). - Check: `STAGE=dev bin/org-import ingest --routes register --ids ` must print `live=live`; `live=error` means the GET - is not permitted (never treated as dead) and the run exits 1. + Check: `STAGE=dev bin/org-import ingest --routes register --ids ` must print `live=live`. A 403 on one Account while + others answer 200 is `live=unregistered` (no b2b_org yet, §3.2); a 403 on every Account checked, or a 403 from the token endpoint, is + `live=error` (never treated as dead) and the run exits 1. - Nothing sensitive is ever written: report files contain company names/ids only; tokens stay in memory. ## 3. Commands @@ -72,7 +73,7 @@ Safety rules: org_import audit [--out-dir ./org-import-out] [report flags] org_import ingest [--apply] [--yes] [--tranche N] [--ids id1,id2] [--mapping map.csv] [--decisions decisions.csv] [--shared-domains domains.txt] [--state state.jsonl] [--routes register,rewrite] [--skip-wait] - [--use-apex] [--wait-max 40m] [--out-dir ./org-import-out] [report flags] + [--use-apex] [--register-unregistered] [--wait-max 40m] [--out-dir ./org-import-out] [report flags] report flags: [--email-to a@x,b@y] [--no-email] [--no-aws-log] [--aws-log-group /easycla/org-import/] ``` @@ -90,6 +91,7 @@ report flags: [--email-to a@x,b@y] [--no-email] [--no-aws-log] [--aws-log-gr | `--routes` | `register`, `rewrite` or both (default both) | | `--skip-wait` | check the new ids in org-service once instead of polling up to `--wait-max` | | `--use-apex` | resolve new ids via the Salesforce Apex endpoint (`ORG_IMPORT_USE_APEX=true` equivalent); refused when the SSM params are missing | +| `--register-unregistered` | also `POST /b2b_orgs` for Accounts whose GET answered 403 (`live=unregistered`); default: they stay `pending`/`unregistered` | | `--wait-max` | org-service propagation wait for new Salesforce accounts (default 40m, poll 30s) | | `--email-to` | report recipients, overrides SSM `cla-org-import-report-emails-{stage}` | | `--no-email` / `--no-aws-log` | do not send the report e-mail / do not copy run.log to CloudWatch Logs | @@ -112,16 +114,21 @@ export AWS_PROFILE=lfproduct-dev AWS_SDK_LOAD_CONFIG=1 STAGE=dev bin/org-import audit --out-dir ./org-import-out/dev-$(date -u +%F) ``` -Output line: `audit stage=dev companies=N eligible_groups=M MISSING_SFID=a INVALID_SFID_FORMAT=b SFID_OK=c SFID_DANGLING_OR_DELETED=d UNKNOWN=e register=r rewrite=w manual=m duplicates=k` +Output line: `audit stage=dev companies=N eligible_groups=M MISSING_SFID=a INVALID_SFID_FORMAT=b SFID_OK=c SFID_DANGLING_OR_DELETED=d UNKNOWN=e register=r rewrite=w manual=m unregistered=u duplicates=k` — the five tiers are 1:1 with `utils/audit_company_reachability.sh`; route counts are per row (only rows in eligible groups have a route). Files (`--out-dir`): -- `audit.csv` — `company_id, company_name, signing_entity_name, company_external_id, id_shape(001|lf|empty|other), active_ccla, ccla_count, ecla_count, org_service(200|404|err), website, duplicate_sfid_group, route, manual_reason, tier`. +- `audit.csv` — `company_id, company_name, signing_entity_name, company_external_id, id_shape(001|lf|empty|other), active_ccla, ccla_count, ecla_count, org_service(200|404|err), website, duplicate_sfid_group, route, manual_reason, tier, acs_roles, suggested_account`. `ecla_count` is filled only for active rows that are manual, duplicate or unresolvable and for every row of `possible_duplicates.csv` (cheap); `-1` = count failed. + `acs_roles` (`role=count;…` of the ACS grants scoped to the old id; `err` = listing failed) is filled for eligible `001`/`lf` groups and every + row of `possible_duplicates.csv`. `suggested_account` lists up to three existing Accounts a dead/legacy/manual/duplicate row could belong to: + ` [inventory:domain|inventory:name|crm:domain|crm:name]` (inventory = other Accounts served by org-service for the same import, + crm = org-service lookup by registrable website domain, then by name); candidates are verified in member-service — dropped Accounts are + omitted, unverifiable ones (403) carry a `?` suffix. Best effort: lookup failures only warn. - `unresolvable.csv` — active rows with empty/invalid ids or dead `001…` ids (#2749 input). - `possible_duplicates.csv` — candidate targets for the #3085 review: rows sharing an id with the same (or empty) signing entity name, and rows with the same normalized company name **or** the same org-service website domain under different ids (#2056 input). Shared/missing domains - (§4.2) never group; a set reached by both name and domain is listed once. Columns: `group, company_id, company_name, signing_entity_name, + (§4.2) never group; sets that overlap by name and domain are merged into one. Columns: `group, company_id, company_name, signing_entity_name, company_external_id, id_shape, domain, active_ccla, ccla_count, ecla_count` (`ecla_count` empty = not measured, `-1` = count failed). Distinct signing entities under one id are **not** duplicates. @@ -152,9 +159,11 @@ c0ffee00-... [manual] empty_external_id: Fill company_external_id (…) or leave stage=dev mode=dry-run eligible=4 registered=0 rewritten=0 pending=1 manual=1 failed=0 ``` -- `live=live|dead|unverified|error` — member-service `GET /b2b_orgs/{id}`; `unverified` = member-service not configured for the stage - (the group stays `pending`/`crm_unverified`, nothing is written); `error` is never treated as dead. -- `pending` — rewrite candidates without a resolved new id (no mapping row, or a `register` POST answered 404 = dead account). They appear in `to_salesforce.csv`. +- `live=live|dead|unregistered|unverified|error` — member-service `GET /b2b_orgs/{id}`; `unregistered` = 403 while other Accounts answer 200 + (the Account has no b2b_org yet; the group stays `pending`/`unregistered` unless `--register-unregistered`); `unverified` = member-service + not configured for the stage (the group stays `pending`/`crm_unverified`, nothing is written); `error` is never treated as dead. +- `pending` — rewrite candidates without a resolved new id (no mapping row, or a `register` POST answered 404 = dead account) and register + candidates that are `unregistered`/`crm_unverified`. Rewrite ones appear in `to_salesforce.csv`. - `skipped` — eligible groups excluded by `--routes`, `--tranche`, or already `done` in `--state`. - Summary line: `eligible` = groups after `--ids`; `registered`/`rewritten` = groups completed in apply mode; `manual`; `failed` = groups with an error — the run exits 1 in dry-run and apply mode alike (a dry run with `live=error` is not a clean dry run). @@ -162,17 +171,17 @@ stage=dev mode=dry-run eligible=4 registered=0 rewritten=0 pending=1 manual=1 fa Runtime: ~10 s on dev, ~7 min on prod (one liveness GET per group). Files (all rewritten after apply with the final state): -- `plan.csv` — `key, old_id, id_shape, route, manual_reason, live, org_service, website, domain, shared_domain, new_id, action, decision, reviewer, company_ids, company_names, error`. -- `manual_actions.csv` — one row per manual/pending/failed group with `reason` and `suggested_action` (what a human must do next). +- `plan.csv` — `key, old_id, id_shape, route, manual_reason, live, org_service, website, domain, shared_domain, new_id, action, decision, reviewer, company_ids, company_names, error, suggested_account`. +- `manual_actions.csv` — one row per manual/pending/failed group with `reason`, `suggested_action` (what a human must do next) and `suggested_account` (§3.1). - `targets.csv` — rewrite destinations grouped by Account: `target_sfid, groups, old_ids, company_ids, company_names, existing_rows, decision, reviewer, status(ok|needs_decision|distinct_conflict|target_forms_differ)`. -- `to_salesforce.csv` — `old_id, name, website, ccla_signed_date, domain, shared_domain` — the hand-off to sales ops (§4). +- `to_salesforce.csv` — `old_id, name, website, ccla_signed_date, domain, shared_domain, suggested_account` — the hand-off to sales ops (§4). Manual reasons: `empty_external_id`, `invalid_id_shape`, `mapping_ambiguous`, `mapping_not_approved`, `mapping_same_id` (also the 15/18-char form of the same Account), `sfid_alias_forms` (rows of one Account carry both its 15- and 18-char id — normalize them to one form first; §4), `target_forms_differ` (ids landing on one Account use both forms — normalize the mapping/rows first; a decision does not lift it), `target_collision` (several old ids → one Account, or the Account already has EasyCLA rows, and no decision covers it — §4.1), `distinct_conflict` (a `distinct` decision spans two ids resolved to the same Account), `missing_website` / `shared_domain` (Apex path only, §4.2), -`apex_match_needs_approval`, `apex_error`; pending reasons: `no_mapping`, `dead_account`, `crm_unverified` (no member-service for the stage). +`apex_match_needs_approval`, `apex_error`; pending reasons: `no_mapping`, `dead_account`, `crm_unverified` (no member-service for the stage), `unregistered` (GET answered 403; `--register-unregistered` registers it). With `--use-apex` a dry-run `created` has no Account id yet (`new_id=`): the id is assigned by the real call at apply and a different answer at apply time (`changed between dry run`) fails the group before any write; a failed resolution is never replayed as approved. @@ -252,10 +261,13 @@ distinct,lfcccc000000000000003;lfdddd000000000000004,,michal,different companies ### 4.2 Shared domains (`--shared-domains`) -Groups whose website domain is in the shared list (`github.com`, `nowebsite.com`, `gmail.com`, `googlemail.com`, `yahoo.com`, `hotmail.com`, -`outlook.com`, `live.com`, `icloud.com`, `protonmail.com`, `qq.com`, `163.com`) or who have no website are `manual` (`shared_domain` / -`missing_website`) instead of being domain-matched by Apex, and `audit` never groups them by domain in `possible_duplicates.csv`. A file -replaces the whole list (one domain per line, `#` comments). Mapping rows are explicit human decisions and are not gated. +The website domain is the registrable domain (public suffix list: `startup.google.com` → `google.com`, `comcast.github.io` stays as is). +Groups whose domain is in the shared list (`github.com`, `nowebsite.com`, `en.wikipedia.org`, `buymeacoffee.com`, `nonameaccount.com`, +`localhost.localhost`, `gmail.com`, `googlemail.com`, `yahoo.com`, `hotmail.com`, `outlook.com`, `live.com`, `icloud.com`, `protonmail.com`, +`qq.com`, `163.com`, `bund.de`, `onmicrosoft.com`) or who have no website are `manual` (`shared_domain` / `missing_website`) instead of being +domain-matched by Apex, and `audit` never groups them by domain in `possible_duplicates.csv`. A listed host and any subdomain of a listed +domain keep their full host as the domain (`digitalservice.bund.de`, `mainh.onmicrosoft.com`: distinct, not shared). A file replaces the whole +list (one domain per line, `#` comments). Mapping rows are explicit human decisions and are not gated. ## 5. Tranche protocol (prod) @@ -306,7 +318,9 @@ GitHub Actions `.github/workflows/org-import-sweep.yml`: | Where | Message | Meaning / action | |---|---|---| | setup | `loading SSM config` / `STAGE is not set` | wrong account/profile or missing stage | -| planning | `live=error` | member-service GET failed (403 = missing `auditor`, network) — fix access; nothing is classified dead | +| planning | `live=error` | member-service GET failed (403 on every Account = missing `auditor`/access tuple, token 403, network) — fix access; nothing is classified dead | +| planning | `live=unregistered` / `unregistered` | the Account answers 403 while others answer 200: no b2b_org yet; pending unless `--register-unregistered` | +| apply | `WARNING: b2b_org … not yet visible after 5 checks` | the POST succeeded but the GET still answers 403/404 (FGA tuples pending); re-check later, nothing to redo | | planning | `live=unverified` / `crm_unverified` | no member-service params for the stage; groups stay pending, nothing is registered | | apply | `rewrite apply requires --state` / `member-service is not configured` | refused before the first write; every planned group is reported as failed | | any step | `state file …: cannot record` | the journal could not be written; the group stops (rows are never rewritten before their `start` line) | diff --git a/cla-backend-go/cmd/org_import/main.go b/cla-backend-go/cmd/org_import/main.go index 2876b0287..6379c0f04 100644 --- a/cla-backend-go/cmd/org_import/main.go +++ b/cla-backend-go/cmd/org_import/main.go @@ -48,7 +48,7 @@ const usageText = `usage: org_import audit [--out-dir ./org-import-out] [report flags] org_import ingest [--apply] [--yes] [--tranche N] [--ids id1,id2] [--mapping map.csv] [--decisions decisions.csv] [--shared-domains domains.txt] [--state state.jsonl] [--routes register,rewrite] [--skip-wait] - [--use-apex] [--out-dir ./org-import-out] [report flags] + [--use-apex] [--register-unregistered] [--out-dir ./org-import-out] [report flags] report flags: [--email-to a@x,b@y] [--no-email] [--no-aws-log] [--aws-log-group /easycla/org-import/] Environment: STAGE=dev|prod (required), AWS credentials for that account (AWS_PROFILE/AWS_SDK_LOAD_CONFIG=1 @@ -112,6 +112,7 @@ func run(args []string, stdin io.Reader) int { routes := fs.String("routes", "register,rewrite", "routes to process: register,rewrite") skipWait := fs.Bool("skip-wait", false, "check new ids in org-service once instead of waiting up to 40 minutes") useApex := fs.Bool("use-apex", os.Getenv("ORG_IMPORT_USE_APEX") == trueString, "resolve new ids through the Salesforce Apex endpoint (needs cla-salesforce-apex-* SSM params)") + registerUnregistered := fs.Bool("register-unregistered", false, "also POST /b2b_orgs for Accounts whose GET /b2b_orgs answered 403 (unregistered; default: pending)") waitMax := fs.Duration("wait-max", 40*time.Minute, "maximum org-service propagation wait") var rf reportFlags fs.StringVar(&rf.emailTo, "email-to", "", "report recipients (comma-separated); default SSM cla-org-import-report-emails-") @@ -140,7 +141,7 @@ func run(args []string, stdin io.Reader) int { var err error opts := orgimport.Options{ Stage: stage, Apply: *apply, Tranche: *tranche, Mapping: *mapping, Decisions: *decisions, SharedDomains: *sharedDomains, - State: *state, SkipWait: *skipWait, UseApex: *useApex, OutDir: *outDir, WaitMax: *waitMax, + State: *state, SkipWait: *skipWait, UseApex: *useApex, RegisterUnregistered: *registerUnregistered, OutDir: *outDir, WaitMax: *waitMax, } if *ids != "" { opts.IDs = strings.Split(*ids, ",") @@ -475,6 +476,7 @@ func wire(ctx context.Context, stage string, useApex bool) (env, error) { ECLAs: orgimport.DynamoECLACounter{DB: dynamodb.New(awsSession), Table: fmt.Sprintf("cla-%s-signatures", stage)}, Events: events.NewRekeyRepository(awsSession, stage), Orgs: orgimport.OrgServiceAdapter{Client: organization_service.GetClient()}, + Lookup: orgimport.OrgServiceAdapter{Client: organization_service.GetClient()}, ACS: acs_service.GetClient(), } diff --git a/cla-backend-go/orgimport/account_forms_test.go b/cla-backend-go/orgimport/account_forms_test.go index 5b65f354f..378022b8c 100644 --- a/cla-backend-go/orgimport/account_forms_test.go +++ b/cla-backend-go/orgimport/account_forms_test.go @@ -259,7 +259,7 @@ func TestCleanupRefusesToDeleteGrantWithoutUsername(t *testing.T) { fx.platform.listHook = func(orgID string) { if orgID == lfID { if lists++; lists == 2 { - fx.platform.addGrant(lfID, "", "role-mgr", "cg-1") + fx.platform.addGrantLocked(lfID, "", "role-mgr", "cg-1") } } } diff --git a/cla-backend-go/orgimport/adapters.go b/cla-backend-go/orgimport/adapters.go index dc65492e6..baa6ac92b 100644 --- a/cla-backend-go/orgimport/adapters.go +++ b/cla-backend-go/orgimport/adapters.go @@ -37,6 +37,29 @@ func (a OrgServiceAdapter) GetOrganization(ctx context.Context, orgID string) (* return &Org{ID: org.ID, Name: org.Name, Website: org.Link, SigningEntityNames: org.SigningEntityName}, nil } +// LookupOrganization finds one Account by name or website domain (ErrOrgNotFound when none). +func (a OrgServiceAdapter) LookupOrganization(ctx context.Context, name, domain string) (*Org, error) { + var namePtr, domainPtr *string + if name != "" { + namePtr = &name + } + if domain != "" { + domainPtr = &domain + } + res, err := a.Client.SearchOrgLookup(ctx, namePtr, domainPtr) + if err != nil { + var notFound *organizations.LookupNotFound + if errors.As(err, ¬Found) { + return nil, ErrOrgNotFound + } + return nil, err + } + if res == nil || res.Payload == nil || res.Payload.ID == "" { + return nil, ErrOrgNotFound + } + return &Org{ID: res.Payload.ID, Name: res.Payload.Name, Website: res.Payload.Link}, nil +} + // CreateUserRoleScope grants a role scope by LFID username; an existing grant is not an error. func (a OrgServiceAdapter) CreateUserRoleScope(ctx context.Context, username, organizationID, objectType, objectID, roleID string) error { return a.Client.CreateOrgUserRoleScopeByUsername(ctx, username, organizationID, objectType, objectID, roleID) diff --git a/cla-backend-go/orgimport/audit.go b/cla-backend-go/orgimport/audit.go index 365984737..0dc3f107c 100644 --- a/cla-backend-go/orgimport/audit.go +++ b/cla-backend-go/orgimport/audit.go @@ -37,6 +37,9 @@ type AuditRow struct { Route Route ManualReason string Tier string + OrgName string + ACSRoles string + Suggested string } // AuditResult is the audit output; Duplicates are the candidate-target sets (rows sharing an id, @@ -48,6 +51,7 @@ type AuditResult struct { Routes map[Route]int Duplicates [][]*Row rowByID map[string]*AuditRow + shared SharedDomains } // Audit classifies every company row (reads only) and writes audit.csv, unresolvable.csv and @@ -66,6 +70,7 @@ func Audit(ctx context.Context, deps Deps, opts Options) (*AuditResult, error) { for _, g := range groups { classify(ctx, deps, g, nil, nil, Options{}) } + livenessGuard(groups) groupByRow := map[string]*Group{} for _, g := range groups { for _, r := range g.Rows { @@ -74,14 +79,16 @@ func Audit(ctx context.Context, deps Deps, opts Options) (*AuditResult, error) { } orgs := lookupOrgs(ctx, deps, inv) - res := &AuditResult{Tiers: map[string]int{}, Routes: map[Route]int{}, rowByID: map[string]*AuditRow{}} + res := &AuditResult{Tiers: map[string]int{}, Routes: map[Route]int{}, rowByID: map[string]*AuditRow{}, shared: shared} byName, byDomain := map[string][]*Row{}, map[string][]*Row{} + known := newAccountIndex() for _, row := range inv.Rows { ar := &AuditRow{Row: row, Shape: ShapeOf(row.ExternalID)} if o := orgs[row.ExternalID]; o != nil { ar.OrgStatus = o.status if o.org != nil { - ar.Website = o.org.Website + ar.Website, ar.OrgName = o.org.Website, o.org.Name + known.add(shared, row.ExternalID, o.org, row.CompanyName) } } ar.Tier = tier(row.ExternalID, ar.OrgStatus) @@ -106,6 +113,23 @@ func Audit(ctx context.Context, deps Deps, opts Options) (*AuditResult, error) { res.collectDuplicates(byName, byDomain) res.countECLAs(ctx, deps.ECLAs) + inDup := map[string]bool{} + for _, set := range res.Duplicates { + for _, r := range set { + inDup[r.CompanyID] = true + } + } + res.collectACSRoles(ctx, deps.ACS, inDup) + suggestRows(ctx, deps, shared, known, res.Rows, func(ar *AuditRow) bool { + if inDup[ar.Row.CompanyID] { + return true + } + g := groupByRow[ar.Row.CompanyID] + if g == nil || g.Err != nil { + return false + } + return g.Route == RouteManual || !(ar.Shape == ShapeSFID && ar.OrgStatus == "200" && g.Live != LiveDead) + }) if opts.OutDir != "" { if err = writeAuditReports(opts.OutDir, res); err != nil { @@ -121,35 +145,117 @@ func Audit(ctx context.Context, deps Deps, opts Options) (*AuditResult, error) { for _, t := range []string{TierMissing, TierInvalid, TierOK, TierDangling, TierUnknown} { fmt.Fprintf(deps.Out, " %s=%d", t, res.Tiers[t]) } - fmt.Fprintf(deps.Out, " register=%d rewrite=%d manual=%d duplicates=%d\n", res.Routes[RouteRegister], res.Routes[RouteRewrite], res.Routes[RouteManual], len(res.Duplicates)) + unregistered := 0 + for _, g := range groups { + if g.Live == LiveUnregistered { + unregistered++ + } + } + fmt.Fprintf(deps.Out, " register=%d rewrite=%d manual=%d unregistered=%d duplicates=%d\n", res.Routes[RouteRegister], res.Routes[RouteRewrite], res.Routes[RouteManual], unregistered, len(res.Duplicates)) return res, nil } +// collectACSRoles fills ACSRoles for the external ids of the eligible groups (001/lf) and of every +// row in a candidate set (4 workers); a failed listing shows as "err". +func (res *AuditResult) collectACSRoles(ctx context.Context, acs ACSService, inDup map[string]bool) { + if acs == nil { + return + } + want := map[string]bool{} + for _, g := range res.Groups { + if g.Shape == ShapeSFID || g.Shape == ShapeLF { + want[g.OldID] = true + } + } + for _, ar := range res.Rows { + if inDup[ar.Row.CompanyID] && (ar.Shape == ShapeSFID || ar.Shape == ShapeLF) { + want[ar.Row.ExternalID] = true + } + } + ids := make([]string, 0, len(want)) + for id := range want { + ids = append(ids, id) + } + sort.Strings(ids) + roles := make([]string, len(ids)) + runWorkers(len(ids), func(i int) { roles[i] = acsRoles(ctx, acs, ids[i]) }) + byID := make(map[string]string, len(ids)) + for i, id := range ids { + byID[id] = roles[i] + } + for _, ar := range res.Rows { + ar.ACSRoles = byID[ar.Row.ExternalID] + } +} + func (ar *AuditRow) unresolvable() bool { return ar.Shape == ShapeEmpty || ar.Shape == ShapeOther || (ar.Shape == ShapeSFID && ar.OrgStatus == "404") } // collectDuplicates builds the candidate-target sets: rows sharing an id (with the same or empty // signing entity), then rows with the same normalized name or the same non-shared domain under -// different ids. +// different ids. Overlapping sets are merged into one (union by company id). func (res *AuditResult) collectDuplicates(byName, byDomain map[string][]*Row) { + var sets [][]*Row for _, g := range res.Groups { if g.Duplicate { - res.addDuplicates(g.Rows) + sets = append(sets, g.Rows) } } - for _, sets := range []map[string][]*Row{byName, byDomain} { - keys := make([]string, 0, len(sets)) - for k := range sets { + for _, m := range []map[string][]*Row{byName, byDomain} { + keys := make([]string, 0, len(m)) + for k := range m { keys = append(keys, k) } sort.Strings(keys) for _, k := range keys { - if rows := sets[k]; k != "" && len(rows) >= 2 && distinctExternalIDs(rows) { - res.addDuplicates(rows) + if rows := m[k]; k != "" && len(rows) >= 2 && distinctExternalIDs(rows) { + sets = append(sets, rows) } } } + for _, set := range mergeOverlapping(sets) { + res.addDuplicates(set) + } +} + +// mergeOverlapping unions sets sharing a row; sets and rows keep their first-appearance order. +func mergeOverlapping(sets [][]*Row) [][]*Row { + parent := map[string]string{} + var find func(string) string + find = func(x string) string { + if parent[x] != x { + parent[x] = find(parent[x]) + } + return parent[x] + } + var order []*Row + seenRow := map[string]bool{} + for _, set := range sets { + for _, r := range set { + if _, ok := parent[r.CompanyID]; !ok { + parent[r.CompanyID] = r.CompanyID + } + if !seenRow[r.CompanyID] { + seenRow[r.CompanyID] = true + order = append(order, r) + } + parent[find(r.CompanyID)] = find(set[0].CompanyID) + } + } + index := map[string]int{} + var out [][]*Row + for _, r := range order { + root := find(r.CompanyID) + i, ok := index[root] + if !ok { + i = len(out) + index[root] = i + out = append(out, nil) + } + out[i] = append(out[i], r) + } + return out } // countECLAs fills ECLACount for the active manual/duplicate/unresolvable rows and for every row of a @@ -282,12 +388,12 @@ func writeAuditReports(dir string, res *AuditResult) error { if err := os.MkdirAll(dir, 0o750); err != nil { return err } - rows := [][]string{{"company_id", "company_name", "signing_entity_name", "company_external_id", "id_shape", "active_ccla", "ccla_count", "ecla_count", "org_service", "website", "duplicate_sfid_group", "route", "manual_reason", "tier"}} + rows := [][]string{{"company_id", "company_name", "signing_entity_name", "company_external_id", "id_shape", "active_ccla", "ccla_count", "ecla_count", "org_service", "website", "duplicate_sfid_group", "route", "manual_reason", "tier", "acs_roles", "suggested_account"}} var unresolvable [][]string unresolvable = append(unresolvable, []string{"company_id", "company_name", "signing_entity_name", "company_external_id", "id_shape", "ccla_count", "ecla_count", "org_service", "manual_reason"}) for _, ar := range res.Rows { r := ar.Row - rows = append(rows, []string{r.CompanyID, r.CompanyName, r.SigningEntityName, r.ExternalID, string(ar.Shape), strconv.FormatBool(r.ActiveCCLA), strconv.Itoa(r.CCLACount), strconv.Itoa(ar.ECLACount), ar.OrgStatus, ar.Website, strconv.FormatBool(ar.Duplicate), string(ar.Route), ar.ManualReason, ar.Tier}) + rows = append(rows, []string{r.CompanyID, r.CompanyName, r.SigningEntityName, r.ExternalID, string(ar.Shape), strconv.FormatBool(r.ActiveCCLA), strconv.Itoa(r.CCLACount), strconv.Itoa(ar.ECLACount), ar.OrgStatus, ar.Website, strconv.FormatBool(ar.Duplicate), string(ar.Route), ar.ManualReason, ar.Tier, ar.ACSRoles, ar.Suggested}) if r.ActiveCCLA && ar.unresolvable() { unresolvable = append(unresolvable, []string{r.CompanyID, r.CompanyName, r.SigningEntityName, r.ExternalID, string(ar.Shape), strconv.Itoa(r.CCLACount), strconv.Itoa(ar.ECLACount), ar.OrgStatus, ar.ManualReason}) } @@ -297,7 +403,7 @@ func writeAuditReports(dir string, res *AuditResult) error { for _, r := range set { shape, domain, ecla := string(ShapeOf(r.ExternalID)), "", "" if ar := res.rowByID[r.CompanyID]; ar != nil { - shape, domain = string(ar.Shape), Domain(ar.Website) + shape, domain = string(ar.Shape), res.shared.Domain(ar.Website) if ar.ECLACounted { ecla = strconv.Itoa(ar.ECLACount) } @@ -317,9 +423,9 @@ func writeIngestReports(dir string, plan *Plan) error { if err := os.MkdirAll(dir, 0o750); err != nil { return err } - rows := [][]string{{"key", "old_id", "id_shape", "route", "manual_reason", "live", "org_service", "website", "domain", "shared_domain", "new_id", "action", "decision", "reviewer", "company_ids", "company_names", "error"}} - toSF := [][]string{{"old_id", "name", "website", "ccla_signed_date", "domain", "shared_domain"}} - manual := [][]string{{"key", "old_id", "route", "reason", "suggested_action", "live", "org_service", "website", "domain", "shared_domain", "new_id", "action", "company_ids", "company_names", "error"}} + rows := [][]string{{"key", "old_id", "id_shape", "route", "manual_reason", "live", "org_service", "website", "domain", "shared_domain", "new_id", "action", "decision", "reviewer", "company_ids", "company_names", "error", "suggested_account"}} + toSF := [][]string{{"old_id", "name", "website", "ccla_signed_date", "domain", "shared_domain", "suggested_account"}} + manual := [][]string{{"key", "old_id", "route", "reason", "suggested_action", "live", "org_service", "website", "domain", "shared_domain", "new_id", "action", "company_ids", "company_names", "error", "suggested_account"}} for _, g := range plan.Groups { names := strings.Join(g.Names(), ";") errText := "" @@ -331,10 +437,10 @@ func writeIngestReports(dir string, plan *Plan) error { if g.Decision != nil { decision, reviewer = g.Decision.Kind, g.Decision.Reviewer } - rows = append(rows, []string{g.Key, g.OldID, string(g.Shape), string(g.Route), g.ManualReason, g.Live, g.OrgStatus, g.Website(), domain, shared, g.NewID, g.Action, decision, reviewer, strings.Join(g.CompanyIDs(), ";"), names, errText}) + rows = append(rows, []string{g.Key, g.OldID, string(g.Shape), string(g.Route), g.ManualReason, g.Live, g.OrgStatus, g.Website(), domain, shared, g.NewID, g.Action, decision, reviewer, strings.Join(g.CompanyIDs(), ";"), names, errText, g.Suggested}) if g.ManualReason == ReasonNoMapping || g.ManualReason == ReasonDeadAccount { req := apexRequest(g, true) - toSF = append(toSF, []string{g.OldID, req.Name, req.Website, req.CCLASignedDate, domain, shared}) + toSF = append(toSF, []string{g.OldID, req.Name, req.Website, req.CCLASignedDate, domain, shared, g.Suggested}) } } for _, a := range plan.ManualActions() { @@ -344,7 +450,7 @@ func writeIngestReports(dir string, plan *Plan) error { errText = g.Err.Error() } domain, shared := plan.domainOf(g) - manual = append(manual, []string{g.Key, g.OldID, string(g.Route), a.Reason, a.Suggested, g.Live, g.OrgStatus, g.Website(), domain, shared, g.NewID, g.Action, strings.Join(g.CompanyIDs(), ";"), strings.Join(g.Names(), ";"), errText}) + manual = append(manual, []string{g.Key, g.OldID, string(g.Route), a.Reason, a.Suggested, g.Live, g.OrgStatus, g.Website(), domain, shared, g.NewID, g.Action, strings.Join(g.CompanyIDs(), ";"), strings.Join(g.Names(), ";"), errText, g.Suggested}) } targets := [][]string{{"target_sfid", "groups", "old_ids", "company_ids", "company_names", "existing_rows", "decision", "reviewer", "status"}} for _, t := range plan.Targets { diff --git a/cla-backend-go/orgimport/decisions.go b/cla-backend-go/orgimport/decisions.go index c5d38e40c..8c2a13591 100644 --- a/cla-backend-go/orgimport/decisions.go +++ b/cla-backend-go/orgimport/decisions.go @@ -13,6 +13,8 @@ import ( "path/filepath" "sort" "strings" + + "golang.org/x/net/publicsuffix" ) // Decision kinds recorded by the duplicate review (lfx-self-serve #3085). @@ -282,8 +284,9 @@ type SharedDomains map[string]bool // DefaultSharedDomains is the built-in list; the #3085 review file replaces it (--shared-domains). var DefaultSharedDomains = []string{ - "github.com", "nowebsite.com", + "github.com", "nowebsite.com", "en.wikipedia.org", "buymeacoffee.com", "nonameaccount.com", "localhost.localhost", "gmail.com", "googlemail.com", "yahoo.com", "hotmail.com", "outlook.com", "live.com", "icloud.com", "protonmail.com", "qq.com", "163.com", + "bund.de", "onmicrosoft.com", } // LoadSharedDomains reads one domain per line (`#` comments); an empty path yields the default list. @@ -317,8 +320,14 @@ func LoadSharedDomains(path string) (SharedDomains, error) { return set, nil } -// Domain extracts the registrable host of a website value (scheme, path and www. stripped). +// Domain extracts the registrable domain of a website value (public suffix list; scheme, path and +// www. stripped): startup.google.com -> google.com, comcast.github.io stays (private suffix). func Domain(website string) string { + return registrable(hostOf(website)) +} + +// hostOf extracts the host of a website value (scheme, path and www. stripped). +func hostOf(website string) string { website = strings.TrimSpace(strings.ToLower(website)) if website == "" { return "" @@ -333,6 +342,27 @@ func Domain(website string) string { return normalizeDomain(u.Hostname()) } +func registrable(host string) string { + if host == "" { + return "" + } + if d, err := publicsuffix.EffectiveTLDPlusOne(host); err == nil { + return d + } + return host +} + +// Domain is the matching key of a website: its registrable domain, except that a listed shared +// parent (bund.de, onmicrosoft.com) keeps its subdomains apart. +func (s SharedDomains) Domain(website string) string { + host := hostOf(website) + reg := registrable(host) + if s[host] || (host != reg && s[reg]) { + return host + } + return reg +} + func normalizeDomain(host string) string { host = strings.TrimSpace(strings.ToLower(host)) host = strings.TrimPrefix(host, "http://") @@ -346,7 +376,7 @@ func normalizeDomain(host string) string { // Shared reports whether website has no domain or a listed shared domain; the reason is the manual reason. func (s SharedDomains) Shared(website string) (string, string) { - d := Domain(website) + d := s.Domain(website) switch { case d == "": return "", "missing_website" diff --git a/cla-backend-go/orgimport/decisions_test.go b/cla-backend-go/orgimport/decisions_test.go index bb4a0648a..725e6efe7 100644 --- a/cla-backend-go/orgimport/decisions_test.go +++ b/cla-backend-go/orgimport/decisions_test.go @@ -81,6 +81,41 @@ func TestSharedDomains(t *testing.T) { assert.Equal(t, "acme.example", d) assert.Empty(t, reason) + // registrable domain (public suffix list): hosts collapse to their registrable parent, private + // suffixes and unparsable hosts stay as they are + assert.Equal(t, "google.com", Domain("https://startup.google.com/x")) + assert.Equal(t, "ibm.com", Domain("research.ibm.com")) + assert.Equal(t, "harvard.edu", Domain("https://rc.fas.harvard.edu")) + assert.Equal(t, "comcast.github.io", Domain("https://comcast.github.io")) + assert.Equal(t, "economie.gouv.fr", Domain("economie.gouv.fr")) + assert.Equal(t, "localhost", Domain("localhost")) + assert.Equal(t, "127.0.0.1", Domain("http://127.0.0.1:8080/")) + assert.Equal(t, "acme.example", Domain("https://labs.acme.example")) + for _, want := range []string{"en.wikipedia.org", "buymeacoffee.com", "nonameaccount.com", "localhost.localhost", "bund.de", "onmicrosoft.com"} { + assert.True(t, def[want], want) + } + // a listed shared parent keeps its subdomains apart (digitalservice.bund.de is not bund.de) + d, reason = def.Shared("https://digitalservice.bund.de") + assert.Equal(t, "digitalservice.bund.de", d) + assert.Empty(t, reason) + d, reason = def.Shared("bund.de") + assert.Equal(t, "bund.de", d) + assert.Equal(t, "shared_domain", reason) + d, reason = def.Shared("https://mainh.onmicrosoft.com") + assert.Equal(t, "mainh.onmicrosoft.com", d) + assert.Empty(t, reason) + d, reason = def.Shared("https://en.wikipedia.org/wiki/x") + assert.Equal(t, "en.wikipedia.org", d) + assert.Equal(t, "shared_domain", reason) + d, reason = def.Shared("https://de.wikipedia.org") + assert.Equal(t, "wikipedia.org", d) + assert.Empty(t, reason) + d, reason = def.Shared("https://hr.163.com") + assert.Equal(t, "hr.163.com", d, "a subdomain of a listed parent is its own non-shared key") + assert.Empty(t, reason) + _, reason = def.Shared("localhost.localhost") + assert.Equal(t, "shared_domain", reason) + path := filepath.Join(t.TempDir(), "shared.txt") require.NoError(t, os.WriteFile(path, []byte("# review list\nWWW.Example.ORG # comment\nhttps://foo.test/\n\n"), 0o600)) custom, err := LoadSharedDomains(path) diff --git a/cla-backend-go/orgimport/followup_test.go b/cla-backend-go/orgimport/followup_test.go new file mode 100644 index 000000000..679f5989e --- /dev/null +++ b/cla-backend-go/orgimport/followup_test.go @@ -0,0 +1,302 @@ +// Copyright The Linux Foundation and each contributor to CommunityBridge. +// SPDX-License-Identifier: MIT + +package orgimport + +import ( + "context" + "errors" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + member_service "github.com/linuxfoundation/easycla/cla-backend-go/v2/member-service" +) + +const ( + unregSFID = "0014100000UnregAAA" + unregSFID2 = "0014100000FreshBBB" +) + +// A 403 on GET /b2b_orgs while other Accounts answer 200 means "no b2b_org yet", not an error. +func TestUnregisteredAccountsArePending(t *testing.T) { + fx := newFixture() + fx.company("c-live", "Live Corp", "", liveSFID, "cg-1") + fx.company("c-unreg", "Fresh Corp", "", unregSFID, "cg-1") + fx.platform.sfAccounts[liveSFID] = true + fx.platform.sfAccounts[unregSFID] = true + fx.platform.forbidden = map[string]int{unregSFID: -1} + fx.platform.orgs[unregSFID] = &Org{ID: unregSFID, Name: "Fresh Corp", Website: "https://fresh.example"} + + plan, err := BuildPlan(context.Background(), fx.deps(), Options{Apply: true}) + require.NoError(t, err) + g := groupByKey(plan.Groups, unregSFID) + require.NotNil(t, g) + assert.Equal(t, "unregistered", g.Live) + assert.Equal(t, RouteRegister, g.Route) + assert.Equal(t, "unregistered", g.ManualReason) + assert.NoError(t, g.Err) + assert.True(t, g.Pending()) + assert.Equal(t, "200", g.OrgStatus, "org-service is still consulted for the report") + require.Len(t, plan.Register, 1) + assert.Equal(t, liveSFID, plan.Register[0].OldID) + + sum, err := Execute(context.Background(), fx.deps(), Options{Apply: true}, plan) + require.NoError(t, err) + assert.Equal(t, Summary{Mode: "apply", Eligible: 2, Registered: 1, Pending: 1}, sum) + assert.Equal(t, []string{liveSFID}, fx.platform.registered) + actions := plan.ManualActions() + require.Len(t, actions, 1) + assert.Equal(t, "unregistered", actions[0].Reason) + assert.Contains(t, actions[0].Suggested, "--register-unregistered") + + dir := t.TempDir() + fx.out.Reset() + res, err := Audit(context.Background(), fx.deps(), Options{Stage: "dev", OutDir: dir}) + require.NoError(t, err) + assert.Equal(t, map[Route]int{RouteRegister: 2}, res.Routes) + rows := readCSV(t, filepath.Join(dir, "audit.csv")) + for _, r := range rows[1:] { + if r[0] == "c-unreg" { + assert.Equal(t, "unregistered", r[12]) + } + } + assert.Contains(t, fx.out.String(), " unregistered=1 ") +} + +// When every GET answers 403 the client cannot see any b2b_org at all: that is the old error shape. +func TestAllForbiddenIsAnAccessError(t *testing.T) { + fx := newFixture() + fx.company("c-a", "A", "", unregSFID, "cg-1") + fx.company("c-b", "B", "", unregSFID2, "cg-1") + fx.company("c-dead", "Dead Corp", "", deadSFID, "cg-1") + fx.platform.sfAccounts[unregSFID] = true + fx.platform.sfAccounts[unregSFID2] = true + fx.platform.forbidden = map[string]int{unregSFID: -1, unregSFID2: -1, deadSFID: -1} + + plan, err := BuildPlan(context.Background(), fx.deps(), Options{Apply: true}) + require.NoError(t, err) + for _, key := range []string{unregSFID, unregSFID2, deadSFID} { + g := groupByKey(plan.Groups, key) + require.NotNil(t, g, key) + assert.Equal(t, LiveError, g.Live) + assert.Empty(t, g.Route) + assert.Empty(t, g.ManualReason) + require.Error(t, g.Err) + assert.ErrorContains(t, g.Err, "every Account checked (3)") + var authErr *member_service.AuthError + assert.ErrorAs(t, g.Err, &authErr) + assert.Contains(t, SuggestError(g.Err), "auditor") + } + assert.Empty(t, plan.Register) + sum, err := Execute(context.Background(), fx.deps(), Options{Apply: true}, plan) + assert.Error(t, err) + assert.Equal(t, 3, sum.Failed) + assert.Empty(t, fx.platform.registered) + + dir := t.TempDir() + fx.out.Reset() + _, err = Audit(context.Background(), fx.deps(), Options{Stage: "dev", OutDir: dir}) + require.NoError(t, err) + rows := readCSV(t, filepath.Join(dir, "audit.csv")) + for _, r := range rows[1:] { + assert.Contains(t, r[12], "every Account checked (3)", r[0]) + } + assert.Contains(t, fx.out.String(), " unregistered=0 ") +} + +// suggested_account: EasyCLA-internal candidates (same registrable domain, then same normalized +// name) among Accounts served by org-service, verified through member-service. +func TestAuditSuggestedAccountFromInventory(t *testing.T) { + const ( + oldSFID = "0014100000OldAAAAA" + old2SFID = "0014100000OldBBBBB" + newSFID = "0014100000NewCCCCC" + ) + fx := newFixture() + fx.company("c-old", "Acme", "", oldSFID, "cg-1") // dead, same name as the live Acme + fx.company("c-old2", "Widgets", "", old2SFID, "cg-1") // dead, website under acme.example + fx.company("c-new", "Acme", "", newSFID, "cg-1") // live Account + fx.company("c-lf", "Acme", "", "lf-acme-legacy-01", "cg-1") // lf id, same name + fx.company("c-unrelated", "Zeta", "", liveSFID2, "cg-1") + fx.platform.sfAccounts[newSFID] = true + fx.platform.sfAccounts[liveSFID2] = true + fx.platform.orgs[newSFID] = &Org{ID: newSFID, Name: "Acme Inc.", Website: "https://www.acme.example"} + fx.platform.orgs[old2SFID] = &Org{ID: old2SFID, Name: "Widgets", Website: "https://labs.acme.example/x"} + fx.platform.orgs[liveSFID2] = &Org{ID: liveSFID2, Name: "Zeta", Website: "https://zeta.example"} + + dir := t.TempDir() + res, err := Audit(context.Background(), fx.deps(), Options{Stage: "dev", OutDir: dir}) + require.NoError(t, err) + rows := readCSV(t, filepath.Join(dir, "audit.csv")) + byID := map[string][]string{} + for _, r := range rows[1:] { + byID[r[0]] = r + } + assert.Equal(t, newSFID+" Acme Inc. [inventory:name]", byID["c-old"][15]) + assert.Equal(t, newSFID+" Acme Inc. [inventory:domain]", byID["c-old2"][15]) + assert.Equal(t, newSFID+" Acme Inc. [inventory:name]", byID["c-lf"][15]) + assert.Equal(t, "", byID["c-new"][15], "a live Account never suggests itself") + assert.Equal(t, "", byID["c-unrelated"][15]) + assert.NotEmpty(t, res.Duplicates) + assert.Empty(t, fx.platform.registered) +} + +// --register-unregistered POSTs the 403 Accounts too and re-reads each one until Heimdall serves it. +func TestRegisterUnregisteredRetriesTheReadBack(t *testing.T) { + fx := newFixture() + fx.company("c-live", "Live Corp", "", liveSFID, "cg-1") + fx.company("c-unreg", "Fresh Corp", "", unregSFID, "cg-1") + fx.platform.sfAccounts[liveSFID] = true + fx.platform.sfAccounts[unregSFID] = true + fx.platform.forbidden = map[string]int{unregSFID: -1} + opts := Options{Apply: true, RegisterUnregistered: true} + + plan, err := BuildPlan(context.Background(), fx.deps(), opts) + require.NoError(t, err) + g := groupByKey(plan.Groups, unregSFID) + require.NotNil(t, g) + assert.Equal(t, "unregistered", g.Live) + assert.Equal(t, RouteRegister, g.Route) + assert.Empty(t, g.ManualReason) + assert.False(t, g.Pending()) + require.Len(t, plan.Register, 2) + + fx.platform.forbidden[unregSFID] = 2 + sum, err := Execute(context.Background(), fx.deps(), opts, plan) + require.NoError(t, err) + assert.Equal(t, Summary{Mode: "apply", Eligible: 2, Registered: 2}, sum) + assert.ElementsMatch(t, []string{liveSFID, unregSFID}, fx.platform.registered) + assert.Equal(t, []time.Duration{3 * time.Second, 3 * time.Second}, fx.sleeps, "two 403 answers, then served") + assert.Contains(t, fx.out.String(), "b2b_org "+unregSFID+" visible after 3 check(s)") + assert.NotContains(t, fx.out.String(), "b2b_org "+liveSFID+" visible", "an Account that was already live is not re-read") + + fx = newFixture() + fx.company("c-unreg", "Fresh Corp", "", unregSFID, "cg-1") + fx.company("c-live", "Live Corp", "", liveSFID, "cg-1") + fx.platform.sfAccounts[liveSFID] = true + fx.platform.sfAccounts[unregSFID] = true + fx.platform.forbidden = map[string]int{unregSFID: -1} + plan, err = BuildPlan(context.Background(), fx.deps(), opts) + require.NoError(t, err) + before := len(fx.platform.calls) + sum, err = Execute(context.Background(), fx.deps(), opts, plan) + require.NoError(t, err) + assert.Equal(t, 2, sum.Registered, "a pending read-back never fails the registration") + assert.Len(t, fx.sleeps, 4) + gets := 0 + for _, c := range fx.platform.calls[before:] { + if c == "get-b2b:"+unregSFID { + gets++ + } + } + assert.Equal(t, 5, gets) + assert.Contains(t, fx.out.String(), "WARNING: b2b_org "+unregSFID+" not yet visible after 5 checks") +} + +// A 403 from the Auth0 token endpoint is a credentials problem, never "unregistered". +func TestTokenForbiddenIsALivenessError(t *testing.T) { + fx := newFixture() + fx.company("c-live", "Live Corp", "", liveSFID, "cg-1") + fx.platform.sfAccounts[liveSFID] = true + fx.platform.tokenDenied = true + plan, err := BuildPlan(context.Background(), fx.deps(), Options{Apply: true, RegisterUnregistered: true}) + require.NoError(t, err) + g := groupByKey(plan.Groups, liveSFID) + require.NotNil(t, g) + assert.Equal(t, LiveError, g.Live) + assert.Empty(t, g.Route) + require.Error(t, g.Err) + assert.NotContains(t, g.Err.Error(), "every Account checked") + assert.Empty(t, plan.Register) + assert.Zero(t, plan.Unregistered()) + sum, err := Execute(context.Background(), fx.deps(), Options{Apply: true, RegisterUnregistered: true}, plan) + assert.Error(t, err) + assert.Equal(t, 1, sum.Failed) + assert.Empty(t, fx.platform.registered) +} + +// suggested_account: org-service lookups by domain and by name fill in after the inventory; an +// unverifiable candidate (403) is kept with a "?" tag and a lookup failure only warns. +func TestSuggestedAccountFromCRM(t *testing.T) { + const ( + oldSFID = "0014100000OldAAAAA" + crmSFID = "0014100000CrmAAAAA" + crmSFID2 = "0014100000CrmBBBBB" + ) + fx := newFixture() + fx.company("c-old", "Acme", "", oldSFID, "cg-1") + fx.company("c-live", "Live Corp", "", liveSFID, "cg-1") + fx.platform.sfAccounts[liveSFID] = true + fx.platform.sfAccounts[crmSFID] = true + fx.platform.forbidden = map[string]int{crmSFID2: -1} + fx.platform.orgs[oldSFID] = &Org{ID: oldSFID, Name: "Acme", Website: "https://www.old.example/about"} + fx.platform.orgs[liveSFID] = &Org{ID: liveSFID, Name: "Live Corp", Website: "https://live.example"} + fx.platform.lookups = map[string]*Org{ + "domain:old.example": {ID: crmSFID, Name: "Old Example Inc", Website: "https://old.example"}, + "name:Acme": {ID: crmSFID2, Name: "Acme Holdings"}, + "domain:live.example": {ID: liveSFID, Name: "Live Corp"}, + } + + dir := t.TempDir() + _, err := Audit(context.Background(), fx.deps(), Options{Stage: "dev", OutDir: dir}) + require.NoError(t, err) + rows := readCSV(t, filepath.Join(dir, "audit.csv")) + byID := map[string][]string{} + for _, r := range rows[1:] { + byID[r[0]] = r + } + assert.Equal(t, crmSFID+" Old Example Inc [crm:domain]; "+crmSFID2+" Acme Holdings [crm:name?]", byID["c-old"][15]) + assert.Equal(t, "", byID["c-live"][15], "a live, registered Account gets no suggestion") + assert.Contains(t, fx.platform.lookupCalls, "domain:old.example") + assert.Contains(t, fx.platform.lookupCalls, "name:Acme") + assert.NotContains(t, fx.platform.lookupCalls, "domain:live.example") + assert.NotContains(t, fx.out.String(), "WARNING: account lookup") + + fx.out.Reset() + dir = t.TempDir() + opts := Options{Stage: "dev", OutDir: dir} + plan, err := BuildPlan(context.Background(), fx.deps(), opts) + require.NoError(t, err) + _, err = Execute(context.Background(), fx.deps(), opts, plan) + require.NoError(t, err) + toSF := readCSV(t, filepath.Join(dir, "to_salesforce.csv")) + require.Len(t, toSF, 2) + assert.Equal(t, "suggested_account", toSF[0][len(toSF[0])-1]) + assert.Equal(t, oldSFID, toSF[1][0]) + assert.Equal(t, crmSFID+" Old Example Inc [crm:domain]; "+crmSFID2+" Acme Holdings [crm:name?]", toSF[1][len(toSF[1])-1]) + manual := readCSV(t, filepath.Join(dir, "manual_actions.csv")) + require.NotEmpty(t, manual) + assert.Equal(t, "suggested_account", manual[0][len(manual[0])-1]) + + fx = newFixture() + fx.company("c-old", "Acme", "", oldSFID, "cg-1") + fx.platform.orgs[oldSFID] = &Org{ID: oldSFID, Name: "Acme", Website: "https://old.example"} + fx.platform.lookupErr = errors.New("org-service 500") + dir = t.TempDir() + _, err = Audit(context.Background(), fx.deps(), Options{Stage: "dev", OutDir: dir}) + require.NoError(t, err) + rows = readCSV(t, filepath.Join(dir, "audit.csv")) + require.Len(t, rows, 2) + assert.Equal(t, "", rows[1][15]) + assert.Equal(t, 2, strings.Count(fx.out.String(), "WARNING: account lookup"), "one warning per lookup key (domain, name)") +} + +// acs_roles: a per-org ACS failure is reported in the cell, not as an audit error. +func TestAuditACSRolesError(t *testing.T) { + fx := newFixture() + fx.company("c-live", "Live Corp", "", liveSFID, "cg-1") + fx.platform.sfAccounts[liveSFID] = true + fx.platform.orgs[liveSFID] = &Org{ID: liveSFID, Name: "Live Corp", Website: "https://live.example"} + fx.platform.grantsErr = map[string]error{liveSFID: errors.New("acs down")} + dir := t.TempDir() + _, err := Audit(context.Background(), fx.deps(), Options{Stage: "dev", OutDir: dir}) + require.NoError(t, err) + rows := readCSV(t, filepath.Join(dir, "audit.csv")) + require.Len(t, rows, 2) + assert.Equal(t, "err", rows[1][14]) +} diff --git a/cla-backend-go/orgimport/manual.go b/cla-backend-go/orgimport/manual.go index 1a4e86515..36cd51d3b 100644 --- a/cla-backend-go/orgimport/manual.go +++ b/cla-backend-go/orgimport/manual.go @@ -32,6 +32,7 @@ var suggestions = map[string]string{ "missing_website": "Org-service has no website for the organization: automatic domain match is impossible — add a mapping row after sales ops name the Account.", "shared_domain": "Website domain is shared (github.com, nowebsite.com, mail provider): never matched automatically — add a mapping row after sales ops name the Account.", "apex_error": "Apex call failed: re-run; if it persists check cla-salesforce-apex-* and the endpoint.", + ReasonUnregistered: "Account exists in Salesforce but has no b2b_org yet (GET /b2b_orgs answered 403): re-run with --register-unregistered to POST /b2b_orgs for it, or wait for the member-service access tuple.", ReasonCRMUnverified: "Account liveness cannot be verified without member-service: set SSM cla-member-service-base-url- / cla-member-service-auth0-audience- and the Auth0 client grant + Heimdall roles (README §2), then re-run.", } diff --git a/cla-backend-go/orgimport/orgimport.go b/cla-backend-go/orgimport/orgimport.go index a18ee2065..7dc584d7e 100644 --- a/cla-backend-go/orgimport/orgimport.go +++ b/cla-backend-go/orgimport/orgimport.go @@ -45,10 +45,11 @@ const ( // Liveness values (Group.Live), run modes and the manual/pending reasons shared by several files. const ( - LiveLive = "live" - LiveDead = "dead" - LiveError = "error" - LiveUnverified = "unverified" + LiveLive = "live" + LiveDead = "dead" + LiveError = "error" + LiveUnverified = "unverified" + LiveUnregistered = "unregistered" ModeApply = "apply" ModeDryRun = "dry-run" @@ -60,6 +61,7 @@ const ( ReasonSharedDomain = "shared_domain" ReasonDistinctConflict = "distinct_conflict" ReasonCRMUnverified = "crm_unverified" + ReasonUnregistered = "unregistered" ReasonSFIDAliasForms = "sfid_alias_forms" ReasonTargetFormsDiffer = "target_forms_differ" @@ -137,12 +139,17 @@ type MemberService interface { RegisterB2BOrg(ctx context.Context, sfid string) (*member_service.B2BOrg, error) } +// OrgLookup finds one Account by website domain or by name in org-service (ErrOrgNotFound when none). +type OrgLookup interface { + LookupOrganization(ctx context.Context, name, domain string) (*Org, error) +} + // ApexService is the Salesforce find-or-create contract (design §4), used only with --use-apex. type ApexService interface { FindOrCreate(ctx context.Context, req ApexRequest) (*ApexResult, error) } -// Deps are the external dependencies; Members, Events, ECLAs and Apex may be nil. +// Deps are the external dependencies; Members, Events, ECLAs, Lookup and Apex may be nil. type Deps struct { Companies CompanyStore Signatures CCLALister @@ -151,6 +158,7 @@ type Deps struct { Orgs OrgService ACS ACSService Members MemberService + Lookup OrgLookup Apex ApexService Out io.Writer Now func() time.Time @@ -159,20 +167,21 @@ type Deps struct { // Options are the ingest/audit flags. type Options struct { - Stage string - Apply bool - Tranche int - IDs []string - Mapping string - Decisions string - SharedDomains string - State string - Routes []Route - SkipWait bool - UseApex bool - OutDir string - WaitMax time.Duration - WaitPoll time.Duration + Stage string + Apply bool + Tranche int + IDs []string + Mapping string + Decisions string + SharedDomains string + State string + Routes []Route + SkipWait bool + UseApex bool + RegisterUnregistered bool + OutDir string + WaitMax time.Duration + WaitPoll time.Duration } // Row is one companies-table row; ExternalID is trimmed for classification, RawExternalID is the @@ -207,7 +216,9 @@ type Group struct { Decision *Decision Replayed bool ViaApex bool + Suggested string Err error + liveErr error } // Summary is the one-line run result. @@ -233,7 +244,7 @@ func (s Summary) String() string { // Account is created by Apex at apply time is not pending. func (g *Group) Pending() bool { if g.Route == RouteRegister { - return g.ManualReason == ReasonCRMUnverified + return g.ManualReason == ReasonCRMUnverified || g.ManualReason == ReasonUnregistered } return g.Route == RouteRewrite && g.NewID == "" && !g.ApexCreates() } diff --git a/cla-backend-go/orgimport/report.go b/cla-backend-go/orgimport/report.go index 9979d2108..cb55fa650 100644 --- a/cla-backend-go/orgimport/report.go +++ b/cla-backend-go/orgimport/report.go @@ -193,7 +193,7 @@ func writePlanSections(h, t *strings.Builder, info RunInfo, plan *Plan) { t.WriteString("None.\n") } else { h.WriteString("

Each row needs a human before the tool can act on it; the suggested action says how.

") - writeTable(h, t, []string{"key", "old id", "route", "reason", "suggested action", "live", "org-service", "website", "domain", "new id", "action", "company ids", "company names", "error"}, manualRows(plan, actions)) + writeTable(h, t, []string{"key", "old id", "route", "reason", "suggested action", "live", "org-service", "website", "domain", "new id", "action", "company ids", "company names", "error", "suggested account"}, manualRows(plan, actions)) } if len(plan.Targets) > 0 { @@ -256,7 +256,7 @@ func manualRows(plan *Plan, actions []ManualAction) [][]string { errText = g.Err.Error() } domain, _ := plan.domainOf(g) - rows = append(rows, []string{g.Key, g.OldID, string(g.Route), a.Reason, a.Suggested, g.Live, g.OrgStatus, g.Website(), domain, g.NewID, g.Action, strings.Join(g.CompanyIDs(), "; "), strings.Join(g.Names(), "; "), errText}) + rows = append(rows, []string{g.Key, g.OldID, string(g.Route), a.Reason, a.Suggested, g.Live, g.OrgStatus, g.Website(), domain, g.NewID, g.Action, strings.Join(g.CompanyIDs(), "; "), strings.Join(g.Names(), "; "), errText, g.Suggested}) } return rows } diff --git a/cla-backend-go/orgimport/report_test.go b/cla-backend-go/orgimport/report_test.go index 7c61194e1..027e4be17 100644 --- a/cla-backend-go/orgimport/report_test.go +++ b/cla-backend-go/orgimport/report_test.go @@ -66,6 +66,7 @@ func TestBuildReport(t *testing.T) { "How to apply this plan", "STAGE=dev ./bin/org-import ingest --mapping", "--apply --yes", "gh workflow run org-import-sweep.yml -R linuxfoundation/easycla -f stage=dev -f mode=apply -f routes=rewrite", "-f mapping="$(tr '\\n' '|' < ", "https://github.com/linuxfoundation/easycla/actions/runs/42", "org-import-out-dev-42", "abc123-dirty", "/easycla/org-import/dev / s1", "1m30s", "line 2", "Attachments", "manual_actions.csv", "plan.csv", "targets.csv", "run.log", "out.zip", "acme.example", + "suggested account", } { assert.Contains(t, rep.HTML, want, want) } diff --git a/cla-backend-go/orgimport/run.go b/cla-backend-go/orgimport/run.go index 9d5d1544a..f0cd5404a 100644 --- a/cla-backend-go/orgimport/run.go +++ b/cla-backend-go/orgimport/run.go @@ -59,8 +59,13 @@ func BuildPlan(ctx context.Context, deps Deps, opts Options) (*Plan, error) { } classify(ctx, deps, g, mapping, shared, opts) } + livenessGuard(groups) targets := applyDecisions(groups, inv, mapping, decisions) + known := knownAccountsFromGroups(shared, groups) groups = filterIDs(groups, opts.IDs) + suggestAccounts(ctx, deps, shared, known, groups, func(g *Group) bool { + return g.Err == nil && (g.Route == RouteRewrite || g.Route == RouteManual) + }) plan := &Plan{Stage: opts.Stage, Apply: opts.Apply, Groups: groups, Targets: targets, mapping: mapping, decisions: decisions, shared: shared, state: state, inv: inv} budget := opts.Tranche @@ -155,6 +160,17 @@ func (p *Plan) Pending() int { return n } +// Unregistered counts the Accounts whose GET /b2b_orgs answered 403 (no b2b_org yet). +func (p *Plan) Unregistered() int { + n := 0 + for _, g := range p.Groups { + if g.Live == LiveUnregistered { + n++ + } + } + return n +} + func routeSet(routes []Route) map[Route]bool { set := map[Route]bool{} for _, r := range routes { @@ -240,6 +256,13 @@ func classify(ctx context.Context, deps Deps, g *Group, mapping *Mapping, shared g.Route, g.ManualReason = RouteRegister, ReasonCRMUnverified lookupOrg(ctx, deps, g) return + case LiveUnregistered: + g.Route, g.liveErr = RouteRegister, liveErr + if !opts.RegisterUnregistered { + g.ManualReason = ReasonUnregistered + } + lookupOrg(ctx, deps, g) + return case LiveDead: g.Route = RouteRewrite default: @@ -294,19 +317,47 @@ func classifyRowTargeted(g *Group, mapping *Mapping) { } // liveness asks member-service (the CRM view) whether the Account exists; without member-service -// the answer is unverified — org-service may still serve an Account deleted from Salesforce. +// the answer is unverified — org-service may still serve an Account deleted from Salesforce. A 403 +// from member-service itself means the Account has no b2b_org yet (unregistered). func liveness(ctx context.Context, deps Deps, id string) (string, error) { if deps.Members == nil { return LiveUnverified, nil } _, err := deps.Members.GetB2BOrg(ctx, id) - if errors.Is(err, member_service.ErrOrgNotFound) { + var authErr *member_service.AuthError + switch { + case err == nil: + return LiveLive, nil + case errors.Is(err, member_service.ErrOrgNotFound): return LiveDead, nil + case errors.As(err, &authErr) && authErr.Status == 403 && !authErr.Token: + return LiveUnregistered, err } - if err != nil { - return LiveError, err + return LiveError, err +} + +// livenessGuard turns "unregistered" back into the access error when no Account at all answered +// 200 or 404: the client then cannot see any b2b_org (missing access tuple), not just these ones. +func livenessGuard(groups []*Group) { + seen, unregistered := 0, 0 + for _, g := range groups { + switch g.Live { + case LiveLive, LiveDead: + seen++ + case LiveUnregistered: + unregistered++ + } + } + if unregistered == 0 || seen > 0 { + return + } + for _, g := range groups { + if g.Live != LiveUnregistered { + continue + } + g.Live, g.Route, g.ManualReason = LiveError, "", "" + g.Err = fmt.Errorf("liveness check failed for %s: member-service answered (403) for every Account checked (%d) and 200 for none: the tool's client cannot see any b2b_org (access tuple missing?): %w", g.OldID, unregistered, g.liveErr) } - return LiveLive, nil } func resolveNewID(ctx context.Context, deps Deps, g *Group, mapping *Mapping, shared SharedDomains, opts Options) (newID, action, reason string) { @@ -364,7 +415,7 @@ func (p *Plan) Print(w io.Writer) { if p.Apply { mode = ModeApply } - fmt.Fprintf(w, "org_import ingest stage=%s mode=%s eligible_groups=%d register=%d rewrite=%d pending=%d skipped=%d\n", p.Stage, mode, len(p.Groups), len(p.Register), len(p.Rewrite), p.Pending(), p.Skipped) + fmt.Fprintf(w, "org_import ingest stage=%s mode=%s eligible_groups=%d register=%d rewrite=%d pending=%d unregistered=%d skipped=%d\n", p.Stage, mode, len(p.Groups), len(p.Register), len(p.Rewrite), p.Pending(), p.Unregistered(), p.Skipped) for _, g := range p.Groups { fmt.Fprintf(w, "%s\n", describe(g)) } @@ -579,9 +630,41 @@ func (r *runner) register(ctx context.Context, g *Group) error { return err } fmt.Fprintf(r.deps.Out, "registered %s as b2b_org %s (%s)\n", g.OldID, org.UID, org.Name) + if g.Live != LiveLive { + r.confirmRegistered(ctx, g.OldID) + } return nil } +const ( + registerChecks = 5 + registerCheckDelay = 3 * time.Second +) + +// confirmRegistered re-reads a freshly registered b2b_org: Heimdall answers 403 until the FGA tuples +// land, so 403/404 are retried within a small budget and the result is only reported. +func (r *runner) confirmRegistered(ctx context.Context, sfid string) { + for i := 1; i <= registerChecks; i++ { + _, err := r.deps.Members.GetB2BOrg(ctx, sfid) + var authErr *member_service.AuthError + switch { + case err == nil: + fmt.Fprintf(r.deps.Out, "b2b_org %s visible after %d check(s)\n", sfid, i) + return + case errors.Is(err, member_service.ErrOrgNotFound), errors.As(err, &authErr) && authErr.Status == 403 && !authErr.Token: + if i < registerChecks { + if sErr := r.deps.Sleep(ctx, registerCheckDelay); sErr != nil { + return + } + } + default: + fmt.Fprintf(r.deps.Out, "WARNING: b2b_org %s check failed: %v; the registration itself succeeded\n", sfid, err) + return + } + } + fmt.Fprintf(r.deps.Out, "WARNING: b2b_org %s not yet visible after %d checks (FGA tuples pending); the registration itself succeeded\n", sfid, registerChecks) +} + // resolveApex performs the real Apex call for every rewrite group resolved through --use-apex; a // group created in the dry run receives its Account id here, anything else must match the dry run. func (r *runner) resolveApex(ctx context.Context) { diff --git a/cla-backend-go/orgimport/run_test.go b/cla-backend-go/orgimport/run_test.go index 69ba90d80..415e55970 100644 --- a/cla-backend-go/orgimport/run_test.go +++ b/cla-backend-go/orgimport/run_test.go @@ -17,6 +17,7 @@ import ( "sort" "strconv" "strings" + "sync" "testing" "time" @@ -135,6 +136,7 @@ func (f *fakeEvents) RekeyEventCompanySFID(_ context.Context, id, oldSFID, newSF // fakePlatform is org-service + ACS + member-service + Salesforce in one. type fakePlatform struct { + mu sync.Mutex orgs map[string]*Org servedAfter map[string]int grants map[string][]acs_service.OrgGrant @@ -146,6 +148,12 @@ type fakePlatform struct { failGetB2B bool noMembers bool listHook func(orgID string) + forbidden map[string]int // remaining 403 answers per id; -1 = always + grantsErr map[string]error + tokenDenied bool + lookups map[string]*Org + lookupErr error + lookupCalls []string } func newPlatform() *fakePlatform { @@ -153,6 +161,8 @@ func newPlatform() *fakePlatform { } func (p *fakePlatform) GetOrganization(_ context.Context, id string) (*Org, error) { + p.mu.Lock() + defer p.mu.Unlock() p.calls = append(p.calls, "get-org:"+id) if n := p.servedAfter[id]; n > 0 { p.servedAfter[id] = n - 1 @@ -165,6 +175,8 @@ func (p *fakePlatform) GetOrganization(_ context.Context, id string) (*Org, erro } func (p *fakePlatform) CreateUserRoleScope(_ context.Context, username, orgID, objectType, objectID, roleID string) error { + p.mu.Lock() + defer p.mu.Unlock() p.calls = append(p.calls, "create-grant:"+orgID+":"+username+":"+roleID+":"+objectID) if p.failCreate { return errors.New("org-service down") @@ -175,6 +187,8 @@ func (p *fakePlatform) CreateUserRoleScope(_ context.Context, username, orgID, o } func (p *fakePlatform) DeleteUserRoleScope(_ context.Context, orgID, roleID, grantID, username string) error { + p.mu.Lock() + defer p.mu.Unlock() p.calls = append(p.calls, "delete-grant:"+orgID+":"+username+":"+roleID+":"+grantID) kept := p.grants[orgID][:0] for _, g := range p.grants[orgID] { @@ -187,17 +201,33 @@ func (p *fakePlatform) DeleteUserRoleScope(_ context.Context, orgID, roleID, gra } func (p *fakePlatform) ListOrgGrants(_ context.Context, orgID string) ([]acs_service.OrgGrant, error) { + p.mu.Lock() + defer p.mu.Unlock() if p.listHook != nil { p.listHook(orgID) } + if err := p.grantsErr[orgID]; err != nil { + return nil, err + } return append([]acs_service.OrgGrant(nil), p.grants[orgID]...), nil } func (p *fakePlatform) GetB2BOrg(_ context.Context, uid string) (*member_service.B2BOrg, error) { + p.mu.Lock() + defer p.mu.Unlock() p.calls = append(p.calls, "get-b2b:"+uid) + if p.tokenDenied { + return nil, &member_service.AuthError{Status: 403, Message: "access_denied", Token: true} + } if p.failGetB2B { return nil, &member_service.AuthError{Status: 403, Message: "forbidden"} } + if n, ok := p.forbidden[uid]; ok && n != 0 { + if n > 0 { + p.forbidden[uid] = n - 1 + } + return nil, &member_service.AuthError{Status: 403, Message: "forbidden"} + } if !p.sfAccounts[uid] { return nil, member_service.ErrOrgNotFound } @@ -205,6 +235,8 @@ func (p *fakePlatform) GetB2BOrg(_ context.Context, uid string) (*member_service } func (p *fakePlatform) RegisterB2BOrg(_ context.Context, sfid string) (*member_service.B2BOrg, error) { + p.mu.Lock() + defer p.mu.Unlock() p.calls = append(p.calls, "register:"+sfid) if !p.sfAccounts[sfid] { return nil, member_service.ErrOrgNotFound @@ -213,7 +245,30 @@ func (p *fakePlatform) RegisterB2BOrg(_ context.Context, sfid string) (*member_s return &member_service.B2BOrg{UID: sfid, Name: "acct " + sfid}, nil } +func (p *fakePlatform) LookupOrganization(_ context.Context, name, domain string) (*Org, error) { + p.mu.Lock() + defer p.mu.Unlock() + key := "name:" + name + if domain != "" { + key = "domain:" + domain + } + p.lookupCalls = append(p.lookupCalls, key) + if p.lookupErr != nil { + return nil, p.lookupErr + } + if o, ok := p.lookups[key]; ok { + return o, nil + } + return nil, ErrOrgNotFound +} + func (p *fakePlatform) addGrant(orgID, username, roleID, project string) { + p.mu.Lock() + defer p.mu.Unlock() + p.addGrantLocked(orgID, username, roleID, project) +} + +func (p *fakePlatform) addGrantLocked(orgID, username, roleID, project string) { p.nextGrant++ g := acs_service.OrgGrant{Username: username, RoleID: roleID, RoleName: "role-" + roleID, GrantID: fmt.Sprintf("g%d", p.nextGrant), ScopeID: fmt.Sprintf("s%d", p.nextGrant), ObjectTypeName: objectTypeOrganization, ObjectID: orgID} if project != "" { @@ -256,6 +311,9 @@ func (fx *fixture) deps() Deps { if !fx.platform.noMembers { d.Members = fx.platform } + if fx.platform.lookups != nil || fx.platform.lookupErr != nil { + d.Lookup = fx.platform + } return d } @@ -506,7 +564,7 @@ func TestBuildPlanClassification(t *testing.T) { assert.Len(t, rows, 14) toSF := readCSV(t, filepath.Join(dir, "out", "to_salesforce.csv")) require.Len(t, toSF, 2, "only rewrite candidates without a mapping go to Salesforce") - assert.Equal(t, []string{"lf-unmapped", "Unmapped Ltd", "", "2024-01-02T00:00:00Z", "", "false"}, toSF[1]) + assert.Equal(t, []string{"lf-unmapped", "Unmapped Ltd", "", "2024-01-02T00:00:00Z", "", "false", ""}, toSF[1]) manual := readCSV(t, filepath.Join(dir, "out", "manual_actions.csv")) require.Len(t, manual, 8, "manual + pending groups") byKey := map[string][]string{} @@ -707,7 +765,7 @@ func TestCleanupPreservesUncopiedGrant(t *testing.T) { fx.platform.listHook = func(orgID string) { if orgID == lfID { if lists++; lists == 2 { - fx.platform.addGrant(lfID, "late-user", "role-cla-manager", "cg-1") + fx.platform.addGrantLocked(lfID, "late-user", "role-cla-manager", "cg-1") } } } @@ -1349,6 +1407,7 @@ func TestRewriteWaitsForOrgService(t *testing.T) { require.NoError(t, err) assert.Equal(t, 1, sum.Rewritten) assert.Equal(t, []time.Duration{time.Second, time.Second}, fx.sleeps) + assert.Contains(t, fx.out.String(), "b2b_org "+targetSFID+" visible after 1 check(s)") fx, mapping, statePath = rewriteFixture(t) fx.platform.servedAfter[targetSFID] = 5 @@ -1602,6 +1661,10 @@ func TestAudit(t *testing.T) { fx.platform.orgs["0014100000TwinAAAA"] = &Org{ID: "0014100000TwinAAAA"} fx.platform.orgs["0014100000TwinBBBB"] = &Org{ID: "0014100000TwinBBBB"} fx.platform.sfAccounts["0014100000TwinAAAA"] = true + fx.platform.addGrant(liveSFID, "u1", "r1", "") + fx.platform.addGrant(liveSFID, "u2", "r1", "") + fx.platform.addGrant(liveSFID, "u3", "r2", "proj-1") + fx.platform.addGrant(liveSFID2, "u4", "r1", "") counts := map[string]int{"c-dead": 4, "c-empty": 2} deps := fx.deps() deps.ECLAs = eclaCounterFunc(func(_ context.Context, id string) (int, error) { return counts[id], nil }) @@ -1616,11 +1679,16 @@ func TestAudit(t *testing.T) { rows := readCSV(t, filepath.Join(dir, "audit.csv")) require.Len(t, rows, 9) + require.Equal(t, []string{"company_id", "company_name", "signing_entity_name", "company_external_id", "id_shape", "active_ccla", "ccla_count", "ecla_count", "org_service", "website", "duplicate_sfid_group", "route", "manual_reason", "tier", "acs_roles", "suggested_account"}, rows[0]) byID := map[string][]string{} for _, r := range rows[1:] { byID[r[0]] = r } - assert.Equal(t, []string{"c-live", "Live Corp", "", liveSFID, "001", "true", "1", "0", "200", "https://live.example", "false", "register", "", TierOK}, byID["c-live"]) + assert.Equal(t, []string{"c-live", "Live Corp", "", liveSFID, "001", "true", "1", "0", "200", "https://live.example", "false", "register", "", TierOK, "role-r1=2;role-r2=1", ""}, byID["c-live"]) + assert.Equal(t, "role-r1=2;role-r2=1", byID["c-live-sub"][14], "ACS roles are per old id, so every row of the group shows them") + assert.Equal(t, "", byID["c-inactive"][14], "ids outside the eligible groups and duplicate sets are not looked up in ACS") + assert.Equal(t, "", byID["c-dead"][14], "an id without grants has an empty cell") + assert.Equal(t, "", byID["c-samename-b"][14], "duplicate-set rows are looked up (no grants here)") assert.Equal(t, "rewrite", byID["c-dead"][11]) assert.Equal(t, "4", byID["c-dead"][7], "ECLA counts only for manual/duplicate rows") assert.Equal(t, TierDangling, byID["c-dead"][13]) @@ -1765,17 +1833,21 @@ func TestAuditDuplicateCandidates(t *testing.T) { assert.Error(t, err) }) - t.Run("overlapping name and domain sets are both reported", func(t *testing.T) { + t.Run("overlapping name and domain sets merge into one", func(t *testing.T) { fx := setup() fx.company("c-twin-c", "Twin", "", "0014100000TwinCCCC", "cg-1") fx.platform.orgs["0014100000TwinCCCC"] = &Org{ID: "0014100000TwinCCCC", Name: "Twin", Website: "https://other.example"} + // chained overlap: Other shares a domain with Twin C but not a name with the twins + fx.company("c-other", "Other", "", "0014100000OtherAAA", "cg-1") + fx.platform.orgs["0014100000OtherAAA"] = &Org{ID: "0014100000OtherAAA", Name: "Other", Website: "other.example"} dir := t.TempDir() res, err := Audit(context.Background(), fx.deps(), Options{Stage: "dev", OutDir: dir}) require.NoError(t, err) sets, _ := dupSets(t, dir) - assert.Len(t, res.Duplicates, 4) - assert.Contains(t, sets, []string{"c-twin-a", "c-twin-b", "c-twin-c"}, "by name") - assert.Contains(t, sets, []string{"c-twin-a", "c-twin-b"}, "by domain") + assert.Len(t, res.Duplicates, 3) + assert.Contains(t, sets, []string{"c-other", "c-twin-a", "c-twin-b", "c-twin-c"}, "name set, domain set and the chained domain set are one candidate set") + assert.NotContains(t, sets, []string{"c-twin-a", "c-twin-b"}) + assert.Contains(t, fx.out.String(), "duplicates=3") }) } diff --git a/cla-backend-go/orgimport/steps.go b/cla-backend-go/orgimport/steps.go index 516100256..622fe8040 100644 --- a/cla-backend-go/orgimport/steps.go +++ b/cla-backend-go/orgimport/steps.go @@ -245,5 +245,6 @@ func (r *runner) registerNew(ctx context.Context, g *Group) error { return err } fmt.Fprintf(r.deps.Out, "registered %s as b2b_org %s (%s)\n", g.NewID, org.UID, org.Name) + r.confirmRegistered(ctx, g.NewID) return nil } diff --git a/cla-backend-go/orgimport/suggest.go b/cla-backend-go/orgimport/suggest.go new file mode 100644 index 000000000..eb4165aa4 --- /dev/null +++ b/cla-backend-go/orgimport/suggest.go @@ -0,0 +1,305 @@ +// Copyright The Linux Foundation and each contributor to CommunityBridge. +// SPDX-License-Identifier: MIT + +package orgimport + +import ( + "context" + "errors" + "fmt" + "sort" + "strings" + "sync" + + member_service "github.com/linuxfoundation/easycla/cla-backend-go/v2/member-service" +) + +const ( + maxSuggestions = 3 + + verifiedOK = "ok" + verifiedDropped = "dropped" + verifiedUnknown = "?" +) + +// knownAccount is an Account served by org-service under a Salesforce id already present in EasyCLA. +type knownAccount struct { + ID string + Name string +} + +// accountIndex finds known Accounts by matching key (website domain) and by normalized name. +type accountIndex struct { + byKey map[string]*knownAccount + byDomain map[string][]*knownAccount + byName map[string][]*knownAccount +} + +func newAccountIndex() *accountIndex { + return &accountIndex{byKey: map[string]*knownAccount{}, byDomain: map[string][]*knownAccount{}, byName: map[string][]*knownAccount{}} +} + +func (ix *accountIndex) add(shared SharedDomains, id string, org *Org, rowNames ...string) { + if org == nil || ShapeOf(id) != ShapeSFID { + return + } + key := accountKey(id) + acct := ix.byKey[key] + if acct == nil { + acct = &knownAccount{ID: id, Name: firstNonEmpty(strings.TrimSpace(org.Name), firstNonEmpty(rowNames...))} + ix.byKey[key] = acct + } + if d, gate := shared.Shared(org.Website); gate == "" { + ix.byDomain[d] = appendAccount(ix.byDomain[d], acct) + } + for _, n := range append([]string{org.Name}, rowNames...) { + if c := canonicalEntity(n); c != "" { + ix.byName[c] = appendAccount(ix.byName[c], acct) + } + } +} + +func appendAccount(list []*knownAccount, acct *knownAccount) []*knownAccount { + for _, a := range list { + if a == acct { + return list + } + } + return append(list, acct) +} + +// knownAccountsFromGroups indexes the Accounts org-service served for the plan's groups. +func knownAccountsFromGroups(shared SharedDomains, groups []*Group) *accountIndex { + ix := newAccountIndex() + for _, g := range groups { + ix.add(shared, g.OldID, g.Org, g.Names()...) + } + return ix +} + +// suggester computes the advisory suggested_account cell: EasyCLA-internal matches first, then +// org-service lookups, each verified through member-service when possible. Never fatal. +type suggester struct { + deps Deps + shared SharedDomains + ix *accountIndex + mu sync.Mutex + crm map[string]*Org + verified map[string]string + warned map[string]bool +} + +func newSuggester(deps Deps, shared SharedDomains, ix *accountIndex) *suggester { + return &suggester{deps: deps, shared: shared, ix: ix, crm: map[string]*Org{}, verified: map[string]string{}, warned: map[string]bool{}} +} + +type candidate struct { + id, name, how string +} + +// suggest returns the cell for one row/group: ownKey is the accountKey of its own id (never +// suggested), website and names are what it is matched by. +func (s *suggester) suggest(ctx context.Context, ownKey, website string, names []string) string { + domain, gate := s.shared.Shared(website) + if gate != "" { + domain = "" + } + canon := map[string]bool{} + var display []string + for _, n := range names { + if c := canonicalEntity(n); c != "" && !canon[c] { + canon[c] = true + display = append(display, strings.TrimSpace(n)) + } + } + var cands []candidate + if domain != "" { + for _, a := range s.ix.byDomain[domain] { + cands = append(cands, candidate{a.ID, a.Name, "inventory:domain"}) + } + } + for _, n := range display { + for _, a := range s.ix.byName[canonicalEntity(n)] { + cands = append(cands, candidate{a.ID, a.Name, "inventory:name"}) + } + } + if s.deps.Lookup != nil { + if domain != "" { + if o := s.lookup(ctx, "", domain); o != nil { + cands = append(cands, candidate{o.ID, o.Name, "crm:domain"}) + } + } + for _, n := range display { + if o := s.lookup(ctx, n, ""); o != nil { + cands = append(cands, candidate{o.ID, o.Name, "crm:name"}) + } + } + } + seen := map[string]bool{} + var cells []string + for _, c := range cands { + key := accountKey(c.id) + if ShapeOf(c.id) != ShapeSFID || key == ownKey || seen[key] { + continue + } + seen[key] = true + switch s.verify(ctx, c.id) { + case verifiedDropped: + continue + case verifiedUnknown: + c.how += verifiedUnknown + } + cells = append(cells, strings.TrimSpace(fmt.Sprintf("%s %s", c.id, c.name))+" ["+c.how+"]") + if len(cells) == maxSuggestions { + break + } + } + return strings.Join(cells, "; ") +} + +// lookup asks org-service for one Account by name or by domain (cached; one warning per key). +func (s *suggester) lookup(ctx context.Context, name, domain string) *Org { + key := "name:" + canonicalEntity(name) + if domain != "" { + key = "domain:" + domain + } + s.mu.Lock() + o, ok := s.crm[key] + s.mu.Unlock() + if ok { + return o + } + o, err := s.deps.Lookup.LookupOrganization(ctx, name, domain) + if err != nil { + o = nil + if !errors.Is(err, ErrOrgNotFound) { + s.warn(key, fmt.Sprintf("WARNING: account lookup %s failed: %v\n", key, err)) + } + } else if o != nil && ShapeOf(o.ID) != ShapeSFID { + o = nil + } + s.mu.Lock() + s.crm[key] = o + s.mu.Unlock() + return o +} + +// verify checks a candidate in member-service: "ok" (200), "dropped" (404) or "?" (403, error, no client). +func (s *suggester) verify(ctx context.Context, id string) string { + key := accountKey(id) + s.mu.Lock() + v, ok := s.verified[key] + s.mu.Unlock() + if ok { + return v + } + v = verifiedUnknown + if s.deps.Members != nil { + _, err := s.deps.Members.GetB2BOrg(ctx, id) + switch { + case err == nil: + v = verifiedOK + case errors.Is(err, member_service.ErrOrgNotFound): + v = verifiedDropped + default: + var authErr *member_service.AuthError + if !(errors.As(err, &authErr) && authErr.Status == 403 && !authErr.Token) { + s.warn("verify:"+key, fmt.Sprintf("WARNING: account check %s failed: %v\n", id, err)) + } + } + } + s.mu.Lock() + s.verified[key] = v + s.mu.Unlock() + return v +} + +func (s *suggester) warn(key, msg string) { + s.mu.Lock() + defer s.mu.Unlock() + if s.warned[key] { + return + } + s.warned[key] = true + fmt.Fprint(s.deps.Out, msg) +} + +// suggestAccounts fills Group.Suggested for the groups fill selects (4 workers). +func suggestAccounts(ctx context.Context, deps Deps, shared SharedDomains, ix *accountIndex, groups []*Group, fill func(*Group) bool) { + s := newSuggester(deps, shared, ix) + var selected []*Group + for _, g := range groups { + if fill(g) { + selected = append(selected, g) + } + } + runWorkers(len(selected), func(i int) { + g := selected[i] + names := g.Names() + if g.Org != nil { + names = append(names, g.Org.Name) + } + g.Suggested = s.suggest(ctx, accountKey(g.OldID), g.Website(), names) + }) +} + +// suggestRows fills AuditRow.Suggested for the rows fill selects (4 workers). +func suggestRows(ctx context.Context, deps Deps, shared SharedDomains, ix *accountIndex, rows []*AuditRow, fill func(*AuditRow) bool) { + s := newSuggester(deps, shared, ix) + var selected []*AuditRow + for _, ar := range rows { + if fill(ar) { + selected = append(selected, ar) + } + } + runWorkers(len(selected), func(i int) { + ar := selected[i] + names := []string{ar.Row.CompanyName} + if ar.OrgName != "" { + names = append(names, ar.OrgName) + } + ar.Suggested = s.suggest(ctx, accountKey(ar.Row.ExternalID), ar.Website, names) + }) +} + +// runWorkers runs fn(0..n-1) on four goroutines. +func runWorkers(n int, fn func(i int)) { + work := make(chan int) + var wg sync.WaitGroup + for w := 0; w < 4; w++ { + wg.Add(1) + go func() { + defer wg.Done() + for i := range work { + fn(i) + } + }() + } + for i := 0; i < n; i++ { + work <- i + } + close(work) + wg.Wait() +} + +// acsRoles formats ACS grants as role=count;... sorted by role name; "err" when the listing failed. +func acsRoles(ctx context.Context, acs ACSService, id string) string { + grants, err := acs.ListOrgGrants(ctx, id) + if err != nil { + return orgStatusErr + } + counts := map[string]int{} + for _, g := range grants { + counts[firstNonEmpty(g.RoleName, g.RoleID)]++ + } + names := make([]string, 0, len(counts)) + for n := range counts { + names = append(names, n) + } + sort.Strings(names) + parts := make([]string, 0, len(names)) + for _, n := range names { + parts = append(parts, fmt.Sprintf("%s=%d", n, counts[n])) + } + return strings.Join(parts, ";") +} diff --git a/cla-backend-go/v2/member-service/client.go b/cla-backend-go/v2/member-service/client.go index 01234c3f7..8a2c37cbf 100644 --- a/cla-backend-go/v2/member-service/client.go +++ b/cla-backend-go/v2/member-service/client.go @@ -32,6 +32,7 @@ var ErrNotConfigured = errors.New("member-service: client not configured") type AuthError struct { Status int Message string + Token bool // the Auth0 token request failed, not the member-service call } func (e *AuthError) Error() string { @@ -254,14 +255,14 @@ func (c *Client) getToken(ctx context.Context) (string, error) { return "", readErr } if resp.StatusCode < 200 || resp.StatusCode > 299 { - return "", &AuthError{Status: resp.StatusCode, Message: responseMessage(body, resp.Status)} + return "", &AuthError{Status: resp.StatusCode, Message: responseMessage(body, resp.Status), Token: true} } var tr struct { AccessToken string `json:"access_token"` ExpiresIn int `json:"expires_in"` } if err = json.Unmarshal(body, &tr); err != nil || tr.AccessToken == "" { - return "", &AuthError{Status: resp.StatusCode, Message: "empty access token"} + return "", &AuthError{Status: resp.StatusCode, Message: "empty access token", Token: true} } c.token = tr.AccessToken c.expiry = time.Now().Add(time.Duration(tr.ExpiresIn) * time.Second) diff --git a/cla-backend-go/v2/member-service/client_test.go b/cla-backend-go/v2/member-service/client_test.go index 93e31338f..dd251cce9 100644 --- a/cla-backend-go/v2/member-service/client_test.go +++ b/cla-backend-go/v2/member-service/client_test.go @@ -24,6 +24,7 @@ type fakeMemberService struct { registerBody []map[string]string status int response interface{} + tokenStatus int } func (f *fakeMemberService) decode(r *http.Request, v interface{}) { @@ -46,6 +47,11 @@ func (f *fakeMemberService) ServeHTTP(w http.ResponseWriter, r *http.Request) { f.decode(r, &req) assert.Equal(f.t, "client_credentials", req["grant_type"]) assert.Equal(f.t, "https://member.example/", req["audience"]) + if f.tokenStatus != 0 { + w.WriteHeader(f.tokenStatus) + f.encode(w, map[string]string{"error": "access_denied", "error_description": "Client is not authorized"}) + return + } f.encode(w, map[string]interface{}{"access_token": "member-token", "token_type": "Bearer", "expires_in": 3600}) case "/b2b_orgs/0014100000Te0G7AAJ", "/b2b_orgs/" + syntheticSFID18: assert.Equal(f.t, http.MethodGet, r.Method) @@ -209,6 +215,7 @@ func TestRegisterB2BOrgErrors(t *testing.T) { var authErr *AuthError require.ErrorAs(t, err, &authErr) assert.Equal(t, http.StatusForbidden, authErr.Status) + assert.False(t, authErr.Token, "a member-service 403 is not a token failure") tokenCalls := fake.tokenCalls fake.status, fake.response = http.StatusServiceUnavailable, map[string]string{"message": "down"} @@ -221,3 +228,15 @@ func TestRegisterB2BOrgErrors(t *testing.T) { _, err = client.RegisterB2BOrg(context.Background(), "0014100000Te0G7AAJ") assert.Error(t, err) } + +func TestTokenForbiddenIsFlagged(t *testing.T) { + fake := &fakeMemberService{tokenStatus: http.StatusForbidden} + client := newTestClient(t, fake) + _, err := client.GetB2BOrg(context.Background(), "0014100000Te0G7AAJ") + var authErr *AuthError + require.ErrorAs(t, err, &authErr) + assert.Equal(t, http.StatusForbidden, authErr.Status) + assert.True(t, authErr.Token) + assert.Contains(t, err.Error(), "Client is not authorized") + assert.Zero(t, fake.getCalls) +} From bc4ddb44721417d5fb0fc442993433a6b6c01230 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Gryglicki?= Date: Tue, 6 Oct 2026 11:07:43 +0200 Subject: [PATCH 2/4] Address AI feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Łukasz Gryglicki Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot) Assisted by [Claude](https://claude.ai) --- .github/workflows/org-import-sweep.yml | 15 +++++++++++---- cla-backend-go/cmd/org_import/README.md | 4 ++-- cla-backend-go/cmd/org_import/main.go | 2 +- cla-backend-go/orgimport/report.go | 16 ++++++++++------ cla-backend-go/orgimport/report_test.go | 9 +++++---- cla-backend-go/v2/member-service/client.go | 4 +++- cla-backend-go/v2/member-service/client_test.go | 8 +++++++- 7 files changed, 39 insertions(+), 19 deletions(-) diff --git a/.github/workflows/org-import-sweep.yml b/.github/workflows/org-import-sweep.yml index 96d0f08cd..6e5069596 100644 --- a/.github/workflows/org-import-sweep.yml +++ b/.github/workflows/org-import-sweep.yml @@ -52,6 +52,10 @@ on: description: E-mail the full run report to SSM cla-org-import-report-emails- type: boolean default: true + register_unregistered: + description: Also register Accounts whose member-service GET answered 403 (default leaves them pending) + type: boolean + default: false schedule: - cron: '0 6 * * *' @@ -75,6 +79,7 @@ jobs: IN_TRANCHE: ${{ inputs.tranche }} IN_IDS: ${{ inputs.ids }} IN_NOTIFY: ${{ inputs.notify }} + IN_REGISTER_UNREGISTERED: ${{ inputs.register_unregistered }} SWEEP_DEV: ${{ vars.ORG_IMPORT_SWEEP_DEV }} SWEEP_PROD: ${{ vars.ORG_IMPORT_SWEEP_PROD }} run: | @@ -87,17 +92,17 @@ jobs: esac } entry() { - jq -cn --arg stage "$1" --arg mode "$2" --arg routes "$3" --arg tranche "$4" --arg ids "$5" --arg notify "$6" --arg role "$(role "$1")" \ - '{stage:$stage, mode:$mode, routes:$routes, tranche:$tranche, ids:$ids, notify:$notify, role:$role}' + jq -cn --arg stage "$1" --arg mode "$2" --arg routes "$3" --arg tranche "$4" --arg ids "$5" --arg notify "$6" --arg register_unregistered "$7" --arg role "$(role "$1")" \ + '{stage:$stage, mode:$mode, routes:$routes, tranche:$tranche, ids:$ids, notify:$notify, register_unregistered:$register_unregistered, role:$role}' } entries=() if [ "$EVENT" = workflow_dispatch ]; then - entries+=("$(entry "$IN_STAGE" "$IN_MODE" "$IN_ROUTES" "${IN_TRANCHE:-0}" "$IN_IDS" "${IN_NOTIFY:-true}")") + entries+=("$(entry "$IN_STAGE" "$IN_MODE" "$IN_ROUTES" "${IN_TRANCHE:-0}" "$IN_IDS" "${IN_NOTIFY:-true}" "${IN_REGISTER_UNREGISTERED:-false}")") else for pair in "dev:${SWEEP_DEV:-off}" "prod:${SWEEP_PROD:-off}"; do stage="${pair%%:*}"; mode="${pair#*:}" case "$mode" in - dry-run|apply) entries+=("$(entry "$stage" "$mode" register 0 '' true)") ;; + dry-run|apply) entries+=("$(entry "$stage" "$mode" register 0 '' true false)") ;; *) echo "scheduled sweep for $stage is off" ;; esac done @@ -219,6 +224,7 @@ jobs: TRANCHE: ${{ matrix.tranche }} IDS: ${{ matrix.ids }} NOTIFY: ${{ matrix.notify }} + REGISTER_UNREGISTERED: ${{ matrix.register_unregistered }} run: | set -euo pipefail args=(ingest --routes "$ROUTES" --out-dir org-import-out --state org-import-out/state.jsonl) @@ -228,6 +234,7 @@ jobs: [ -s org-import-out/decisions.csv ] && args+=(--decisions org-import-out/decisions.csv) [ -s org-import-out/shared_domains.txt ] && args+=(--shared-domains org-import-out/shared_domains.txt) [ "$NOTIFY" = false ] && args+=(--no-email) + [ "$REGISTER_UNREGISTERED" = true ] && args+=(--register-unregistered) [ "$MODE" = apply ] && args+=(--apply --yes) echo "bin/org-import ${args[*]}" bin/org-import "${args[@]}" diff --git a/cla-backend-go/cmd/org_import/README.md b/cla-backend-go/cmd/org_import/README.md index b95ca42e6..21d3fc4de 100644 --- a/cla-backend-go/cmd/org_import/README.md +++ b/cla-backend-go/cmd/org_import/README.md @@ -145,7 +145,7 @@ STAGE=dev bin/org-import ingest --routes rewrite --mapping map.csv --state state Annotated sample: ``` -org_import ingest stage=dev mode=dry-run eligible_groups=4 register=1 rewrite=1 pending=1 skipped=0 +org_import ingest stage=dev mode=dry-run eligible_groups=4 register=1 rewrite=1 pending=1 unregistered=0 skipped=0 register 0014100000Te0G7AAJ shape=001 rows=2 live=live | Live Corp; Live Corp / Live Corp Asia rewrite lfbd1c2b3a4d5e6f7a8 shape=lf rows=1 new_id=0014100000NewNewNe action=matched domain=legacy.example | Legacy Ltd rewrite 0014100000DeadDead1 shape=001 rows=1 live=dead reason=no_mapping | Dead Corp <- pending: needs a mapping row @@ -296,7 +296,7 @@ manual rewrite tranche of §5 (a named owner is required, see the design doc). GitHub Actions `.github/workflows/org-import-sweep.yml`: - Actions tab → "Org import sweep" → Run workflow: `stage` (`dev|prod`), `mode` (`dry-run|apply`), `routes` (default `register`), `tranche`, optional `ids`, optional `mapping` / `decisions` / `shared_domains` (the files of §4 pasted as text; rows separated by newlines or - `|` — e.g. `old_id,new_id,action,approved|lf…,001…,matched,true`), `notify` (default true; false ⇒ `--no-email`). Without a mapping, rewrite + `|` — e.g. `old_id,new_id,action,approved|lf…,001…,matched,true`), `notify` (default true; false ⇒ `--no-email`), `register_unregistered` (default false; true ⇒ `--register-unregistered`; scheduled runs never set it). Without a mapping, rewrite candidates are only reported as pending. Same from a shell (the report e-mail of every dry run prints this line ready to paste): `gh workflow run org-import-sweep.yml -f stage=dev -f mode=dry-run -f routes=register,rewrite -f mapping="$(tr '\n' '|' < map.csv)"`. - State: apply runs upload `state.jsonl` as artifact `org-import-state-`; the next run of the same stage restores the newest one first, diff --git a/cla-backend-go/cmd/org_import/main.go b/cla-backend-go/cmd/org_import/main.go index 6379c0f04..2bab75549 100644 --- a/cla-backend-go/cmd/org_import/main.go +++ b/cla-backend-go/cmd/org_import/main.go @@ -176,7 +176,7 @@ func run(args []string, stdin io.Reader) int { info := orgimport.RunInfo{ Stage: stage, Command: cmd, Apply: *apply, Args: args, Start: time.Now().UTC(), Runner: runnerName(), Repository: os.Getenv("GITHUB_REPOSITORY"), Revision: buildRevision(), OutDir: *outDir, Notices: notices, - Workflow: orgimport.WorkflowInputs{Routes: *routes, Tranche: fmt.Sprint(*tranche), IDs: *ids, Mapping: *mapping, Decisions: *decisions, SharedDomains: *sharedDomains}, + Workflow: orgimport.WorkflowInputs{Routes: *routes, Tranche: fmt.Sprint(*tranche), IDs: *ids, Mapping: *mapping, Decisions: *decisions, SharedDomains: *sharedDomains, RegisterUnregistered: *registerUnregistered}, } if runID := os.Getenv("GITHUB_RUN_ID"); runID != "" { info.RunURL = fmt.Sprintf("%s/%s/actions/runs/%s", os.Getenv("GITHUB_SERVER_URL"), os.Getenv("GITHUB_REPOSITORY"), runID) diff --git a/cla-backend-go/orgimport/report.go b/cla-backend-go/orgimport/report.go index cb55fa650..89c5f3cd6 100644 --- a/cla-backend-go/orgimport/report.go +++ b/cla-backend-go/orgimport/report.go @@ -71,12 +71,13 @@ const ( // WorkflowInputs mirrors the org-import-sweep.yml dispatch inputs for the "how to apply" command. type WorkflowInputs struct { - Routes string - Tranche string - IDs string - Mapping string - Decisions string - SharedDomains string + Routes string + Tranche string + IDs string + Mapping string + Decisions string + SharedDomains string + RegisterUnregistered bool } // Mode is dry-run or apply. @@ -333,6 +334,9 @@ func ApplyCommands(info RunInfo) []string { gh += fmt.Sprintf(" -f %s=\"$(tr '\\n' '|' < %s)\"", f[0], recordPath(info.OutDir, f[2])) } } + if w.RegisterUnregistered { + gh += " -f register_unregistered=true" + } return []string{local, gh} } diff --git a/cla-backend-go/orgimport/report_test.go b/cla-backend-go/orgimport/report_test.go index 027e4be17..b45342aaf 100644 --- a/cla-backend-go/orgimport/report_test.go +++ b/cla-backend-go/orgimport/report_test.go @@ -131,11 +131,11 @@ func TestBuildReportEscapesAndTruncates(t *testing.T) { } func TestApplyCommands(t *testing.T) { - cmds := ApplyCommands(RunInfo{Stage: "prod", OutDir: "/runs/out 1", Args: []string{"ingest", "--apply", "--yes", "--ids", "a,b", "--mapping", "/home/op/my map.csv", "--no-email"}, - Workflow: WorkflowInputs{Routes: "register,rewrite", Tranche: "10", IDs: "a,b", Mapping: "/home/op/my map.csv", Decisions: "d.csv", SharedDomains: "s.txt"}}) + cmds := ApplyCommands(RunInfo{Stage: "prod", OutDir: "/runs/out 1", Args: []string{"ingest", "--apply", "--yes", "--ids", "a,b", "--mapping", "/home/op/my map.csv", "--no-email", "--register-unregistered"}, + Workflow: WorkflowInputs{Routes: "register,rewrite", Tranche: "10", IDs: "a,b", Mapping: "/home/op/my map.csv", Decisions: "d.csv", SharedDomains: "s.txt", RegisterUnregistered: true}}) require.Len(t, cmds, 2) - assert.Equal(t, `STAGE=prod ./bin/org-import ingest --ids a,b --mapping "${RECORD:-/runs/out 1}/input-mapping.csv" --state "${RECORD:-/runs/out 1}/state.jsonl" --apply --yes`, cmds[0]) - assert.Equal(t, `gh workflow run org-import-sweep.yml -R linuxfoundation/easycla -f stage=prod -f mode=apply -f routes=register,rewrite -f tranche=10 -f ids=a,b -f mapping="$(tr '\n' '|' < "${RECORD:-/runs/out 1}/input-mapping.csv")" -f decisions="$(tr '\n' '|' < "${RECORD:-/runs/out 1}/input-decisions.csv")" -f shared_domains="$(tr '\n' '|' < "${RECORD:-/runs/out 1}/input-shared_domains.txt")"`, cmds[1]) + assert.Equal(t, `STAGE=prod ./bin/org-import ingest --ids a,b --mapping "${RECORD:-/runs/out 1}/input-mapping.csv" --register-unregistered --state "${RECORD:-/runs/out 1}/state.jsonl" --apply --yes`, cmds[0]) + assert.Equal(t, `gh workflow run org-import-sweep.yml -R linuxfoundation/easycla -f stage=prod -f mode=apply -f routes=register,rewrite -f tranche=10 -f ids=a,b -f mapping="$(tr '\n' '|' < "${RECORD:-/runs/out 1}/input-mapping.csv")" -f decisions="$(tr '\n' '|' < "${RECORD:-/runs/out 1}/input-decisions.csv")" -f shared_domains="$(tr '\n' '|' < "${RECORD:-/runs/out 1}/input-shared_domains.txt")" -f register_unregistered=true`, cmds[1]) assert.Equal(t, "''", shellQuote("")) assert.Equal(t, `'it'\''s'`, shellQuote("it's")) assert.Equal(t, []string{"a@x.org", "b@y.org", "c@z.org"}, ParseRecipients(" a@x.org, b@y.org;c@z.org\n")) @@ -167,6 +167,7 @@ func TestApplyCommandsUseTheRecord(t *testing.T) { // without --state the apply command still carries a persistent journal (rewrite apply requires one) cmds = ApplyCommands(RunInfo{Stage: "dev", OutDir: "/r", Args: []string{"ingest", "--routes", "register"}}) assert.Equal(t, `STAGE=dev ./bin/org-import ingest --routes register --state "${RECORD:-/r}/state.jsonl" --apply --yes`, cmds[0]) + assert.NotContains(t, cmds[1], "register_unregistered", "the workflow input is only passed when the run used it") // the RECORD default is safe inside double quotes assert.Equal(t, `"${RECORD:-/a b/\$x/\}y\"\\z\`+"`"+`}/state.jsonl"`, recordPath(`/a b/$x/}y"\z`+"`", RecordState)) diff --git a/cla-backend-go/v2/member-service/client.go b/cla-backend-go/v2/member-service/client.go index 8a2c37cbf..e2a6a7f69 100644 --- a/cla-backend-go/v2/member-service/client.go +++ b/cla-backend-go/v2/member-service/client.go @@ -218,7 +218,9 @@ func (c *Client) do(req *http.Request, tok, operation string) (*B2BOrg, error) { case resp.StatusCode == http.StatusBadRequest: return nil, fmt.Errorf("%w: %s", ErrInvalidSFID, responseMessage(body, resp.Status)) case resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden: - c.forgetToken(tok) + if resp.StatusCode == http.StatusUnauthorized { + c.forgetToken(tok) + } return nil, &AuthError{Status: resp.StatusCode, Message: responseMessage(body, resp.Status)} default: return nil, fmt.Errorf("member-service: %s returned %d: %s", operation, resp.StatusCode, responseMessage(body, resp.Status)) diff --git a/cla-backend-go/v2/member-service/client_test.go b/cla-backend-go/v2/member-service/client_test.go index dd251cce9..d2a102fc7 100644 --- a/cla-backend-go/v2/member-service/client_test.go +++ b/cla-backend-go/v2/member-service/client_test.go @@ -218,11 +218,17 @@ func TestRegisterB2BOrgErrors(t *testing.T) { assert.False(t, authErr.Token, "a member-service 403 is not a token failure") tokenCalls := fake.tokenCalls + fake.status, fake.response = http.StatusUnauthorized, map[string]string{"message": "expired"} + _, err = client.RegisterB2BOrg(context.Background(), "0014100000Te0G7AAJ") + require.ErrorAs(t, err, &authErr) + assert.Equal(t, http.StatusUnauthorized, authErr.Status) + assert.Equal(t, tokenCalls, fake.tokenCalls, "the cached token survives a 403") + fake.status, fake.response = http.StatusServiceUnavailable, map[string]string{"message": "down"} _, err = client.RegisterB2BOrg(context.Background(), "0014100000Te0G7AAJ") require.Error(t, err) assert.Contains(t, err.Error(), "503") - assert.Equal(t, tokenCalls+1, fake.tokenCalls, "token is re-minted after an authorization failure") + assert.Equal(t, tokenCalls+1, fake.tokenCalls, "token is re-minted only after a 401") fake.status, fake.response = http.StatusCreated, map[string]string{"name": "no uid"} _, err = client.RegisterB2BOrg(context.Background(), "0014100000Te0G7AAJ") From 5585ff219fb1ce89d08c88a8efc1aac8bfc8a3b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Gryglicki?= Date: Tue, 6 Oct 2026 11:34:06 +0200 Subject: [PATCH 3/4] Address AI feedback - 2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Łukasz Gryglicki Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot) Assisted by [Claude](https://claude.ai) --- cla-backend-go/cmd/org_import/README.md | 4 +-- cla-backend-go/orgimport/followup_test.go | 32 +++++++++++++++++++++++ cla-backend-go/orgimport/run.go | 1 + 3 files changed, 35 insertions(+), 2 deletions(-) diff --git a/cla-backend-go/cmd/org_import/README.md b/cla-backend-go/cmd/org_import/README.md index 21d3fc4de..4ef91e482 100644 --- a/cla-backend-go/cmd/org_import/README.md +++ b/cla-backend-go/cmd/org_import/README.md @@ -120,8 +120,8 @@ Output line: `audit stage=dev companies=N eligible_groups=M MISSING_SFID=a INVAL Files (`--out-dir`): - `audit.csv` — `company_id, company_name, signing_entity_name, company_external_id, id_shape(001|lf|empty|other), active_ccla, ccla_count, ecla_count, org_service(200|404|err), website, duplicate_sfid_group, route, manual_reason, tier, acs_roles, suggested_account`. `ecla_count` is filled only for active rows that are manual, duplicate or unresolvable and for every row of `possible_duplicates.csv` (cheap); `-1` = count failed. - `acs_roles` (`role=count;…` of the ACS grants scoped to the old id; `err` = listing failed) is filled for eligible `001`/`lf` groups and every - row of `possible_duplicates.csv`. `suggested_account` lists up to three existing Accounts a dead/legacy/manual/duplicate row could belong to: + `acs_roles` (`role=count;…` of the ACS grants scoped to the old id; `err` = listing failed) is filled in `audit.csv` for eligible `001`/`lf` groups and + for duplicate-set rows with those id shapes (`possible_duplicates.csv` has no `acs_roles` column). `suggested_account` lists up to three existing Accounts a dead/legacy/manual/duplicate row could belong to: ` [inventory:domain|inventory:name|crm:domain|crm:name]` (inventory = other Accounts served by org-service for the same import, crm = org-service lookup by registrable website domain, then by name); candidates are verified in member-service — dropped Accounts are omitted, unverifiable ones (403) carry a `?` suffix. Best effort: lookup failures only warn. diff --git a/cla-backend-go/orgimport/followup_test.go b/cla-backend-go/orgimport/followup_test.go index 679f5989e..92ea4f518 100644 --- a/cla-backend-go/orgimport/followup_test.go +++ b/cla-backend-go/orgimport/followup_test.go @@ -300,3 +300,35 @@ func TestAuditACSRolesError(t *testing.T) { require.Len(t, rows, 2) assert.Equal(t, "err", rows[1][14]) } + +func TestPlanSuggestedAccountFromInventory(t *testing.T) { + const ( + oldSFID = "0014100000OldAAAAA" + old2SFID = "0014100000OldBBBBB" + newSFID = "0014100000NewCCCCC" + ) + fx := newFixture() + fx.company("c-old", "Acme", "", oldSFID, "cg-1") + fx.company("c-old2", "Widgets", "", old2SFID, "cg-1") + fx.company("c-new", "Acme", "", newSFID, "cg-1") + fx.company("c-lf", "Acme", "", "lf-acme-legacy-01", "cg-1") + fx.company("c-unrelated", "Zeta", "", liveSFID2, "cg-1") + fx.platform.sfAccounts[newSFID] = true + fx.platform.sfAccounts[liveSFID2] = true + fx.platform.orgs[newSFID] = &Org{ID: newSFID, Name: "Acme Inc.", Website: "https://www.acme.example"} + fx.platform.orgs[old2SFID] = &Org{ID: old2SFID, Name: "Widgets", Website: "https://labs.acme.example/x"} + fx.platform.orgs[liveSFID2] = &Org{ID: liveSFID2, Name: "Zeta", Website: "https://zeta.example"} + + plan, err := BuildPlan(context.Background(), fx.deps(), Options{Stage: "dev"}) + require.NoError(t, err) + live := groupByKey(plan.Groups, newSFID) + require.NotNil(t, live) + assert.Equal(t, LiveLive, live.Live) + assert.Equal(t, "200", live.OrgStatus) + assert.Equal(t, "", live.Suggested, "a live Account never suggests itself") + assert.Equal(t, newSFID+" Acme Inc. [inventory:name]", groupByKey(plan.Groups, oldSFID).Suggested) + assert.Equal(t, newSFID+" Acme Inc. [inventory:domain]", groupByKey(plan.Groups, old2SFID).Suggested) + assert.Equal(t, newSFID+" Acme Inc. [inventory:name]", groupByKey(plan.Groups, "lf-acme-legacy-01").Suggested) + assert.Equal(t, "", groupByKey(plan.Groups, liveSFID2).Suggested) + assert.Empty(t, fx.platform.registered) +} diff --git a/cla-backend-go/orgimport/run.go b/cla-backend-go/orgimport/run.go index f0cd5404a..d1af8bcb9 100644 --- a/cla-backend-go/orgimport/run.go +++ b/cla-backend-go/orgimport/run.go @@ -251,6 +251,7 @@ func classify(ctx context.Context, deps Deps, g *Group, mapping *Mapping, shared switch g.Live { case LiveLive: g.Route = RouteRegister + lookupOrg(ctx, deps, g) return case LiveUnverified: g.Route, g.ManualReason = RouteRegister, ReasonCRMUnverified From 0ebe80b4b49312f54be162cab8dcfc685c049702 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Gryglicki?= Date: Tue, 6 Oct 2026 11:57:45 +0200 Subject: [PATCH 4/4] Address AI feedback - 3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Łukasz Gryglicki Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot) Assisted by [Claude](https://claude.ai) --- cla-backend-go/orgimport/audit.go | 5 ++++- cla-backend-go/orgimport/followup_test.go | 22 +++++++++++++++++++--- cla-backend-go/orgimport/manual.go | 13 ++++++++++++- cla-backend-go/orgimport/run.go | 5 ++++- 4 files changed, 39 insertions(+), 6 deletions(-) diff --git a/cla-backend-go/orgimport/audit.go b/cla-backend-go/orgimport/audit.go index 0dc3f107c..1e59de712 100644 --- a/cla-backend-go/orgimport/audit.go +++ b/cla-backend-go/orgimport/audit.go @@ -67,7 +67,11 @@ func Audit(ctx context.Context, deps Deps, opts Options) (*AuditResult, error) { return nil, err } groups := inv.EligibleGroups() + orgs := lookupOrgs(ctx, deps, inv) for _, g := range groups { + if o := orgs[g.OldID]; o != nil { + g.Org, g.OrgStatus = o.org, o.status + } classify(ctx, deps, g, nil, nil, Options{}) } livenessGuard(groups) @@ -78,7 +82,6 @@ func Audit(ctx context.Context, deps Deps, opts Options) (*AuditResult, error) { } } - orgs := lookupOrgs(ctx, deps, inv) res := &AuditResult{Tiers: map[string]int{}, Routes: map[Route]int{}, rowByID: map[string]*AuditRow{}, shared: shared} byName, byDomain := map[string][]*Row{}, map[string][]*Row{} known := newAccountIndex() diff --git a/cla-backend-go/orgimport/followup_test.go b/cla-backend-go/orgimport/followup_test.go index 92ea4f518..ca04da84f 100644 --- a/cla-backend-go/orgimport/followup_test.go +++ b/cla-backend-go/orgimport/followup_test.go @@ -6,6 +6,7 @@ package orgimport import ( "context" "errors" + "fmt" "path/filepath" "strings" "testing" @@ -87,7 +88,7 @@ func TestAllForbiddenIsAnAccessError(t *testing.T) { assert.Empty(t, g.Route) assert.Empty(t, g.ManualReason) require.Error(t, g.Err) - assert.ErrorContains(t, g.Err, "every Account checked (3)") + assert.ErrorContains(t, g.Err, "403 for 3") var authErr *member_service.AuthError assert.ErrorAs(t, g.Err, &authErr) assert.Contains(t, SuggestError(g.Err), "auditor") @@ -104,7 +105,7 @@ func TestAllForbiddenIsAnAccessError(t *testing.T) { require.NoError(t, err) rows := readCSV(t, filepath.Join(dir, "audit.csv")) for _, r := range rows[1:] { - assert.Contains(t, r[12], "every Account checked (3)", r[0]) + assert.Contains(t, r[12], "403 for 3", r[0]) } assert.Contains(t, fx.out.String(), " unregistered=0 ") } @@ -144,6 +145,21 @@ func TestAuditSuggestedAccountFromInventory(t *testing.T) { assert.Equal(t, "", byID["c-unrelated"][15]) assert.NotEmpty(t, res.Duplicates) assert.Empty(t, fx.platform.registered) + gets := map[string]int{} + for _, c := range fx.platform.calls { + if strings.HasPrefix(c, "get-org:") { + gets[c]++ + } + } + assert.Equal(t, 1, gets["get-org:"+newSFID], "audit looks each Account up once") + assert.Equal(t, 1, gets["get-org:"+old2SFID]) +} + +func TestSuggestErrorAuthError(t *testing.T) { + wrap := func(e error) error { return fmt.Errorf("liveness check failed for X: %w", e) } + assert.Contains(t, SuggestError(wrap(&member_service.AuthError{Status: 403, Message: "Client is not authorized", Token: true})), "client grant") + assert.Contains(t, SuggestError(wrap(&member_service.AuthError{Status: 401, Message: "bad secret", Token: true})), "token request for the member-service audience failed (401)") + assert.Contains(t, SuggestError(wrap(&member_service.AuthError{Status: 403, Message: "forbidden"})), "auditor") } // --register-unregistered POSTs the 403 Accounts too and re-reads each one until Heimdall serves it. @@ -211,7 +227,7 @@ func TestTokenForbiddenIsALivenessError(t *testing.T) { assert.Equal(t, LiveError, g.Live) assert.Empty(t, g.Route) require.Error(t, g.Err) - assert.NotContains(t, g.Err.Error(), "every Account checked") + assert.NotContains(t, g.Err.Error(), "for no Account") assert.Empty(t, plan.Register) assert.Zero(t, plan.Unregistered()) sum, err := Execute(context.Background(), fx.deps(), Options{Apply: true, RegisterUnregistered: true}, plan) diff --git a/cla-backend-go/orgimport/manual.go b/cla-backend-go/orgimport/manual.go index 36cd51d3b..0be19bc41 100644 --- a/cla-backend-go/orgimport/manual.go +++ b/cla-backend-go/orgimport/manual.go @@ -4,8 +4,11 @@ package orgimport import ( + "errors" "fmt" "strings" + + member_service "github.com/linuxfoundation/easycla/cla-backend-go/v2/member-service" ) // ManualAction is one row of manual_actions.csv: a group the tool will not process on its own. @@ -48,11 +51,19 @@ func Suggest(reason string) string { } // SuggestError maps a group error to an action; access errors get the ops item instead of "re-run". +const clientGrantAdvice = "EasyCLA's Auth0 M2M client has no client grant for the member-service audience (SSM cla-member-service-auth0-audience-): ops must add it (README §2), then re-run." + func SuggestError(err error) string { msg := err.Error() + var authErr *member_service.AuthError switch { + case errors.As(err, &authErr) && authErr.Token: + if authErr.Status == 403 { + return clientGrantAdvice + } + return fmt.Sprintf("the Auth0 token request for the member-service audience failed (%d): check SSM cla-auth0-platform-client-id/-secret- and cla-member-service-auth0-audience- (README §2), then re-run.", authErr.Status) case strings.Contains(msg, "client-grant"), strings.Contains(msg, "not authorized to access resource server"): - return "EasyCLA's Auth0 M2M client has no client grant for the member-service audience (SSM cla-member-service-auth0-audience-): ops must add it (README §2), then re-run." + return clientGrantAdvice case strings.Contains(msg, "(403)"), strings.Contains(msg, "(401)"): return "member-service refused the call: the client needs auditor (GET /b2b_orgs) and global_org_admin (POST /b2b_orgs) in the member-service Heimdall ruleset (README §2), then re-run." case strings.Contains(msg, ErrNotConfigured.Error()): diff --git a/cla-backend-go/orgimport/run.go b/cla-backend-go/orgimport/run.go index d1af8bcb9..62094a1e8 100644 --- a/cla-backend-go/orgimport/run.go +++ b/cla-backend-go/orgimport/run.go @@ -289,6 +289,9 @@ func classify(ctx context.Context, deps Deps, g *Group, mapping *Mapping, shared // lookupOrg records the org-service view of the group's old id. func lookupOrg(ctx context.Context, deps Deps, g *Group) { + if g.OrgStatus != "" { + return + } org, err := deps.Orgs.GetOrganization(ctx, g.OldID) switch { case err == nil: @@ -357,7 +360,7 @@ func livenessGuard(groups []*Group) { continue } g.Live, g.Route, g.ManualReason = LiveError, "", "" - g.Err = fmt.Errorf("liveness check failed for %s: member-service answered (403) for every Account checked (%d) and 200 for none: the tool's client cannot see any b2b_org (access tuple missing?): %w", g.OldID, unregistered, g.liveErr) + g.Err = fmt.Errorf("liveness check failed for %s: member-service answered 200 or 404 for no Account and 403 for %d: the tool's client cannot see any b2b_org (access tuple missing?): %w", g.OldID, unregistered, g.liveErr) } }