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

feat: Render okta errors based on error param #3196

Merged
merged 6 commits into from
Sep 16, 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
43 changes: 43 additions & 0 deletions src/shared/GlobalTopBanners/OktaBanners/OktaBanners.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import OktaBanners from './OktaBanners'

jest.mock('../OktaEnabledBanner', () => () => 'OktaEnabledBanner')
jest.mock('../OktaEnforcedBanner', () => () => 'OktaEnforcedBanner')
jest.mock('../OktaErrorBanners', () => () => 'OktaErrorBanners')

const wrapper =
(initialEntries = ['/gh/owner']): React.FC<React.PropsWithChildren> =>
Expand Down Expand Up @@ -145,6 +146,27 @@ describe('OktaBanners', () => {
const banner = await screen.findByText(/OktaEnabledBanner/)
expect(banner).toBeInTheDocument()
})

it('should render OktaErrorBanners', async () => {
setup({
owner: {
isUserOktaAuthenticated: false,
account: {
oktaConfig: {
enabled: true,
enforced: false,
url: 'https://okta.com',
clientId: 'clientId',
clientSecret: 'clientSecret',
},
},
},
})

render(<OktaBanners />, { wrapper: wrapper() })
const banner = await screen.findByText(/OktaErrorBanners/)
expect(banner).toBeInTheDocument()
})
})

describe('when Okta is enforced', () => {
Expand All @@ -169,6 +191,27 @@ describe('OktaBanners', () => {
const banner = await screen.findByText(/OktaEnforcedBanner/)
expect(banner).toBeInTheDocument()
})

it('should render OktaErrorBanners', async () => {
setup({
owner: {
isUserOktaAuthenticated: false,
account: {
oktaConfig: {
enabled: true,
enforced: true,
url: 'https://okta.com',
clientId: 'clientId',
clientSecret: 'clientSecret',
},
},
},
})

render(<OktaBanners />, { wrapper: wrapper() })
const banner = await screen.findByText(/OktaErrorBanners/)
expect(banner).toBeInTheDocument()
})
})
})
})
Expand Down
8 changes: 7 additions & 1 deletion src/shared/GlobalTopBanners/OktaBanners/OktaBanners.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ interface URLParams {

const OktaEnabledBanner = lazy(() => import('../OktaEnabledBanner'))
const OktaEnforcedBanner = lazy(() => import('../OktaEnforcedBanner'))
const OktaErrorBanners = lazy(() => import('../OktaErrorBanners'))

function OktaBanners() {
const { provider, owner } = useParams<URLParams>()
Expand All @@ -25,7 +26,12 @@ function OktaBanners() {
if (!owner || !oktaConfig?.enabled || data?.owner?.isUserOktaAuthenticated)
return null

return oktaConfig?.enforced ? <OktaEnforcedBanner /> : <OktaEnabledBanner />
return (
<div className="flex flex-col gap-2">
{oktaConfig?.enforced ? <OktaEnforcedBanner /> : <OktaEnabledBanner />}
<OktaErrorBanners />
</div>
)
}

export default OktaBanners
111 changes: 111 additions & 0 deletions src/shared/GlobalTopBanners/OktaErrorBanners/OktaErrorBanners.spec.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
import * as Sentry from '@sentry/react'
import { render, screen } from '@testing-library/react'
import { MemoryRouter, Route } from 'react-router-dom'

import OktaErrorBanners from './OktaErrorBanners'

jest.mock('@sentry/react', () => {
const originalModule = jest.requireActual('@sentry/react')
return {
...originalModule,
captureMessage: jest.fn(),
}
})

const wrapper =
(
initialEntries = ['/gh/codecov'],
path = '/:provider/:owner'
): React.FC<React.PropsWithChildren> =>
({ children }) => (
<MemoryRouter initialEntries={initialEntries}>
<Route path={path}>{children}</Route>
</MemoryRouter>
)

describe('OktaErrorBanners', () => {
it('should return null if owner is not provided', () => {
const { container } = render(<OktaErrorBanners />, {
wrapper: wrapper(['/gh/'], '/:provider'),
})

expect(container).toBeEmptyDOMElement()
})

it('should return null if error is not provided', () => {
const { container } = render(<OktaErrorBanners />, {
wrapper: wrapper(['/gh/codecov']),
})

expect(container).toBeEmptyDOMElement()
})

it('should render error message for invalid_request', () => {
render(<OktaErrorBanners />, {
wrapper: wrapper(['/gh/codecov?error=invalid_request']),
})

const content = screen.getByText(
/Invalid request: The request parameters aren't valid. Please try again or contact support./
)
expect(content).toBeInTheDocument()
})

it('should render error message for unauthorized_client', () => {
render(<OktaErrorBanners />, {
wrapper: wrapper(['/gh/codecov?error=unauthorized_client']),
})

const content = screen.getByText(
/Unauthorized client: The client isn't authorized to request an authorization code using this method. Please reach out to your administrator./
)
expect(content).toBeInTheDocument()
})

it('should render error message for access_denied', () => {
render(<OktaErrorBanners />, {
wrapper: wrapper(['/gh/codecov?error=access_denied']),
})

const content = screen.getByText(
/The resource owner or authorization server denied the request/
)
expect(content).toBeInTheDocument()
})

it('should render default error message for unknown error', () => {
render(<OktaErrorBanners />, {
wrapper: wrapper(['/gh/codecov?error=unknown']),
})

const content = screen.getByText(
/An unknown error occurred. Please try again or contact support./
)
expect(content).toBeInTheDocument()
})

it('should capture unknown error message', () => {
render(<OktaErrorBanners />, {
wrapper: wrapper(['/gh/codecov?error=unknown']),
})

expect(Sentry.captureMessage).toHaveBeenCalledWith(
'Unknown Okta error: unknown',
{
fingerprint: ['unknown-okta-error'],
tags: {
error: 'unknown',
},
}
)
})

it('should render dismiss button', () => {
render(<OktaErrorBanners />, {
wrapper: wrapper(['/gh/codecov?error=invalid_request']),
})

const dismissButton = screen.getByRole('button', { name: /Dismiss/ })
expect(dismissButton).toBeInTheDocument()
})
})
41 changes: 41 additions & 0 deletions src/shared/GlobalTopBanners/OktaErrorBanners/OktaErrorBanners.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { useLocation, useParams } from 'react-router-dom'

import Icon from 'ui/Icon'
import TopBanner from 'ui/TopBanner'

import { getOktaErrorMessage } from './enums'

interface URLParams {
owner?: string
}

const OktaErrorBanners = () => {
const { owner } = useParams<URLParams>()
const location = useLocation()

const searchParams = new URLSearchParams(location.search)
const error = searchParams.get('error')

if (!owner || !error) return null

const errorMessage = getOktaErrorMessage(error)

return (
<TopBanner variant="error">
<TopBanner.Start>
<p className="items-center gap-1 md:flex">
<span className="flex items-center gap-1 font-semibold">
<Icon name="exclamationCircle" />
Okta Authentication Error
</span>
{errorMessage}
</p>
</TopBanner.Start>
<TopBanner.End>
<TopBanner.DismissButton>Dismiss</TopBanner.DismissButton>
</TopBanner.End>
</TopBanner>
)
}

export default OktaErrorBanners
55 changes: 55 additions & 0 deletions src/shared/GlobalTopBanners/OktaErrorBanners/enums.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a benefit to having these split out from the component where they're being used? Exporting getErrorMessage by itself may be confusing for others as it's not clear what error messages it's getting, and may be accidentally imported elsewhere.

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 object is big and messy. I think it’s cleaner this way. We don’t have a rigid structure for enum files in the codebase, so I don’t think it’s confusing, especially since it’s a file nested within the banner folder

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, they tend to get large and a little messy, and that's alright. If the error messages only relates to Okta and this component specifically, it might make sense to just move the code definitions inside the banner so they are more tightly coupled or rename the export to getOktaErrorMessage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool I'll rename it to make it even more explicit for its use

Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import * as Sentry from '@sentry/react'

const ErrorType = {
UnauthorizedClient: 'unauthorized_client',
AccessDenied: 'access_denied',
UnsupportedResponseType: 'unsupported_response_type',
UnsupportedResponseMode: 'unsupported_response_mode',
InvalidScope: 'invalid_scope',
ServerError: 'server_error',
TemporarilyUnavailable: 'temporarily_unavailable',
InvalidClient: 'invalid_client',
LoginRequired: 'login_required',
InvalidRequest: 'invalid_request',
UserCanceledRequest: 'user_canceled_request',
} as const
Comment on lines +3 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a benefit to using this enum object here? We don't seem to be accessing the value outside of the object definition, could it just be simplified to one object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're aligning with JavaScript's property naming conventions by receiving snake_case values and converting them to the preferred camelCase format

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I might not have explained myself well, but what I wanted to say is that since we are not actually accessing the enum properties and checking for individual keys inside the getErrorMessage function or anywhere else, there is no need to align this and we can just type it as:

const OktaErrorMappings = { 'server_error': "...", } as const
function getOktaErrorMessage(status){
  return OktaHTTPErrorMappings[status] ?? 'default status'
}

This way we can drop the enum and make keep the definition in a single type as opposed to having to define enum + record to make changes in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand your point; the way you listed it is more direct. However, this goes against our ESLint rules and JavaScript's preferred key structure. It will also make more sense if we ever need to access the enum keys, instead of having to refactor it later

Screenshot 2024-09-16 at 10 21 06 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh more silly linting rules, gonna address these kinda rules a bit more during the GV2 work because they're hampering us more than helping us.


const errorMessages = {
[ErrorType.UnauthorizedClient]:
"Unauthorized client: The client isn't authorized to request an authorization code using this method. Please reach out to your administrator.",
[ErrorType.AccessDenied]:
'Access denied: The resource owner or authorization server denied the request. Please reach out to your administrator.',
[ErrorType.UnsupportedResponseType]:
"Unsupported response type: The authorization server doesn't support obtaining an authorization code using this method. Please reach out to your administrator.",
[ErrorType.UnsupportedResponseMode]:
"Unsupported response mode: The authorization server doesn't support the requested response mode. Please reach out to your administrator.",
[ErrorType.InvalidScope]:
'Invalid scope: The requested scope is invalid, unknown, or malformed. Please reach out to your administrator.',
[ErrorType.ServerError]:
'Server error: The authorization server encountered an unexpected condition that prevented it from fulfilling the request. Please try again later or contact support.',
[ErrorType.TemporarilyUnavailable]:
'Temporarily unavailable: The authorization server is currently unable to handle the request due to temporary overloading or maintenance. Please try again later.',
[ErrorType.InvalidClient]:
"Invalid client: The specified client isn't valid. Please reach out to your administrator.",
[ErrorType.LoginRequired]:
"Login required: The client specified not to prompt, but the user isn't signed in. Please sign in and try again.",
[ErrorType.InvalidRequest]:
"Invalid request: The request parameters aren't valid. Please try again or contact support.",
[ErrorType.UserCanceledRequest]:
'Request canceled: User canceled the social sign-in request. Please try again if this was unintentional.',
} as const

export const getOktaErrorMessage = (error: string): string => {
if (error in errorMessages) {
return errorMessages[error as keyof typeof errorMessages]
}

Sentry.captureMessage(`Unknown Okta error: ${error}`, {
fingerprint: ['unknown-okta-error'],
tags: {
error: error,
},
})

return 'An unknown error occurred. Please try again or contact support.'
Copy link
Contributor

Choose a reason for hiding this comment

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

We could log this to Sentry via Sentry.captureMessage and include the error type so we can keep tabs on cases we might have missed (or if they change for some reason)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point

Copy link
Contributor

Choose a reason for hiding this comment

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

Another thing you may want to add in alongside the fingerprint is a tag. I added one to the prismLanguageMapper capture message and this is what it provides in the Sentry UI:

Screenshot 2024-09-16 at 09 20 16

And the code is pretty simple to add in:

Sentry.captureMessage(`Unsupported language type for filename ${fileName}`, {
  fingerprint: ['unsupported-prism-language'],
  tags: {
    'file.extension': fileExtension,
  },
})

}
1 change: 1 addition & 0 deletions src/shared/GlobalTopBanners/OktaErrorBanners/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default } from './OktaErrorBanners'
4 changes: 2 additions & 2 deletions src/ui/TopBanner/TopBanner.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const meta: Meta<typeof TopBannerComponent> = {
},
},
variant: {
options: ['default', 'warning'],
options: ['default', 'warning', 'error'],
control: 'radio',
description:
'This prop controls the variation of top banner that is displayed',
Expand All @@ -35,7 +35,7 @@ export const TopBanner: Story = {
},
argTypes: {
variant: {
options: ['default', 'warning'],
options: ['default', 'warning', 'error'],
control: 'radio',
},
children: {
Expand Down
11 changes: 10 additions & 1 deletion src/ui/TopBanner/TopBanner.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,21 @@ const variants = {
iconColor: 'text-ds-primary-yellow',
bgColor: 'bg-orange-100',
},
error: {
icon: 'exclamationCircle',
iconColor: '',
bgColor: 'bg-ds-primary-red',
},
} as const

type Variants = keyof typeof variants

const topBannerContext = z.object({
variant: z.union([z.literal('default'), z.literal('warning')]),
variant: z.union([
z.literal('default'),
z.literal('warning'),
z.literal('error'),
]),
localStorageKey: z.string().optional(),
setHideBanner: z.function().args(z.boolean()).returns(z.void()),
})
Expand Down
Loading