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

refactor: fix third-party app experience branding #6223

Merged
merged 1 commit into from
Jul 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 4 additions & 1 deletion packages/console/src/components/ImageInputs/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,10 @@ export type ImageField<FormContext extends FieldValues> = {
type Props<FormContext extends FieldValues> = {
/** The condensed title when user assets service is available. */
readonly uploadTitle: React.ComponentProps<typeof FormField>['title'];
/** The tooltip to show for all the fields. */
/**
* When user assets service is available, the tip will be displayed for the `uploadTitle`;
* otherwise, it will be displayed for each text input.
*/
readonly tip?: React.ComponentProps<typeof FormField>['tip'];
readonly control: Control<FormContext>;
readonly register: UseFormRegister<FormContext>;
Expand Down
12 changes: 10 additions & 2 deletions packages/core/src/libraries/sign-in-experience/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@
return;
}

return pick(found, 'branding', 'color');
return pick(found, 'branding', 'color', 'type', 'isThirdParty');

Check warning on line 150 in packages/core/src/libraries/sign-in-experience/index.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/libraries/sign-in-experience/index.ts#L150

Added line #L150 was not covered by tests
};

const getFullSignInExperience = async ({
Expand Down Expand Up @@ -223,9 +223,17 @@
};
};

/** Get the branding and color from the app sign-in experience if it is not a third-party app. */
const getAppSignInExperience = () => {
if (!appSignInExperience || appSignInExperience.isThirdParty) {
return {};
}
return pick(appSignInExperience, 'branding', 'color');
};

return {
...deepmerge(
deepmerge(signInExperience, appSignInExperience ?? {}),
deepmerge(signInExperience, getAppSignInExperience()),
organizationOverride ?? {}
),
socialConnectors,
Expand Down
26 changes: 21 additions & 5 deletions packages/core/src/queries/application-sign-in-experience.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import { ApplicationSignInExperiences, type ApplicationSignInExperience } from '@logto/schemas';
import {
type Application,
ApplicationSignInExperiences,
Applications,
type ApplicationSignInExperience,
} from '@logto/schemas';
import { type Nullable } from '@silverhand/essentials';
import { sql, type CommonQueryMethods } from '@silverhand/slonik';

import { buildInsertIntoWithPool } from '#src/database/insert-into.js';
Expand All @@ -10,12 +16,22 @@
returning: true,
});

const safeFindSignInExperienceByApplicationId = async (applicationId: string) => {
const { table, fields } = convertToIdentifiers(ApplicationSignInExperiences);
type ApplicationSignInExperienceReturn = ApplicationSignInExperience &
Pick<Application, 'type' | 'isThirdParty'>;

return pool.maybeOne<ApplicationSignInExperience>(sql`
select ${sql.join(Object.values(fields), sql`, `)}
const safeFindSignInExperienceByApplicationId = async (
applicationId: string
): Promise<Nullable<ApplicationSignInExperienceReturn>> => {
const applications = convertToIdentifiers(Applications, true);
const { table, fields } = convertToIdentifiers(ApplicationSignInExperiences, true);

return pool.maybeOne<ApplicationSignInExperienceReturn>(sql`
select
${sql.join(Object.values(fields), sql`, `)},
${applications.fields.type},
${applications.fields.isThirdParty}

Check warning on line 32 in packages/core/src/queries/application-sign-in-experience.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/queries/application-sign-in-experience.ts#L23-L32

Added lines #L23 - L32 were not covered by tests
from ${table}
join ${applications.table} on ${fields.applicationId}=${applications.fields.id}

Check warning on line 34 in packages/core/src/queries/application-sign-in-experience.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/queries/application-sign-in-experience.ts#L34

Added line #L34 was not covered by tests
where ${fields.applicationId}=${applicationId}
`);
};
Expand Down
46 changes: 42 additions & 4 deletions packages/integration-tests/src/tests/experience/overrides.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,20 @@
*/

import { ConnectorType } from '@logto/connector-kit';
import { ApplicationType, type Branding, type Color, SignInIdentifier } from '@logto/schemas';
import { pick } from '@silverhand/essentials';

import {
ApplicationType,
type Branding,
type Color,
SignInIdentifier,
type FullSignInExperience,
} from '@logto/schemas';
import { appendPath, pick } from '@silverhand/essentials';

import api from '#src/api/api.js';
import { setApplicationSignInExperience } from '#src/api/application-sign-in-experience.js';
import { createApplication, deleteApplication } from '#src/api/application.js';
import { updateSignInExperience } from '#src/api/sign-in-experience.js';
import { demoAppRedirectUri, demoAppUrl } from '#src/constants.js';
import { demoAppRedirectUri, demoAppUrl, logtoUrl } from '#src/constants.js';
import { clearConnectorsByTypes } from '#src/helpers/connector.js';
import { OrganizationApiTest } from '#src/helpers/organization.js';
import ExpectExperience from '#src/ui-helpers/expect-experience.js';
Expand Down Expand Up @@ -194,9 +201,39 @@ describe('overrides', () => {

await expectMatchBranding('light', organizationBranding.logoUrl, 'rgb(0, 0, 255)');
await expectMatchBranding('dark', organizationBranding.darkLogoUrl, 'rgb(255, 0, 255)');
await deleteApplication(application.id);
await experience.page.close();
});

it('should not use app-level branding when the app is an third-party app', async () => {
const application = await createApplication(
'Sign-in experience override',
ApplicationType.Traditional,
{
isThirdParty: true,
oidcClientMetadata: {
redirectUris: [demoAppRedirectUri],
postLogoutRedirectUris: [demoAppRedirectUri],
},
}
);

await setApplicationSignInExperience(application.id, {
color: appColor,
branding: appBranding,
});

// It's hard to simulate third-party apps because their type is "Traditional" while our demo
// app is an SPA. Only test the API response here.
const experience = await api
.get(appendPath(new URL(logtoUrl), 'api/.well-known/sign-in-exp'))
.json<FullSignInExperience>();

expect(experience.branding).toEqual(omniBranding);

await deleteApplication(application.id);
});

describe('override fallback', () => {
beforeAll(async () => {
await updateSignInExperience({
Expand Down Expand Up @@ -261,6 +298,7 @@ describe('overrides', () => {
expect(faviconElement).toBe(appBranding.favicon);
expect(appleFavicon).toBe(appBranding.favicon);

await deleteApplication(application.id);
await experience.page.close();
});
});
Loading