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

fix(passport): displaying app's favicon in custom domain #2418

Merged
merged 12 commits into from
Jun 17, 2023

Conversation

poolsar42
Copy link
Collaborator

@poolsar42 poolsar42 commented Jun 15, 2023

Description

App's favicon on now displayed on the MetaMask Notification window

Loader in passport now reflects colours that were set in designer for specific app

Fixes bugs with redirects in passport which were not indicated in any other issues, stable temporary solution for /sign route when there's an error occurs. Solves issue with /authenticate route to not redirect to /settings/dashboard logged-in users.

Fetches appProps from starbase before loading context and puts it in the context object.

Related Issues

Testing

Go to authn screen and pop-up metamask window to sign a message. Under custom domain or just with clientId for the specific app.

Checklist

  • I have read the CONTRIBUTING guidelines
  • I have tested my code (manually and/or automated if applicable)
  • I have updated the documentation (if necessary)

@poolsar42 poolsar42 self-assigned this Jun 15, 2023
@@ -90,6 +93,25 @@ export const loader: LoaderFunction = getRollupReqFunctionErrorWrapper(
return redirect(`/authenticate`)
}

let appProps: GetAppPublicPropsResult
Copy link
Contributor

Choose a reason for hiding this comment

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

is this not already in context?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@maurerbot nope

{
  context: {
    authzQueryParams: {
      clientId: '',
      state: '',
      redirectUri: '',
      scope: [],
      prompt: undefined,
      login_hint: undefined,
      rollup_action: undefined,
      rollup_result: undefined
    },
    env: ServiceWorkerGlobalScope {},
    traceSpan: TraceSpan {
      traceId: '70c7e789dfcd23ea82eecac91c7aaf53',
      spanId: 'a0c0d5b6de5115dd',
      startTime: 1686932216223
    }
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

we must be checking for custom domain somewhere and pulling in appdata?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All things wee need to fetch appData is clientId. In custom domains it is being provided if I recall correctly and we tested its validity for custom domain with @szkl recently

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's the check for clientId

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Haha its done actually take a look here

@poolsar42 poolsar42 force-pushed the fix/2406-favicon-in-mematask-for-cutom-domain branch from e880ca6 to 5193eed Compare June 16, 2023 16:31
@poolsar42 poolsar42 requested a review from maurerbot June 16, 2023 22:05
@poolsar42 poolsar42 force-pushed the fix/2406-favicon-in-mematask-for-cutom-domain branch from 5193eed to 0cd280a Compare June 16, 2023 22:37
@szkl
Copy link
Contributor

szkl commented Jun 16, 2023

The commit message should be about the changes in the pull request.

@poolsar42 poolsar42 changed the title fix(passport): Rollup's favicon still displayed on the MetaMask Notification window post enabling custom domain and white labelling fix(passport): displaying app's favicon in metamask and fetching appTheme for everything with clientId including custom domains Jun 16, 2023
@poolsar42 poolsar42 force-pushed the fix/2406-favicon-in-mematask-for-cutom-domain branch from 2455979 to 9f30f17 Compare June 16, 2023 23:38
@@ -78,6 +81,32 @@ export const links: LinksFunction = () => [

export const loader: LoaderFunction = getRollupReqFunctionErrorWrapper(
async ({ request, context, params }) => {
let appProps: GetAppPublicPropsResult

if (context?.authzQueryParams?.app_props) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we assume it will always be there? if it's not it is probably console or passport?

@@ -90,6 +95,7 @@ const handleEvent = async (event: FetchEvent) => {

try {
const app = await starbaseClient.getAppPublicProps.query({ clientId })
Copy link
Contributor

Choose a reason for hiding this comment

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

why not skip if passport or console?

Copy link
Contributor

Choose a reason for hiding this comment

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

Line 84 checks for a custom domain request. If it is a custom domain request then there's a clientId.

@poolsar42 poolsar42 changed the title fix(passport): displaying app's favicon in metamask and fetching appTheme for everything with clientId including custom domains fix(passport): displaying app's favicon in custom domain Jun 17, 2023
@szkl szkl requested a review from maurerbot June 17, 2023 19:31
@maurerbot maurerbot merged commit 6cbe0d1 into main Jun 17, 2023
13 of 14 checks passed
@maurerbot maurerbot deleted the fix/2406-favicon-in-mematask-for-cutom-domain branch June 17, 2023 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants