feat: DH-18583: Saml Login #222
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
End-to-end Test Summary
Detailed Test Results
Failed Test SummaryNo failed tests ✨Flaky Test SummaryNo flaky tests detected. ✨ |
Unit Test Summary
Detailed Test Results
Failed Test SummaryNo failed tests ✨Flaky Test SummaryNo flaky tests detected. ✨ |
There was a problem hiding this comment.
PR Overview
This PR adds support for SAML login in the extension by integrating SAML authentication flow and UI components. Key changes include:
- New SAML auth provider implementation with workflow and URI handling.
- Updates to authentication UI prompts and login flow in the user controller.
- Enhancements to utility functions and constants to support SAML scopes and session keys.
Reviewed Changes
| File | Description |
|---|---|
| src/util/dataUtils.spec.ts | Adds tests for SAML scope parsing functionality |
| src/providers/SamlAuthProvider.ts | Introduces SAML auth provider and associated workflow methods |
| src/services/UriEventHandler.ts | Provides a new URI event handler implementation |
| src/util/idUtils.ts | Implements session key generation with a base64-encoded random key |
| src/util/promiseUtils.ts | Adds helper for promise rejection with timeout |
| src/util/dataUtils.ts | Refactors SAML scopes parsing and authentication flow related utils |
| src/controllers/UserLoginController.ts | Updates login workflow to incorporate SAML authentication |
| src/util/uiUtils.ts | Refines UI prompts for multi authentication flows |
| src/common/constants.ts | Adds new constants for SAML provider configuration |
| src/controllers/ExtensionController.ts | Integrates the new SAML auth provider initialization in the extension |
| src/providers/index.ts | Exports the new SAML auth provider for usage by the extension |
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/util/idUtils.ts:24
- [nitpick] Consider replacing the custom random char generation with a cryptographically secure random method (e.g. using Node's crypto.randomBytes) to ensure the generated session key has proper entropy.
key += String.fromCharCode(Math.floor(Math.random() * 255));
src/providers/SamlAuthProvider.ts:96
- Ensure that the URI path is not empty or malformed before calling substring, so that an unexpected URI does not lead to a runtime error.
const stateId = uri.path.substring(1);
bmingles
commented
Feb 27, 2025
| * @param authConfig | ||
| * @returns The selected auth flow or null if cancelled. | ||
| */ | ||
| export async function promptForAuthFlow( |
There was a problem hiding this comment.
This was just a function + return type rename. Shouldn't be any logic change.
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DH-18583: Saml Login
Test Plan
You can use https://jxn-feature-test-saml.int.illumon.com:8123/ for testing Google SAML login (Note that Python isn't currently configured on the server, so you'll have to use Groovy)
e.g.
{ "url": "https://jxn-feature-test-saml.int.illumon.com:8123/", "experimentalWorkerConfig": { "heapSize": 0.5, "scriptLanguage": "Groovy" } },Clicking on the DH server node for in VS Code for this server should now prompt with the option to login with
GoogleorBasic Login.Google login should work in the following scenarios.
Basic Login should work as it always has. You can use iris user on this server.