Skip to content

chore(auth): update GitHub and GitLab auth to use secure sign-in resolvers#4347

Open
JessicaJHee wants to merge 1 commit intoredhat-developer:mainfrom
JessicaJHee:pull-secure-git-resolvers
Open

chore(auth): update GitHub and GitLab auth to use secure sign-in resolvers#4347
JessicaJHee wants to merge 1 commit intoredhat-developer:mainfrom
JessicaJHee:pull-secure-git-resolvers

Conversation

@JessicaJHee
Copy link
Member

Description

Pull in the resolvers added upstream to improve the security of the default sign-in resolver, as they now resolve on an immutable user identifier. There is minimal impact to customers, the default configuration will still work out of the box. There will/ should be a release note noting this enhancement.

See the description for more details on the motivation:

Which issue(s) does this PR fix

PR acceptance criteria

Ensure the auth provider E2E tests pass

Please make sure that the following steps are complete:

  • GitHub Actions are completed and successful
  • Unit Tests are updated and passing
  • E2E Tests are updated and passing
  • Documentation is updated if necessary (requirement for new features)
  • Add a screenshot if the change is UX/UI related

How to test changes / Special notes to the reviewer

@rhdh-qodo-merge
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

RHIDP-11591 - Partially compliant

Compliant requirements:

  • Default GitHub and GitLab sign-in resolvers switched to secure immutable-ID based resolvers.
  • Update auth-provider E2E tests to reflect the new default resolver behavior (QE impacted).

Non-compliant requirements:

  • Update GitHub and GitLab authentication documentation to reflect the new default resolvers (documentation impacted).

Requires further human verification:

  • Pull in / align with upstream Backstage 1.48.0 changes that introduce these resolvers.
  • Provide/ensure upstream documentation updates (design docs, release notes, etc.).
  • Provide technical enablement / demo.
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🔒 No security concerns identified
⚡ Recommended focus areas for review

Config Migration

Changing the default sign-in resolver to userIdMatchingUserEntityAnnotation may require that catalog user entities are annotated with the expected immutable ID keys for GitHub/GitLab. Verify that out-of-the-box defaults, sample catalogs, and existing deployments include/produce the required annotations; otherwise sign-in may fail with "no user found" until users update their catalog.

case 'github':
  return createOAuthProviderFactory({
    authenticator: githubAuthenticator,
    ...applySignInResolvers({
      signInResolver:
        githubSignInResolvers.userIdMatchingUserEntityAnnotation(),
      signInResolverFactories: {
        ...githubSignInResolvers,
        ...commonSignInResolvers,
      },
    }),
  });
case 'gitlab':
  return createOAuthProviderFactory({
    authenticator: gitlabAuthenticator,
    ...applySignInResolvers({
      signInResolver:
        gitlabSignInResolvers.userIdMatchingUserEntityAnnotation(),
      signInResolverFactories: {
        ...gitlabSignInResolvers,
        ...commonSignInResolvers,
      },
📚 Focus areas based on broader codebase context

Test Robustness

The new test hardcodes the GitHub username (rhdhqeauth1) and uses a fixed page.waitForTimeout(3000), which can make E2E runs flaky across environments and harder to configure in CI. Prefer using environment-driven credentials and condition-based waits (URL/element readiness) to match the existing auth-provider E2E patterns. (Ref 1)

test("Login with Github usernameMatchingUserEntityName resolver", async () => {
  //A github sign-in resolver that looks up the user using their github username as the entity name.
  await deployment.setGithubResolver("usernameMatchingUserEntityName", false);
  await deployment.updateAllConfigs();
  await deployment.restartLocalDeployment();
  await page.waitForTimeout(3000);
  await deployment.waitForDeploymentReady();

  // wait for rhdh first sync and portal to be reachable
  await deployment.waitForSynced();

  const login = await common.githubLogin(
    "rhdhqeauth1",
    process.env.AUTH_PROVIDERS_GH_USER_PASSWORD,
    process.env.AUTH_PROVIDERS_GH_USER_2FA,
  );
  expect(login).toBe("Login successful");

  await uiHelper.verifyAlertErrorMessage(
    NO_USER_FOUND_IN_CATALOG_ERROR_MESSAGE,
  );
  await context.clearCookies();
});

Reference reasoning: The existing Playwright auth E2E flow uses process.env.* for user identity/credentials and relies on explicit page state checks (e.g., waitForURL, expect(...).toBeVisible()/toBeEnabled()) rather than fixed sleeps, making tests more portable and less timing-sensitive.

📄 References
  1. redhat-developer/rhdh/e2e-tests/playwright/e2e/auth-providers/oidc.spec.ts [21-536]

@rhdh-qodo-merge
Copy link

PR Type

Enhancement


Description

  • Update default GitHub and GitLab sign-in resolvers to use secure userIdMatchingUserEntityAnnotation

  • Add E2E test for legacy usernameMatchingUserEntityName resolver compatibility

  • Update test documentation to reflect new default resolver behavior

  • Improve resolver version compatibility documentation


File Walkthrough

Relevant files
Enhancement
authProvidersModule.ts
Update default sign-in resolvers to secure user ID matching

packages/backend/src/modules/authProvidersModule.ts

  • Changed default GitHub sign-in resolver from
    usernameMatchingUserEntityName to userIdMatchingUserEntityAnnotation
  • Changed default GitLab sign-in resolver from
    usernameMatchingUserEntityName to userIdMatchingUserEntityAnnotation
  • Both resolvers remain available through signInResolverFactories for
    backward compatibility
+2/-2     
Tests
github.spec.ts
Add legacy resolver E2E test and update documentation       

e2e-tests/playwright/e2e/auth-providers/github.spec.ts

  • Updated supported resolvers documentation to indicate
    userIdMatchingUserEntityAnnotation as default for version >=1.10.x
  • Added new E2E test for legacy usernameMatchingUserEntityName resolver
  • Test verifies backward compatibility and expected error handling when
    user not found
+26/-1   
Documentation
gitlab.spec.ts
Add GitLab supported resolvers documentation                         

e2e-tests/playwright/e2e/auth-providers/gitlab.spec.ts

  • Added supported resolvers documentation block for GitLab provider
  • Documents userIdMatchingUserEntityAnnotation as default for version
    >=1.10.x
  • Documents legacy usernameMatchingUserEntityName as default for version
    <=1.9.x
+8/-0     

@rhdh-qodo-merge
Copy link

rhdh-qodo-merge bot commented Mar 3, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Add a corresponding GitLab E2E test

The PR adds an E2E test for the old default GitHub resolver but not for
GitLab's. To ensure consistent test coverage, a similar test for the
usernameMatchingUserEntityName resolver should be added to the GitLab test
suite.

Examples:

e2e-tests/playwright/e2e/auth-providers/github.spec.ts [150-172]
  test("Login with Github usernameMatchingUserEntityName resolver", async () => {
    //A github sign-in resolver that looks up the user using their github username as the entity name.
    await deployment.setGithubResolver("usernameMatchingUserEntityName", false);
    await deployment.updateAllConfigs();
    await deployment.restartLocalDeployment();
    await page.waitForTimeout(3000);
    await deployment.waitForDeploymentReady();

    // wait for rhdh first sync and portal to be reachable
    await deployment.waitForSynced();

 ... (clipped 13 lines)
e2e-tests/playwright/e2e/auth-providers/gitlab.spec.ts [17]
test.describe("Configure GitLab Provider", async () => {

Solution Walkthrough:

Before:

// e2e-tests/playwright/e2e/auth-providers/gitlab.spec.ts

/* SUPPORTED RESOLVERS
GITLAB:
    [x] userIdMatchingUserEntityAnnotation -> (Default >=1.10.x)
    [x] usernameMatchingUserEntityName -> (Default <=1.9.x)
    ...
*/

test.describe("Configure GitLab Provider", async () => {
  // ... setup ...

  // ... other tests exist ...

  // No test for the old default 'usernameMatchingUserEntityName' resolver.
});

After:

// e2e-tests/playwright/e2e/auth-providers/gitlab.spec.ts

/* SUPPORTED RESOLVERS
GITLAB:
    [x] userIdMatchingUserEntityAnnotation -> (Default >=1.10.x)
    [x] usernameMatchingUserEntityName -> (Default <=1.9.x)
    ...
*/

test.describe("Configure GitLab Provider", async () => {
  // ... setup ...

  test("Login with Gitlab usernameMatchingUserEntityName resolver", async () => {
    await deployment.setGitlabResolver("usernameMatchingUserEntityName", false);
    await deployment.updateAllConfigs();
    await deployment.restartLocalDeployment();
    // ...
    await common.gitlabLogin(...);
    await uiHelper.verifyAlertErrorMessage(NO_USER_FOUND_IN_CATALOG_ERROR_MESSAGE);
  });

  // ... other tests ...
});
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies an inconsistency in E2E test coverage; a test was added for the old GitHub resolver but not for the corresponding GitLab resolver, leaving a gap in verification for a core change.

Medium
Possible issue
Add version-based sign-in resolver

Make the GitHub sign-in resolver selection conditional based on a version or
configuration flag to maintain backward compatibility.

packages/backend/src/modules/authProvidersModule.ts [157-158]

+const githubResolver = process.env.AUTH_BACKEND_VERSION >= "1.10.0"
+  ? githubSignInResolvers.userIdMatchingUserEntityAnnotation()
+  : githubSignInResolvers.usernameMatchingUserEntityName();
+...
 signInResolver:
-  githubSignInResolvers.userIdMatchingUserEntityAnnotation(),
+  githubResolver,

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the hardcoded change of the default resolver could be problematic. Making it configurable or version-dependent improves the flexibility and backward compatibility of the authentication module.

Medium
General
Replace fixed sleep with readiness wait

Replace the fixed page.waitForTimeout(3000) with
deployment.waitForDeploymentReady() to prevent flaky tests by waiting for the
deployment to be ready.

e2e-tests/playwright/e2e/auth-providers/github.spec.ts [154-155]

 await deployment.restartLocalDeployment();
-await page.waitForTimeout(3000);
+await deployment.waitForDeploymentReady();
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that using a fixed timeout with page.waitForTimeout() is a bad practice that can lead to flaky tests. Removing it and relying on a readiness check improves test stability.

Low
  • Update

@JessicaJHee JessicaJHee changed the title chore(auth): Update GitHub and GitLab auth to use secure sign-in resolvers chore(auth): update GitHub and GitLab auth to use secure sign-in resolvers Mar 3, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 3, 2026

Image was built and published successfully. It is available at:

@JessicaJHee
Copy link
Member Author

/test e2e-ocp-operator-auth-providers-nightly

@JessicaJHee JessicaJHee force-pushed the pull-secure-git-resolvers branch from f46ba6f to 37e94ec Compare March 4, 2026 02:47
@JessicaJHee
Copy link
Member Author

/test e2e-ocp-operator-auth-providers-nightly

@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

Image was built and published successfully. It is available at:

@JessicaJHee JessicaJHee force-pushed the pull-secure-git-resolvers branch from 37e94ec to d8536f7 Compare March 9, 2026 17:43
@JessicaJHee
Copy link
Member Author

/test e2e-ocp-operator-auth-providers-nightly

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2026

Image was built and published successfully. It is available at:

…lvers

Signed-off-by: Jessica He <jhe@redhat.com>
@JessicaJHee JessicaJHee force-pushed the pull-secure-git-resolvers branch from d8536f7 to 0b96433 Compare March 10, 2026 21:33
@JessicaJHee
Copy link
Member Author

/test e2e-ocp-operator-auth-providers-nightly

@sonarqubecloud
Copy link

@github-actions
Copy link
Contributor

Image was built and published successfully. It is available at:

@zdrapela
Copy link
Member

/test e2e-ocp-operator-auth-providers-nightly

@zdrapela
Copy link
Member

/test e2e-ocp-helm

@openshift-ci
Copy link

openshift-ci bot commented Mar 11, 2026

@JessicaJHee: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-ocp-operator-auth-providers-nightly 0b96433 link false /test e2e-ocp-operator-auth-providers-nightly

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants