Add BC Services Card sign-in to appointment booking#1093
Conversation
Add BCSC Keycloak login as booking step
it kept looping after singing in
|
chrsamp
left a comment
chrsamp
left a comment
There was a problem hiding this comment.
Looks solid @veenupunyani, left some comments in the review. I've also added https://citz-sbc.atlassian.net/browse/DEV-78 to our next sprint to handle refreshing the user's token.
| "@react-router/serve": "8.0.0", | ||
| "geolib": "^3.3.14", | ||
| "isbot": "^5.1.36", | ||
| "keycloak-js": "^24.0.5", |
There was a problem hiding this comment.
Why are we using such an old version of this package?
| "VUE_APP_ROOT_API": "http://localhost:5000/api/v1" | ||
| "VUE_APP_ROOT_API": "http://localhost:5000/api/v1", | ||
| "KEYCLOAK_CONFIG_URL": "/config/kc/keycloak-public.json", | ||
| "BCServicesCardUrl": "https://www2.gov.bc.ca/gov/content?id=B2B3A21E797A421A8FD39EEA86E245D6" |
There was a problem hiding this comment.
Code style note: use SCREAMING_SNAKE_CASE like the other env vars already are.
| @@ -2,43 +2,62 @@ | |||
| // Each environment has its own ConfigMap with the same filename but different values. | |||
|
|
|||
| type RuntimeConfig = { | |||
There was a problem hiding this comment.
See my other comment about the config key names in configuration.json. If you want to be more idiomatic you can map the keys like KEYCLOAK_CONFIG_URL to KeycloakConfigUrl and manually create the RuntimeConfig instance instead of just casting the JSON response to that type, but that's more work. Either way, I would pick one style here as well, and stick with it (either PascalCase or SCREAMING_SNAKE_CASE).
| import { getBookingLocations, type Location } from '../api/locations' | ||
| import type { Service } from '../api/services' | ||
| import { useBooking } from '../booking/booking-context' | ||
| import { useBooking } from '~/booking/booking-context' | ||
| import { BookingBackRow } from '../components/BookingBackRow' | ||
| import { BookingContinueRow } from '../components/BookingContinueRow' | ||
| import { BookingStepProgress } from '../components/BookingStepProgress' |
There was a problem hiding this comment.
First thought when reading this: use the ~ alias consistently (across all your imports) or stick with relative paths, I wouldn't just replace one.
| } as const | ||
|
|
||
| // Citizen booking accepts only BCSC for now (OTP may be added later). | ||
| export const ALLOWED_BOOKING_IDPS = [IdpHint.BCSC] as const |
There was a problem hiding this comment.
This is fun, smart! 👍 We could enable and disable IdP's without removing them from the app.
| function resolveFullName(claims: TokenClaims): string { | ||
| const firstName = claims.firstname || claims.given_name || '' | ||
| const lastName = claims.lastname || claims.family_name || '' | ||
| return ( | ||
| `${firstName} ${lastName}`.trim() || | ||
| claims.display_name || | ||
| claims.name || | ||
| claims.preferred_username || | ||
| '' | ||
| ) | ||
| } |
There was a problem hiding this comment.
Please avoid creating a user's name using a first name/last name order, this is not consistent across all cultures. Prefer display_name instead or return something like email (if we know we get this from every supported IdP, I believe we do) or some generic string like "Appointment User". Please don't expose the preferred_username claim to the user themselves, it's not private but e.g. for BCSC is the users DID (like a GUID).
2 participants
Summary###
Test plan locally
Test on OpenShift
1 - Visit - https://dev-appointments.apps.silver.devops.gov.bc.ca/
2 - Sign in using test username and password given in email.