Integrate and implement a library on login-page#1277
Conversation
There was a problem hiding this comment.
Pull request overview
This PR integrates the @dspace/idp-discovery library into the Angular login page to allow users to select an institution (IdP) and be redirected to a Shibboleth login flow.
Changes:
- Add
@dspace/idp-discoverydependency and lockfile entries. - Render
<ds-idp-discovery>on the login page and handle IdP selection via a redirect. - Provide
IDP_DISCOVERY_CONFIGapplication-wide for the discovery component.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/themes/custom/app/login-page/login-page.component.ts | Adds IdpDiscoveryComponent to themed login page standalone imports. |
| src/app/login-page/login-page.component.ts | Imports IdpDiscoveryComponent and adds onIdpSelected redirect handler. |
| src/app/login-page/login-page.component.html | Adds <ds-idp-discovery> to the login page template. |
| src/app/app.config.ts | Registers IDP_DISCOVERY_CONFIG provider for discovery feed/service text. |
| package.json | Adds @dspace/idp-discovery GitHub dependency reference. |
| package-lock.json | Locks the new dependency and updates transitive dependency graph. |
| onIdpSelected(idp: { entityID: string }): void { | ||
| const loginEndpoint = 'https://lindat.mff.cuni.cz/Shibboleth.sso/Login'; | ||
| window.location.href = `${loginEndpoint}?entityID=${encodeURIComponent(idp.entityID)}`; |
There was a problem hiding this comment.
onIdpSelected hardcodes the Shibboleth login endpoint URL in the component. This makes the login behavior environment-specific and hard to change across deployments; consider moving the endpoint into configuration (e.g., extending the existing IDP_DISCOVERY_CONFIG or app config) and injecting/reading it here instead of inlining the URL.
| onIdpSelected(idp: { entityID: string }): void { | ||
| const loginEndpoint = 'https://lindat.mff.cuni.cz/Shibboleth.sso/Login'; | ||
| window.location.href = `${loginEndpoint}?entityID=${encodeURIComponent(idp.entityID)}`; | ||
| } |
There was a problem hiding this comment.
New redirect behavior in onIdpSelected isn’t covered by the existing LoginPageComponent unit tests. Please add a spec verifying that the selected IdP’s entityID is URL-encoded and that navigation is triggered with the expected URL (mock/spyon window.location or wrap navigation in an injectable service to test safely).
| provide: IDP_DISCOVERY_CONFIG, | ||
| useValue: { | ||
| feedUrl: "https://lindat.mff.cuni.cz/Shibboleth.sso/DiscoFeed", | ||
| serviceName: 'Sign in via your institution', | ||
| helpText: 'If you cannot find your institution, use local login.', | ||
| maxResults: 25, | ||
| }, |
There was a problem hiding this comment.
The IDP_DISCOVERY_CONFIG provider hardcodes a deployment-specific feedUrl directly in app.config.ts. To keep environments configurable (and avoid editing code for different deployments), consider sourcing this from the existing app/environment configuration instead of inlining the URL here.
| "@angular/platform-server": "^20.3.14", | ||
| "@angular/router": "^20.3.14", | ||
| "@angular/ssr": "^20.3.10", | ||
| "@dspace/idp-discovery": "github:dataquest-dev/idp-discovery#release", |
There was a problem hiding this comment.
@dspace/idp-discovery is referenced via github:...#release (a moving branch/tag name). This can lead to non-reproducible installs when the release ref moves; prefer pinning to an immutable tag/version or a specific commit SHA (and keep the lockfile in sync).
| "@dspace/idp-discovery": "github:dataquest-dev/idp-discovery#release", | |
| "@dspace/idp-discovery": "github:dataquest-dev/idp-discovery#<PINNED_COMMIT_SHA>", |
Problem description
npm install github:dataquest-dev/idp-discovery#release --legacy-peer-deps