Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

fix: refactor authProviderModule and dangerouslyAllowSignInWithoutUserInCatalog config #2354

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JessicaJHee
Copy link
Member

@JessicaJHee JessicaJHee commented Feb 10, 2025

Description

  • Refactor and clean up the behaviour of dangerouslyAllowSignInWithoutUserInCatalog config
    • dangerouslyAllowSignInWithoutUserInCatalog is now a resolver level config and applies to each resolver declaratively
    • This config now works with all supported resolvers
    • All auth providers now support declarative resolver definition
  • Update auth provider e2e tests

Example usage:

auth:
  providers:
    github:
      development:
        clientId: ${CLIENT_ID}
        clientSecret: ${CLIENT_SECRET}
        signIn:
          resolvers:
            - resolver: usernameMatchingUserEntityName
              dangerouslyAllowSignInWithoutUserInCatalog: true

See upstream changes included in the patches here

  • This PR introduces a lot of patches, but the intention is to only maintain them for the 1.5 release since fixing the bug is of high priority, then once these changes are proposed and merged upstream, we can remove them.

The new error message with this is:
"Login failed; caused by Error: Failed to sign-in, unable to resolve user identity. Please verify that your catalog contains the expected User entities that would match your configured sign-in resolver. For non-production environments, manually provision the user or disable the user provisioning requirement by setting the dangerouslyAllowSignInWithoutUserInCatalog option."

Notes/Changes on TP auth providers:

auth0 resolver in RHDH resolves by fullProfile.id

  • Backstage only supports the common resolvers
  • Since we haven’t tested this auth provider and upstream has, we should use their resolvers

gcp-iap resolver in RHDH resolves by iapToken.email.split('@')[0]

onelogin resolver in RHDH resolves by fullProfile.id

SAML doesn’t have it’s own auth provider module

  • Only has partial support, see issue
  • removed in RHDH

Which issue(s) does this PR fix

PR acceptance criteria

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

Manual testing has been done for GitHub and Microsoft resolvers

Copy link

openshift-ci bot commented Feb 10, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link

openshift-ci bot commented Feb 10, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jessicajhee. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@JessicaJHee JessicaJHee changed the title Fix auth config fix: refactor authProviderModule and dangerouslyAllowSignInWithoutUserInCatalog config Feb 10, 2025
@JessicaJHee JessicaJHee changed the title fix: refactor authProviderModule and dangerouslyAllowSignInWithoutUserInCatalog config fix: refactor authProviderModule and dangerouslyAllowSignInWithoutUserInCatalog config Feb 10, 2025
Copy link
Contributor

@JessicaJHee JessicaJHee force-pushed the fix-auth-config branch 3 times, most recently from 42907c8 to 77e816b Compare February 12, 2025 17:11
Copy link
Contributor

Copy link
Contributor

Copy link
Contributor

Copy link
Contributor

Copy link
Contributor

@JessicaJHee
Copy link
Member Author

auth e2e tests are broken due to UI changes - will be fixed in this PR

Copy link
Contributor

Copy link
Contributor

@JessicaJHee
Copy link
Member Author

/retest

Copy link

openshift-ci bot commented Feb 24, 2025

@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-tests 8d43e8d link true /test e2e-tests

Full PR test history. Your PR dashboard.

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants