Skip to content

Added on-prem IDM Deployment type #21

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

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

Added on-prem IDM Deployment type #21

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
10d8ec8
modified for idm
skootrivir May 5, 2025
842addf
Changes for on prem IDM
skootrivir May 7, 2025
c756f58
ran lint
skootrivir May 12, 2025
706ffc6
Made changes based on the PR feedback
skootrivir May 14, 2025
de6b8e5
Made changes on parenthesis
skootrivir May 15, 2025
b0ff7e5
changed authenticateOps.ts
skootrivir Jun 4, 2025
f214452
Changed includeReadOnly
skootrivir Jun 4, 2025
c6dda67
Added AuthenticationApi test for on-prem idm
skootrivir Sep 24, 2025
cb11e18
Modified AuthenticateApi.test made a new record for the test
skootrivir Nov 10, 2025
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
31 changes: 30 additions & 1 deletion src/api/AuthenticateApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import util from 'util';

import { State } from '../shared/State';
import { debugMessage } from '../utils/Console';
import { getRealmPath } from '../utils/ForgeRockUtils';
import { generateAmApi } from './BaseApi';
import { generateAmApi, generateIdmApi } from './BaseApi';

const authenticateUrlTemplate = '%s/json%s/authenticate';
const authenticateWithServiceUrlTemplate = `${authenticateUrlTemplate}?authIndexType=service&authIndexValue=%s`;
Expand Down Expand Up @@ -73,3 +74,31 @@ export async function step({
}).post(urlString, body, config);
return data;
}

/**
*
* @param {any} body POST request body
* @param {any} config request config
* @returns Promise resolving to the authentication service response
*/
export async function stepIdm({
Copy link

@hfranklin hfranklin Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally I prefer consistency, but in this case I don't think the function name stepIdm makes sense. I think something like loginIdm or authenticateIdm would make more sense since IDM authentication doesn't have multiple steps like AM. Did I miss something here?

Copy link
Collaborator

@phalestrivir phalestrivir Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the reason he named it that is because there is a step function that does the usual AM authentication calls, so he called this one stepIdm since it is for IDM authentication which is why I didn't think too much of it. However, I agree that stepIdm is not the best name for it, since the reason the AM one is called step is because it is stepping through a journey to authenticate the Admin user when we create connection profiles. For IDM, we don't have steps to authenticate, it's just part of any request that is made to IDM to include the username/password to authenticate, so this would be a good change for you to make @skootrivir, assuming we can't find a better alternative to determine if the deployment is an IDM deployment as mentioned in the next comment.

Copy link
Author

@skootrivir skootrivir Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will re-name this to 'authenticateIdm'

body = {},
config = {},
state,
}: {
body?: object;
config?: object;
realm?: string;
service?: string;
state: State;
}): Promise<any> {
debugMessage({
message: `AuthenticateApi.stepIdm: function start `,
state,
});
const urlString = `${state.getHost()}/authentication?_action=login`;
const response = await generateIdmApi({
state,
}).post(urlString, body, config);
return response;
}
4 changes: 4 additions & 0 deletions src/api/BaseApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,10 @@ export function generateIdmApi({
...(state.getBearerToken() && {
Authorization: `Bearer ${state.getBearerToken()}`,
}),
...(!state.getBearerToken() && {
'X-OpenIDM-Username': state.getUsername(),
'X-OpenIDM-Password': state.getPassword(),
}),
},
httpAgent: getHttpAgent(),
httpsAgent: getHttpsAgent(state.getAllowInsecureConnection()),
Expand Down
78 changes: 68 additions & 10 deletions src/ops/AuthenticateOps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { createHash, randomBytes } from 'crypto';
import url from 'url';
import { v4 } from 'uuid';

import { step } from '../api/AuthenticateApi';
import { step, stepIdm } from '../api/AuthenticateApi';
import { getServerInfo, getServerVersionInfo } from '../api/ServerInfoApi';
import Constants from '../shared/Constants';
import { State } from '../shared/State';
Expand Down Expand Up @@ -155,12 +155,23 @@ let adminClientId = fidcClientId;
* @returns {string} cookie name
*/
async function determineCookieName(state: State): Promise<string> {
const data = await getServerInfo({ state });
let cookieName = null;
try {
const data = await getServerInfo({ state });
cookieName = data.cookieName;
} catch (e) {
if (
e.response?.status !== 401 ||
Copy link

@hfranklin hfranklin Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what scenarios would we get a 401 or Access Denied?

Copy link
Collaborator

@phalestrivir phalestrivir Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I don't think we should get a 401 here since the endpoint doesn't require any credentials, so we can remove that. We do need to keep the try-catch though since there is no server info endpoint for IDM, and we don't want the program to fail because of it. @skootrivir Could you remove this if check?

Copy link

@hfranklin hfranklin Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we already know the deployment type by the time this call is being made? If so, it seems like it would be better to check the state to see if this is an IDM deployment.

Copy link
Collaborator

@phalestrivir phalestrivir Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's the first time the connection is created, we don't know the deployment type, so this is necessary. I remember we tried to find a way to re-order things so that we try and get the cookie name after determining the deployment type, but it's not possible because we need the cookie name for determining the deployment type (at least for AM, Cloud, and ForgeOps deployments) since it is used in the requests that it makes.

e.response?.data.message !== 'Access Denied'
) {
throw e;
}
}
debugMessage({
message: `AuthenticateOps.determineCookieName: cookieName=${data.cookieName}`,
message: `AuthenticateOps.determineCookieName: cookieName=${cookieName}`,
state,
});
return data.cookieName;
return cookieName;
}

/**
Expand Down Expand Up @@ -335,6 +346,7 @@ async function determineDeploymentType(state: State): Promise<string> {
return deploymentType;

case Constants.CLASSIC_DEPLOYMENT_TYPE_KEY:
case Constants.IDM_DEPLOYMENT_TYPE_KEY:
debugMessage({
message: `AuthenticateOps.determineDeploymentType: end [type=${deploymentType}]`,
state,
Expand Down Expand Up @@ -409,10 +421,35 @@ async function determineDeploymentType(state: State): Promise<string> {
});
deploymentType = Constants.FORGEOPS_DEPLOYMENT_TYPE_KEY;
} else {
verboseMessage({
message: `Classic deployment`['brightCyan'] + ` detected.`,
state,
});
try {
const idmresponse = await stepIdm({
Copy link

@hfranklin hfranklin Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are just trying to determine if the server is a stand alone IDM deployment, there should be a better way to do this than attempting an authentication.

Copy link
Collaborator

@phalestrivir phalestrivir Sep 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hfranklin There probably is another way to do it, but I'm thinking that this might be the best way to do it, the reason being that typically when you are creating a new connection profile you want it to validate the credentials you are providing so that you aren't saving invalid credentials. This function should only be called when creating a connection profile when it tries to determine the deployment type for the first time (unless we are attempting to use Frodo without an existing connection profile, which is possible, you would just need to provide the username/password for every command), and when it is called using the authenticate endpoint like we're doing it helps validate that the credentials work against the deployment. For AM, Cloud, and ForgeOps deployments they do this by exchanging the username/password for a cookie or by exchanging the service account id/jwk for a token sometime after determining the deployment type. However, since we can't do this for IDM deployments, we would need to authenticate at some point, so we do it in this function when it is trying to determine deployment type.

Another way we could do it is instead have some other way to determine it's an IDM deployment (such as attempting to hit the serverinfo endpoint, which will only work for AM, Cloud, and ForgeOps), but we would still have to call this function later anyways just to validate the connection, so I think using this authenticate function hits two birds with one stone by determining the deployment type while also validating that the credentials are valid.

As a side note, creating a connection profile, you can include the --no-validate to not validate credentials when creating the connection profile, but if you do this you'll notice it doesn't save the deployment type to the connection profile because it never validated that the credentials you gave it actually work.

However, I do think the way this is currently implemented (referring to how Frodo determines deployment type and validates credentials) is very confusing and not obvious. I think that possibly refactoring and cleaning it all up like we were talking about today would be very good. I did do some refactoring changes to this file already when I was working on this PR for the Amster authentication changes, but I didn't modify or refactor the code that determines the deployment type, only the code that validates the credentials, so I think waiting until this PR and that one are merged in might be a good idea before we try to refactor this file.

Copy link

@hfranklin hfranklin Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main complaint is it isn't obvious what the intent of the code is from the way it is structured right now. I'm fine refactoring it after merging this and PR #33

body: {},
config: {},
state,
});
if (
idmresponse.status === 200 &&
idmresponse.data?.authorization.authLogin
) {
verboseMessage({
message:
`Ping Identity IDM deployment`['brightCyan'] +
` detected.`,
state,
});
deploymentType = Constants.IDM_DEPLOYMENT_TYPE_KEY;
} else {
verboseMessage({
message: `Classic deployment`['brightCyan'] + ` detected.`,
state,
});
}
} catch {
Copy link

@hfranklin hfranklin Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this may not be a good assumption that all exceptions indicate that this is not a stand alone IDM deployment

Copy link
Collaborator

@phalestrivir phalestrivir Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true, I think we should be at least checking for the 401 case (if the credentials are invalid), that's the only case I can think of that this would throw which wouldn't result in the deployment being something other than an IDM deployment. @skootrivir Would you be able to add this check in the catch statement?

verboseMessage({
message: `Classic deployment`['brightCyan'] + ` detected.`,
state,
});
}
}
}
}
Expand Down Expand Up @@ -471,7 +508,18 @@ async function getFreshUserSessionToken({
'X-OpenAM-Password': state.getPassword(),
},
};
let response = await step({ body: {}, config, state });
let response;
try {
response = await step({ body: {}, config, state });
} catch (e) {
if (
e.response?.status !== 401 ||
e.response?.data.message !== 'Access Denied'
) {
throw e;
}
return null;
}

let skip2FA = null;
let steps = 0;
Expand Down Expand Up @@ -553,6 +601,7 @@ async function getUserSessionToken(
otpCallbackHandler: otpCallback,
state,
});
if (!token) return token;
Copy link

@hfranklin hfranklin Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is either incorrect or it is unclear what condition you are handling. If the token is falsey, why are you returning it?

Copy link
Collaborator

@phalestrivir phalestrivir Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for the if statement is that it is possible for the getFreshUserSessionToken to return null, and I believe it does return null if it's an IDM deployment (since the method is intended only for deployments with AM where it can receive a token), and so we are returning null to indicate that it wasn't able to get any tokens. However, it's been a while since I looked at it, so it could very well be unnecessary. @skootrivir Would you be able to confirm this is necessary?

Copy link

@hfranklin hfranklin Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that getFreshUserSessionToken could return null. My point is it is confusing to return token if token is falsey. It would be better to return null, undefined, false, or nothing depending on what the API contract is with the calling functions.

Copy link
Collaborator

@phalestrivir phalestrivir Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean. I think we just return null then in this case if the token is falsey since that's one of the possible expected return values for that function. @skootrivir, would you be able to change this line return null instead?

Copy link
Author

@skootrivir skootrivir Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course!

token.from_cache = false;
debugMessage({
message: `AuthenticateOps.getUserSessionToken: fresh`,
Expand Down Expand Up @@ -940,6 +989,7 @@ async function determineDeploymentTypeAndDefaultRealmAndVersion(
state,
});
state.setDeploymentType(await determineDeploymentType(state));
if (state.getDeploymentType() === Constants.IDM_DEPLOYMENT_TYPE_KEY) return;
determineDefaultRealm(state);
debugMessage({
message: `AuthenticateOps.determineDeploymentTypeAndDefaultRealmAndVersion: realm=${state.getRealm()}, type=${state.getDeploymentType()}`,
Expand Down Expand Up @@ -1162,7 +1212,7 @@ export async function getTokens({
});
const token = await getUserSessionToken(callbackHandler, state);
if (token) state.setUserSessionTokenMeta(token);
if (usingConnectionProfile && !token.from_cache) {
if (usingConnectionProfile && (!token || !token.from_cache)) {
saveConnectionProfile({ host: state.getHost(), state });
}
await determineDeploymentTypeAndDefaultRealmAndVersion(state);
Expand Down Expand Up @@ -1191,6 +1241,14 @@ export async function getTokens({
else {
throw new FrodoError(`Incomplete or no credentials`);
}
// Return IDM tokens for IDM deployment type
if (state.getDeploymentType() === Constants.IDM_DEPLOYMENT_TYPE_KEY) {
return {
subject: state.getUsername(),
host: state.getHost(),
realm: state.getRealm() ? state.getRealm() : 'root',
};
}
if (
state.getCookieValue() ||
(state.getUseBearerTokenForAmApis() && state.getBearerToken())
Expand Down
Loading