feat(app-defaults): add app-auth and app-integration#2693
feat(app-defaults): add app-auth and app-integration#2693gashcrumb merged 1 commit intoredhat-developer:mainfrom
Conversation
gashcrumb
requested review from
a team,
christoph-jerolimov and
rohitkrai03
as code owners
Missing ChangesetsThe following package(s) are changed by this PR but do not have a changeset:
See CONTRIBUTING.md for more information about how to add changesets. Changed Packages
|
Review Summary by QodoAdd app-auth and app-integrations plugins for new frontend system
WalkthroughsDescription• Add app-auth plugin with multi-provider sign-in page and OAuth2 APIs • Add app-integrations plugin for default SCM integrations and auth wiring • Suppress noisy CSS parse errors in jsdom test environment • Update ESLint config to handle .cjs files and add lint-staged configuration • Integrate new plugins into sample app with updated dependencies Diagramflowchart LR
A["app-auth plugin"] -->|"provides sign-in page"| B["Sample App"]
C["app-integrations plugin"] -->|"provides SCM auth"| B
D["Jest environment"] -->|"suppress CSS errors"| E["Test setup"]
F["ESLint config"] -->|"handle .cjs files"| G["Lint pipeline"]
B -->|"uses"| H["New Frontend System"]
File Changes1. workspaces/app-defaults/plugins/app-auth/package.json
|
Code Review by Qodo
1. Jest env path invalid
|
gashcrumb
force-pushed
the
RHIDP-12359
branch
2 times, most recently
from
00f04dd to
6b21b19
Compare
![rhdh-qodo-merge[bot]](https://avatars.githubusercontent.com/in/1961108?s=60&v=4){kind=small}
gashcrumb
force-pushed
the
RHIDP-12359
branch
2 times, most recently
from
cfc4aac to
00941ba
Compare
|
not sure the best version to use in the package.json files for these, maybe I should have just kept them at 0.0.1. |
This change adds an 'app-auth' plugin which provides the sign-in page and related authentiction module API factories to an app using the new frontend system. This change also adds a second 'app-integration' plugin which provides a default ScmAuth configuration. In both cases these plugins bring code migrated from the old frontend system setup in the RHDH repository. This change also adds a change to cleanup some noisy non-outcome altering output on the console when running the tests, along with some related updates to the eslint config so that the lint run by the pre-commit hook still works. This change also contains some updates to the app-defaults workspace README. Based on review feedback, this change also includes an improved error message should the app be misconfigured such that the sign-in page would not render properly Assisted-By: Cursor Desktop
|
|
|
||
| auth: | ||
| # Required for RHDH app-auth SignInPage (guest in dev) and local e2e | ||
| environment: development |
There was a problem hiding this comment.
just a question: shouldn't environment be outside auth? can we have multiple environments defined in the same file?
There was a problem hiding this comment.
This is the typical location for this variable, for example here from RHDH. I believe yes, under here multiple environments can be defined per auth provider, however this sets the effective environment. Also this is for the local development config for these plugins, ensuring the development app can be started properly.
| gitlab: oauthStub, | ||
| azure: oauthStub, | ||
| bitbucket: oauthStub, | ||
| configApi, |
There was a problem hiding this comment.
I assume integrations.* is configApi?
There was a problem hiding this comment.
yep, normally configApi is a service a plugin would import in the API to access the config, this test mocks that.
| configApi, | ||
| }); | ||
|
|
||
| // Two GitHub hosts + default singletons for gitlab, azure, bitbucket. |
|
|
||
| import { mergeScmAuthFromDeps } from './mergeScmAuthFromDeps'; | ||
|
|
||
| describe('mergeScmAuthFromDeps', () => { |
There was a problem hiding this comment.
Just a comment, non-blocking at all. We could increase the test base to cover all the providers. Currently, we only test github. I know it will be the same for the rest of the providers 😄
There was a problem hiding this comment.
yep, for sure. Worth discussing the best approach here down the line, as this could be done somewhat here but also some of this kind of testing could make more sense when there's an entry in the overlay repo, allowing for tests to be done against actual systems as needed. At this level probably the tests should use mocks rather than real services.
| "@backstage/core-plugin-api": "^1.12.4", | ||
| "@backstage/frontend-plugin-api": "^0.15.1", | ||
| "@backstage/plugin-app": "^0.4.2", | ||
| "@mui/material": "5.18.0" |
There was a problem hiding this comment.
mui/material yes, the other dependencies I don't think so. The backstage-cli would override these pinned versions to use ^ on the next backstage version bump anyways.
| icon: () => null, | ||
| }, | ||
| defaultScopes: ['openid', 'email', 'profile'], | ||
| environment: configApi.getOptionalString('auth.environment'), |
There was a problem hiding this comment.
So we can't have auth providers using different environments?
There was a problem hiding this comment.
they can be configured, but afaik there can only be one effective enabled environment. I should mention that this is based on the behavior of the existing sign-in page from the old frontend system that the plugin brings in.
3 participants
This change adds an 'app-auth' plugin which provides the sign-in page and related authentiction module API factories to an app using the new frontend system. This change also adds a second 'app-integration' plugin which provides a default ScmAuth configuration. In both cases these plugins bring code migrated from the old frontend system setup in the RHDH repository. This change also adds a change to cleanup some noisy non-outcome altering output on the console when running the tests, along with some related updates to the eslint config so that the lint run by the pre-commit hook still works. Based on review feedback, this change also includes an improved error message should the app be misconfigured such that the sign-in page would not render properly
Fixes RHIDP-12359
Assisted-By: Cursor Desktop
Hey, I just made a Pull Request!
✔️ Checklist
Exporting these modules as dynamic plugins and starting RHDH backend's
start:nextscript along with the relevant configuration that enables these modules provides a sign-in page that uses the correct authentication method as well as the ability to sign in using a secondary auth provider to access source control, for example:When starting the workspace in development mode using
yarn startthe guest authentication method can be used to sign-in.