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: Refresh the SAML request URL for each login attempt #10593

Merged
merged 2 commits into from
Dec 13, 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
18 changes: 14 additions & 4 deletions packages/client/Atmosphere.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
ConcreteRequest,
Environment,
FetchFunction,
FetchQueryFetchPolicy,
GraphQLResponse,
GraphQLTaggedNode,
Network,
Expand Down Expand Up @@ -336,13 +337,22 @@ export default class Atmosphere extends Environment {

fetchQuery = async <T extends OperationType>(
taggedNode: GraphQLTaggedNode,
variables: Variables = {}
variables: Variables = {},
cacheConfig?: {
networkCacheConfig?: CacheConfig
fetchPolicy?: FetchQueryFetchPolicy
}
) => {
let res: T['response']
try {
res = await fetchQuery<T>(this, taggedNode, variables, {
fetchPolicy: 'store-or-network'
}).toPromise()
res = await fetchQuery<T>(
this,
taggedNode,
variables,
cacheConfig ?? {
fetchPolicy: 'store-or-network'
}
).toPromise()
} catch (e) {
return null
}
Expand Down
14 changes: 7 additions & 7 deletions packages/client/components/EmailPasswordAuthForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,7 @@ const EmailPasswordAuthForm = forwardRef((props: Props, ref: any) => {
const signInWithSSOOnly = isSSOAuthEnabled && !isInternalAuthEnabled
const [isSSO, setIsSSO] = useState(isSSODefault || signInWithSSOOnly)
const [pendingDomain, setPendingDomain] = useState('')
const [ssoURL, setSSOURL] = useState('')
const [ssoDomain, setSSODomain] = useState('')
const [ssoDomain, setSSODomain] = useState<string>()
const {submitMutation, onCompleted, submitting, error, onError} = useMutationProps()
const atmosphere = useAtmosphere()
const {history} = useRouter()
Expand Down Expand Up @@ -131,9 +130,10 @@ const EmailPasswordAuthForm = forwardRef((props: Props, ref: any) => {
const domain = getSSODomainFromEmail(email)
if (domain && domain !== pendingDomain) {
setPendingDomain(domain)
// Fetch the url to verify SSO is configured for this domain.
// Don't cache it as we need a fresh one for login
Copy link
Member

Choose a reason for hiding this comment

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

Given that the URL is generated on the fly by the server now, I'm curious, since this URL is generated but never used, why can't we use it for login? when does it go invalid?

Copy link
Contributor Author

@Dschoordsch Dschoordsch Dec 12, 2024

Choose a reason for hiding this comment

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

It really depends on the IDP. For CloudFlare it's about a minute. So if they idle a bit between entering their email and pushing the button, it's outdated. We really just fetch it here to check whether or not SAML is enabled for that domain.

Copy link
Member

Choose a reason for hiding this comment

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

a minute, whoa!
we don't send off that request until the email field is blurred, so we can probably get away with caching it, but if the delay isn't too bad it shouldn't matter either way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is we cannot really check if it will be rejected because it happens after the redirect, so I would like to stay on the safe side.

const url = await getSSOUrl(atmosphere, email)
setSSODomain(domain)
setSSOURL(url || '')
setSSODomain(url ? domain : undefined)
}
}
}
Expand All @@ -148,8 +148,8 @@ const EmailPasswordAuthForm = forwardRef((props: Props, ref: any) => {

const tryLoginWithSSO = async (email: string) => {
const domain = getSSODomainFromEmail(email)!
const validSSOURL = domain === ssoDomain && ssoURL
const isProbablySSO = isSSO || !fields.password.value || validSSOURL
const hadValidSSOURL = domain === ssoDomain
const isProbablySSO = isSSO || !fields.password.value || hadValidSSOURL
const top = getOffsetTop?.() || 56
let optimisticPopup
if (isProbablySSO) {
Expand All @@ -168,7 +168,7 @@ const EmailPasswordAuthForm = forwardRef((props: Props, ref: any) => {
getOAuthPopupFeatures({width: 385, height: 576, top})
)
}
const url = validSSOURL || (await getSSOUrl(atmosphere, email))
const url = await getSSOUrl(atmosphere, email)
if (!url) {
optimisticPopup?.close()
return false
Expand Down
4 changes: 3 additions & 1 deletion packages/client/utils/getSAMLIdP.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ const query = graphql`
`

const getSAMLIdP = async (atmosphere: Atmosphere, variables: getSAMLIdPQuery['variables']) => {
const res = await atmosphere.fetchQuery<getSAMLIdPQuery>(query, variables)
const res = await atmosphere.fetchQuery<getSAMLIdPQuery>(query, variables, {
fetchPolicy: 'network-only'
})
return res?.SAMLIdP ?? null
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ const resolveDowngradeToStarter = async (
.execute(),
pg
.updateTable('SAML')
.set({metadata: null, url: null, lastUpdatedBy: user.id})
.set({metadata: null, metadataURL: null, lastUpdatedBy: user.id})
.where('orgId', '=', orgId)
.execute(),
updateTeamByOrgId(
Expand Down
3 changes: 2 additions & 1 deletion packages/server/graphql/private/mutations/loginSAML.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,13 +115,14 @@ const loginSAML: MutationResolvers['loginSAML'] = async (
if (newMetadata) {
// The user is updating their SAML metadata
// Revalidate it & persist to DB
// Generate the URL to verify the metadata, don't persist it as it needs to be generated fresh
const url = getSignOnURL(metadata, normalizedName)
if (url instanceof Error) {
return standardError(url)
}
await pg
.updateTable('SAML')
.set({metadata: newMetadata, metadataURL: newMetadataURL, url})
.set({metadata: newMetadata, metadataURL: newMetadataURL})
.where('id', '=', normalizedName)
.execute()
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import type {Kysely} from 'kysely'

export async function up(db: Kysely<any>): Promise<void> {
await db.schema.alterTable('SAML').dropColumn('url').execute()
}

export async function down(db: Kysely<any>): Promise<void> {
await db.schema.alterTable('SAML').addColumn('url', 'varchar(2056)').execute()
}
20 changes: 13 additions & 7 deletions packages/server/utils/getSAMLURLFromEmail.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import base64url from 'base64url'
import getSSODomainFromEmail from 'parabol-client/utils/getSSODomainFromEmail'
import {URL} from 'url'
import {DataLoaderWorker} from '../graphql/graphql'
import getSignOnURL from '../graphql/public/mutations/helpers/SAMLHelpers/getSignOnURL'
import getKysely from '../postgres/getKysely'

export const isSingleTenantSSO =
Expand All @@ -26,14 +27,17 @@ const getSAMLURLFromEmail = async (
if (isSingleTenantSSO) {
// For PPMI use
const pg = getKysely()
const instanceURLres = await pg
const instanceRes = await pg
.selectFrom('SAML')
.select('url')
.where('url', 'is not', null)
.select(['id', 'metadata'])
.where('metadata', 'is not', null)
.limit(1)
.executeTakeFirst()
const instanceURL = instanceURLres?.url
if (!instanceURL) return null
if (!instanceRes) return null
const {id, metadata} = instanceRes
if (!metadata) return null
const instanceURL = getSignOnURL(metadata, id)
if (instanceURL instanceof Error) return null
return urlWithRelayState(instanceURL, isInvited)
}
if (!email) return null
Expand All @@ -42,8 +46,10 @@ const getSAMLURLFromEmail = async (

const saml = await dataLoader.get('samlByDomain').load(domainName)
if (!saml) return null
const {url} = saml
if (!url) return null
const {id, metadata} = saml
if (!metadata) return null
const url = getSignOnURL(metadata, id)
if (url instanceof Error) return null
return urlWithRelayState(url, isInvited)
}

Expand Down
Loading