Skip to content

fix(auth): drop acr_values from OAuth flow, make no-acr the default - [PLT-107095] #608

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.

Sign up for GitHub

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

Draft
Raina451 wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(auth): drop acr_values from OAuth flow, make no-acr the default - [PLT-107095] #608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion samples/data-fabric-app-angular/src/app/core/auth.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ export class AuthService {
this.isLoading.set(true)
this.error.set(null)
try {
this.sdk.setMultiLogin()
await this.sdk.initialize()
this.isAuthenticated.set(this.sdk.isAuthenticated())
} catch (err) {
Expand Down
24 changes: 4 additions & 20 deletions src/core/auth/service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,9 @@ import { hasOAuthConfig } from '../config/sdk-config';
import { isBrowser } from '../../utils/platform';
import { IDENTITY_ENDPOINTS } from '../../utils/constants/endpoints';

const GUID_REGEX = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;

export class AuthService {
private config: Config;
private tokenManager: TokenManager;
private skipAcrValues: boolean = false;

constructor(config: Config, executionContext: ExecutionContext) {
// Only use stored OAuth context when completing an active callback (URL has ?code=).
Expand Down Expand Up @@ -118,15 +115,6 @@ export class AuthService {
return this.tokenManager;
}

/**
* Enables the UiPath login picker during OAuth sign-in.
*
* @internal
*/
public setMultiLogin(): void {
this.skipAcrValues = true;
}

/**
* Authenticates the user based on the provided SDK configuration.
* This method handles OAuth 2.0 authentication flow only.
Expand Down Expand Up @@ -340,10 +328,6 @@ export class AuthService {
scope: string;
state?: string;
}): string {
const orgName = this.config.orgName;
const isGuid = GUID_REGEX.test(orgName);
const acrValues = isGuid ? `tenant:${orgName}` : `tenantName:${orgName}`;

const queryParams = new URLSearchParams({
response_type: 'code',
client_id: params.clientId,
Expand All @@ -354,10 +338,10 @@ export class AuthService {
state: params.state || this.generateCodeVerifier().slice(0, 16)
});

const authorizeUrl = `${this.config.baseUrl}/${IDENTITY_ENDPOINTS.AUTHORIZE}?${queryParams.toString()}`;
return this.skipAcrValues
? authorizeUrl
: `${authorizeUrl}&acr_values=${acrValues}`;
// acr_values is intentionally omitted: Identity resolves the target org from
// client_id, and sending acr_values routes directly to the org's SAML IdP,
// which blocks Basic Auth users in mixed-auth orgs.
return `${this.config.baseUrl}/${IDENTITY_ENDPOINTS.AUTHORIZE}?${queryParams.toString()}`;
}

/**
Expand Down
5 changes: 0 additions & 5 deletions src/core/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,6 @@ export interface IUiPath {
*/
initialize(): Promise<void>;

/**
* Enables the UiPath login picker during OAuth sign-in.
*/
setMultiLogin(): void;

/**
* Check if the SDK has been initialized
*/
Expand Down
14 changes: 0 additions & 14 deletions src/core/uipath.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ export class UiPath implements IUiPath {
#authService?: AuthService;
#initialized: boolean = false;
#partialConfig?: PartialUiPathConfig;
#multiLogin: boolean = false;
// Folder key sourced only from `<meta name="uipath:folder-key">` (coded-app
// deployments). Not accepted via the public constructor; lives here so the
// SDK can flow it through to BaseService.config without polluting BaseConfig.
Expand Down Expand Up @@ -101,9 +100,6 @@ export class UiPath implements IUiPath {

const executionContext = new ExecutionContext();
this.#authService = new AuthService(internalConfig, executionContext);
if (this.#multiLogin) {
this.#authService.setMultiLogin();
}
this.#config = internalConfig;

// Store internals in SDKInternalsRegistry (not visible on instance).
Expand Down Expand Up @@ -212,16 +208,6 @@ export class UiPath implements IUiPath {
}
}

/**
* Enables the UiPath login picker during OAuth sign-in.
*
* @internal
*/
public setMultiLogin(): void {
this.#multiLogin = true;
this.#authService?.setMultiLogin();
}

/**
* Check if the SDK has been initialized
*/
Expand Down
44 changes: 14 additions & 30 deletions tests/unit/core/auth/service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ describe('AuthService', () => {
expect(url).toContain('offline_access');
});

it('should include all required OAuth params alongside acr_values', () => {
it('should include all required OAuth params', () => {
const service = createService(TEST_CONSTANTS.ORGANIZATION_ID);
const url = service.getAuthorizationUrl({ clientId, redirectUri, codeChallenge, scope });
const params = new URLSearchParams(url.split('?')[1]);
Expand All @@ -50,37 +50,21 @@ describe('AuthService', () => {
expect(params.get('code_challenge')).toBe(codeChallenge);
expect(params.get('code_challenge_method')).toBe('S256');
expect(params.get('scope')).toContain(scope);
expect(params.get('acr_values')).toBe(`tenantName:${TEST_CONSTANTS.ORGANIZATION_ID}`);
});

it('should set acr_values with tenant: prefix when orgName is a GUID', () => {
const service = createService(TEST_CONSTANTS.GUID_ORG_ID);
const url = service.getAuthorizationUrl({ clientId, redirectUri, codeChallenge, scope });
const parsedUrl = new URL(url);
expect(parsedUrl.searchParams.get('acr_values')).toBe(`tenant:${TEST_CONSTANTS.GUID_ORG_ID}`);
});

it('should set acr_values with tenantName: prefix when orgName is a human-readable name', () => {
const service = createService(TEST_CONSTANTS.ORGANIZATION_ID);
const url = service.getAuthorizationUrl({ clientId, redirectUri, codeChallenge, scope });
const parsedUrl = new URL(url);
expect(parsedUrl.searchParams.get('acr_values')).toBe(`tenantName:${TEST_CONSTANTS.ORGANIZATION_ID}`);
});

it('should set acr_values with tenantName: prefix for a GUID-like string with wrong segment length', () => {
const service = createService(TEST_CONSTANTS.INVALID_GUID_ORG_ID);
const url = service.getAuthorizationUrl({ clientId, redirectUri, codeChallenge, scope });
const parsedUrl = new URL(url);
expect(parsedUrl.searchParams.get('acr_values')).toBe(`tenantName:${TEST_CONSTANTS.INVALID_GUID_ORG_ID}`);
});

it('should omit acr_values when multi-login is enabled', () => {
const service = createService(TEST_CONSTANTS.ORGANIZATION_ID);
service.setMultiLogin();
const url = service.getAuthorizationUrl({ clientId, redirectUri, codeChallenge, scope });
const parsedUrl = new URL(url);
expect(parsedUrl.searchParams.has('acr_values')).toBe(false);
expect(url).not.toContain('acr_values=');
// acr_values is never sent: Identity resolves the org from client_id, and
// sending it routes directly to the org's SAML IdP, blocking Basic Auth users.
it('should never emit acr_values, regardless of orgName format', () => {
for (const orgName of [
TEST_CONSTANTS.ORGANIZATION_ID,
TEST_CONSTANTS.GUID_ORG_ID,
TEST_CONSTANTS.INVALID_GUID_ORG_ID,
]) {
const service = createService(orgName);
const url = service.getAuthorizationUrl({ clientId, redirectUri, codeChallenge, scope });
expect(new URL(url).searchParams.has('acr_values')).toBe(false);
expect(url).not.toContain('acr_values');
}
});
});

Expand Down
26 changes: 0 additions & 26 deletions tests/unit/core/uipath.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ const mockTokenManager = {
};

const mockLogout = vi.fn();
const mockSetMultiLogin = vi.fn();

vi.mock('../../../src/core/auth/service', () => {
const AuthService: any = vi.fn().mockImplementation(function () { return ({
Expand All @@ -26,7 +25,6 @@ vi.mock('../../../src/core/auth/service', () => {
getToken: () => 'mock-access-token',
authenticateWithSecret: vi.fn(),
authenticate: vi.fn().mockResolvedValue(true),
setMultiLogin: mockSetMultiLogin,
logout: mockLogout
}); });

Expand Down Expand Up @@ -104,30 +102,6 @@ describe('UiPath Core', () => {
expect(sdk.isInitialized()).toBe(false); // OAuth requires initialize()
});

it('should enable multi-login on OAuth config', () => {
mockSetMultiLogin.mockClear();
const sdk = new UiPath({
baseUrl: TEST_CONSTANTS.BASE_URL,
orgName: TEST_CONSTANTS.ORGANIZATION_ID,
tenantName: TEST_CONSTANTS.TENANT_ID,
clientId: TEST_CONSTANTS.CLIENT_ID,
redirectUri: 'http://localhost:3000/callback',
scope: 'offline_access'
});

sdk.setMultiLogin();

expect(mockSetMultiLogin).toHaveBeenCalledOnce();
});

it('should allow multi-login before config is loaded', () => {
mockSetMultiLogin.mockClear();
const sdk = new UiPath();

expect(() => sdk.setMultiLogin()).not.toThrow();
expect(mockSetMultiLogin).not.toHaveBeenCalled();
});

it('should validate required config fields', async () => {
const sdk = new UiPath({
baseUrl: '',
Expand Down
Loading