Skip to content
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.

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

[STCOR-787] Always retrieve clientId and tenant values from config.tenantOptions in stripes.config.js #1487

Merged
merged 12 commits into from
Jun 10, 2024
Merged
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
* Idle-session timeout and "Keep working?" modal. Refs STCOR-776.
* Use keycloak URLs in place of users-bl for tenant-switch. Refs US1153537.
* Fix 404 error page when logging in after changing password in Eureka. Refs STCOR-845.
* Always retrieve `clientId` and `tenant` values from `config.tenantOptions` in stripes.config.js. Retires `okapi.tenant`, `okapi.clientId`, and `config.isSingleTenant`. Refs STCOR-787.

## [10.1.1](https://github.com/folio-org/stripes-core/tree/v10.1.1) (2024-03-25)
[Full Changelog](https://github.com/folio-org/stripes-core/compare/v10.1.0...v10.1.1)
Expand Down
17 changes: 14 additions & 3 deletions src/RootWithIntl.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ jest.mock('./components/PreLoginLanding', () => () => '<preloginlanding>');
describe('RootWithIntl', () => {
describe('AuthnLogin', () => {
it('handles legacy login', () => {
const stripes = { okapi: {}, config: {} };
const stripes = { okapi: {}, config: { tenantOptions: {} } };
ryandberger marked this conversation as resolved.
Show resolved Hide resolved
render(<AuthnLogin stripes={stripes} />);

expect(screen.getByText(/<login>/)).toBeInTheDocument();
Expand All @@ -35,7 +35,12 @@ describe('RootWithIntl', () => {
it('handles single-tenant', () => {
const stripes = {
okapi: { authnUrl: 'https://barbie.com' },
config: { isSingleTenant: true }
config: {
isSingleTenant: true,
tenantOptions: {
diku: { name: 'diku', clientId: 'diku-application' }
}
},
};
render(<AuthnLogin stripes={stripes} />);

Expand All @@ -45,7 +50,13 @@ describe('RootWithIntl', () => {
it('handles multi-tenant', () => {
const stripes = {
okapi: { authnUrl: 'https://oppie.com' },
config: { },
config: {
isSingleTenant: false,
tenantOptions: {
diku: { name: 'diku', clientId: 'diku-application' },
diku2: { name: 'diku2', clientId: 'diku2-application' }
}
},
};
render(<AuthnLogin stripes={stripes} />);

Expand Down
1 change: 1 addition & 0 deletions src/Stripes.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export const stripesShape = PropTypes.shape({
logTimestamp: PropTypes.bool,
showHomeLink: PropTypes.bool,
showPerms: PropTypes.bool,
tenantOptions: PropTypes.object,
}).isRequired,
connect: PropTypes.func.isRequired,
currency: PropTypes.string,
Expand Down
8 changes: 6 additions & 2 deletions src/components/AuthnLogin/AuthnLogin.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import { setUnauthorizedPathToSession } from '../../loginServices';

const AuthnLogin = ({ stripes }) => {
const { config, okapi } = stripes;
const { tenantOptions = {} } = config;
const tenants = Object.values(tenantOptions);

useEffect(() => {
if (okapi.authnUrl) {
Expand All @@ -22,9 +24,11 @@ const AuthnLogin = ({ stripes }) => {
}, []); // eslint-disable-line react-hooks/exhaustive-deps

if (okapi.authnUrl) {
if (config.isSingleTenant) {
// If only 1 tenant is defined in config, skip the tenant selection screen.
if (tenants.length === 1) {
const loginTenant = tenants[0];
const redirectUri = `${window.location.protocol}//${window.location.host}/oidc-landing`;
const authnUri = `${okapi.authnUrl}/realms/${okapi.tenant}/protocol/openid-connect/auth?client_id=${okapi.clientId}&response_type=code&redirect_uri=${redirectUri}&scope=openid`;
const authnUri = `${okapi.authnUrl}/realms/${loginTenant.name}/protocol/openid-connect/auth?client_id=${loginTenant.clientId}&response_type=code&redirect_uri=${redirectUri}&scope=openid`;
return <Redirect to={authnUri} />;
}

Expand Down
11 changes: 7 additions & 4 deletions src/components/OIDCLanding.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ const OIDCLanding = () => {
const location = useLocation();
const store = useStore();
// const samlError = useRef();
const { okapi } = useStripes();
const { okapi, config } = useStripes();
const { tenantOptions = {} } = config;

const [potp, setPotp] = useState();
const [samlError, setSamlError] = useState();
Expand Down Expand Up @@ -57,10 +58,12 @@ const OIDCLanding = () => {
const otp = getOtp();

if (otp) {
const loginTenant = Object.values(tenantOptions)[0];

ryandberger marked this conversation as resolved.
Show resolved Hide resolved
setPotp(otp);
fetch(`${okapi.url}/authn/token?code=${otp}&redirect-uri=${window.location.protocol}//${window.location.host}/oidc-landing`, {
credentials: 'include',
headers: { 'X-Okapi-tenant': okapi.tenant, 'Content-Type': 'application/json' },
headers: { 'X-Okapi-tenant': loginTenant.name, 'Content-Type': 'application/json' },
mode: 'cors',
})
.then((resp) => {
Expand All @@ -73,7 +76,7 @@ const OIDCLanding = () => {
});
})
.then(() => {
return requestUserWithPerms(okapi.url, store, okapi.tenant);
return requestUserWithPerms(okapi.url, store, loginTenant.name);
});
} else {
return resp.json().then((error) => {
Expand All @@ -90,7 +93,7 @@ const OIDCLanding = () => {
// we only want to run this effect once, on load.
// keycloak authentication will redirect here and the other deps will be constant:
// location.search: the query string; this will never change
// okapi.tenant, okapi.url: these are defined in stripes.config.js
// config.tenantOptions, okapi.url: these are defined in stripes.config.js
// store: the redux store
}, []); // eslint-disable-line react-hooks/exhaustive-deps

Expand Down
85 changes: 85 additions & 0 deletions src/components/OIDCLanding.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
import { render, screen, waitFor } from '@folio/jest-config-stripes/testing-library/react';

import OIDCLanding from './OIDCLanding';

jest.mock('react-router-dom', () => ({
useLocation: () => ({
search: 'session_state=dead-beef&code=c0ffee'
}),
Redirect: () => <>Redirect</>,
}));

jest.mock('react-redux', () => ({
useStore: () => { },
}));

jest.mock('../StripesContext', () => ({
useStripes: () => ({
okapi: { url: 'https://whaterver' },
config: { tenantOptions: { diku: { name: 'diku', clientId: 'diku-application' } } },
}),
}));

// jest.mock('../loginServices');


const mockSetTokenExpiry = jest.fn();
const mockRequestUserWithPerms = jest.fn();
const mockFoo = jest.fn();
jest.mock('../loginServices', () => ({
setTokenExpiry: () => mockSetTokenExpiry(),
requestUserWithPerms: () => mockRequestUserWithPerms(),
foo: () => mockFoo(),
}));


// fetch success: resolve promise with ok == true and $data in json()
const mockFetchSuccess = (data) => {
global.fetch = jest.fn().mockImplementation(() => (
Promise.resolve({
ok: true,
json: () => Promise.resolve(data),
headers: new Map(),
})
));
};

// fetch failure: resolve promise with ok == false and $error in json()
const mockFetchError = (error) => {
global.fetch = jest.fn().mockImplementation(() => (
Promise.resolve({
ok: false,
json: () => Promise.resolve(error),
headers: new Map(),
})
));
};

// restore default fetch impl
const mockFetchCleanUp = () => {
global.fetch.mockClear();
delete global.fetch;
};

describe('OIDCLanding', () => {
it('calls requestUserWithPerms, setTokenExpiry on success', async () => {
mockFetchSuccess({
accessTokenExpiration: '2024-05-23T09:47:17.000-04:00',
refreshTokenExpiration: '2024-05-23T10:07:17.000-04:00',
});

await render(<OIDCLanding />);
screen.getByText('Loading');
await waitFor(() => expect(mockSetTokenExpiry).toHaveBeenCalledTimes(1));
await waitFor(() => expect(mockRequestUserWithPerms).toHaveBeenCalledTimes(1));
mockFetchCleanUp();
});

it('displays an error on failure', async () => {
mockFetchError('barf');

await render(<OIDCLanding />);
await screen.findByText('errors.saml.missingToken');
mockFetchCleanUp();
});
});
4 changes: 2 additions & 2 deletions src/components/OIDCRedirect.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@ describe('OIDCRedirect', () => {
afterAll(() => sessionStorage.removeItem('unauthorized_path'));

it('redirects to value from session storage under unauthorized_path key', () => {
useStripes.mockReturnValue({ okapi:{ authnUrl: 'http://example.com/authn' } });
useStripes.mockReturnValue({ okapi: { authnUrl: 'http://example.com/authn' }, config: {} });
render(<OIDCRedirect />);

expect(screen.getByText(/internalredirect/)).toBeInTheDocument();
});

it('redirects fwd if no authn provided to stripes okapi config', () => {
useStripes.mockReturnValue({ okapi: { } });
useStripes.mockReturnValue({ okapi: { }, config: {} });
render(<OIDCRedirect />);

expect(screen.getByText(/internalredirect/)).toBeInTheDocument();
Expand Down
2 changes: 1 addition & 1 deletion src/loginServices.js
Original file line number Diff line number Diff line change
Expand Up @@ -778,7 +778,7 @@ export function validateUser(okapiUrl, store, tenant, session) {
token,
tokenExpiration,
}));
return loadResources(store, sessionTenant, user.id);
return loadResources(store, sessionTenant, user?.id ?? data.user?.id);
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I've seen this too but not been able to figure out how things get into this state. It is mitigated in the community builds in #1466.

});
} else {
store.dispatch(clearCurrentUser());
Expand Down
Loading