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.

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

Redirect from auth to a blank page #34

Merged
merged 3 commits into from
Oct 5, 2023
Merged

Redirect from auth to a blank page #34

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
822dcd5
Create a new `/blank` page which does what it says on the tin...nothing
bcspragu Oct 4, 2023
dce0bc2
Lint and stuff
bcspragu Oct 5, 2023
bb28933
Better lint
bcspragu Oct 5, 2023
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
2 changes: 1 addition & 1 deletion frontend/composables/useMSAL.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ export const useMSAL = async (): Promise<MSAL> => {
const signOut = async (): Promise<void> => {
const logoutRequest = {
postLogoutRedirectUri: msalConfig.auth.redirectUri,
mainWindowRedirectUri: msalConfig.auth.redirectUri
mainWindowRedirectUri: msalConfig.auth.logoutUri
}
const userClient = userClientWithCustomToken('') // Logging out doesn't require auth.
try {
Expand Down
4 changes: 3 additions & 1 deletion frontend/envs/env.dev
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,6 @@ MSAL_USER_FLOW_NAME=B2C_1_susi_dev
MSAL_USER_FLOW_AUTHORITY=https://rmiauthdev.b2clogin.com/rmiauthdev.onmicrosoft.com/B2C_1_susi_dev
MSAL_AUTHORITY_DOMAIN=rmiauthdev.b2clogin.com
MSAL_CLIENT_ID=218f47ee-11b1-459a-b914-b7fa6f107e7b
MSAL_REDIRECT_URI=https://pacta.dev.rmi.siliconally.dev
# See https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/dev/lib/msal-browser/docs/initialization.md#redirecturi-considerations
MSAL_REDIRECT_URI=https://pacta.dev.rmi.siliconally.dev/blank
Copy link
Contributor

Choose a reason for hiding this comment

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

meganit - can we name this something clearer? blank has no context, and is more likely to be thought of as new tab etc. Very odd ms requires this - let's make the URL clear the purpose, since nothing else on the page will lol. azure-ad-redirect-destination ex

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a very reasonable and sensible comment, but I'm going to stick with /blank for a few reasons:

  1. Pure laziness - I already submitted the TF PR and don't want to deploy both frontends + TF again
  2. /blank is their recommendation, and I've seen similar things elsewhere. Not sure how strong of a convention it is, but getting a blank page at /blank seems reasonable.
  3. Only frontend developers associate new tab and blank (i.e. target=_blank)
  4. Nobody will ever actually see this, the only time it shows up is for 0.1seconds after signing in, in the Azure popup, just before it closes. The main site window will never show /blank

MSAL_LOGOUT_URI=https://pacta.dev.rmi.siliconally.dev/
4 changes: 3 additions & 1 deletion frontend/envs/env.local
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,6 @@ MSAL_USER_FLOW_NAME=B2C_1_susi_local
MSAL_USER_FLOW_AUTHORITY=https://rmiauthlocal.b2clogin.com/rmiauthlocal.onmicrosoft.com/B2C_1_susi_local
MSAL_AUTHORITY_DOMAIN=rmiauthlocal.b2clogin.com
MSAL_CLIENT_ID=2d77a4a9-b7be-4451-ad47-c151d8b6c05f
MSAL_REDIRECT_URI=http://localhost:3000
# See https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/dev/lib/msal-browser/docs/initialization.md#redirecturi-considerations
MSAL_REDIRECT_URI=http://localhost:3000/blank
MSAL_LOGOUT_URI=http://localhost:3000/
3 changes: 3 additions & 0 deletions frontend/layouts/empty.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<template>
<NuxtPage />
</template>

Check failure on line 3 in frontend/layouts/empty.vue

View workflow job for this annotation

GitHub Actions / frontend 

Newline required at end of file but not found
3 changes: 2 additions & 1 deletion frontend/nuxt.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
userFlowAuthority: process.env.MSAL_USER_FLOW_AUTHORITY ?? '',
authorityDomain: process.env.MSAL_AUTHORITY_DOMAIN ?? '',
clientID: process.env.MSAL_CLIENT_ID ?? '',
redirectURI: process.env.MSAL_REDIRECT_URI ?? ''
redirectURI: process.env.MSAL_REDIRECT_URI ?? '',
logoutURI: process.env.MSAL_LOGOUT_URI ?? '',

Check failure on line 27 in frontend/nuxt.config.ts

View workflow job for this annotation

GitHub Actions / frontend 

Unexpected trailing comma
}
}
},
Expand Down
11 changes: 11 additions & 0 deletions frontend/pages/blank.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<script setup lang="ts">
// This page is used as a redirect URI for our popup login flow, see
// https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/dev/lib/msal-browser/docs/initialization.md#redirecturi-considerations

definePageMeta({
layout: 'empty'
Copy link
Contributor

Choose a reason for hiding this comment

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

another (maybe simpler) approach? Through an index.html in some folder in the public directory. That way nuxt could never mess this up?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did consider it, and that would also be a totally reasonable approach. I'm going to stick to this for a few reasons:

  • The redirectUri actually is important in the redirect flow (we currently use the popup flow). We've seen on other projects that there are scenarios (PWAs, some mobile browsers) where popup mode doesn't play nice and you want to fallback to redirect mode, so we're going to want to be able to add little niceties to this page over time, like taking query parameters and doing stuff with them.
  • I think there's utility in having the same <head> matter that the rest of the site has

I agree there's a minor risk that something like middleware causes problems with this page, but since the page is only ever shown for a fraction of a second, I think the risk is minimal.

})
</script>

<template>

Check failure on line 10 in frontend/pages/blank.vue

View workflow job for this annotation

GitHub Actions / frontend 

The template requires child element
</template>
6 changes: 4 additions & 2 deletions frontend/plugins/msal.client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
userFlowAuthority,
authorityDomain,
clientID,
redirectURI
redirectURI,
logoutURI,

Check failure on line 14 in frontend/plugins/msal.client.ts

View workflow job for this annotation

GitHub Actions / frontend 

Unexpected trailing comma
}
}
} = useRuntimeConfig()
Expand All @@ -31,7 +32,8 @@
clientId: clientID,
authority: b2cPolicies.authorities.signUpSignIn.authority, // Choose sign-up/sign-in user-flow as your default.
knownAuthorities: [b2cPolicies.authorityDomain], // You must identify your tenant's domain as a known authority.
redirectUri: redirectURI // Must be registered as a SPA redirectURI on your app registration
redirectUri: redirectURI, // Must be registered as a SPA redirectURI on your app registration
logoutUri: logoutURI,

Check failure on line 36 in frontend/plugins/msal.client.ts

View workflow job for this annotation

GitHub Actions / frontend 

Unexpected trailing comma
},
cache: {
cacheLocation: 'localStorage',
Expand Down