-
Notifications
You must be signed in to change notification settings - Fork 87
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: platform api v1 - get and update webhook settings #6608
Conversation
src/app/routes/api/platform/v1/admin/forms/admin-forms.platform.routes.ts
Outdated
Show resolved
Hide resolved
src/app/routes/api/platform/v1/admin/forms/admin-forms.platform.routes.ts
Outdated
Show resolved
Hide resolved
shared/types/user.ts
Outdated
@@ -29,6 +29,7 @@ export const UserBase = z.object({ | |||
keyHash: z.string(), | |||
createdAt: z.date(), | |||
lastUsedAt: z.date().optional(), | |||
isPlatform: z.string().optional(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a thought: if platform users are also a kind of formsg user, will this allow platforms to modify each other's user settings in future (e.g. contact number)? I was wondering if platform tokens could be issued without creating a user account for the platform
not a super major point, just food for thought
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if platform users are also a kind of formsg user, will this allow platforms to modify each other's user settings in future (e.g. contact number)?
Yes can, if we create an endpoint which allows them to modify other users' contact number settings. We currently only have get and update webhook settings (from this PR), but we might add more endpoints in the future depending on the need. This is potentially giving them a lot of power though, so might have to think about permissions as well. I'm intentionally ignoring that right now as this is just an MVP, but recognise that this is a possible future extension!
I was wondering if platform tokens could be issued without creating a user account for the platform
And yes valid point too, worth considering hm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @wanlingt ! PR looks good with just some minor clarifications please 😃 will re-review again thereafter
#for future PR - the platform endpoint sounds like it could potentially give platforms too much power. I wonder if we need to build in additional safeguards against key loss or MITM attacks. e.g. require platform users to sign their requests, and host their signing keys on a well known endpoint for us to verify.
thanks for the review @tshuli , made the changes. I think the additional safeguards is a great point. The first phase is making this available to OGP-only admins, I think we should definitely make this mechanism more secure before releasing this to WOG form admins (e.g. CPF). Might arrange a session with Eugene to better understand the safeguards we can have, will list your suggestion too |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @wanlingt for addressing the comments, lgtm!
Additional suggestion for future PR - perhaps you could also consider a flag to restrict platforms to managing accounts under specified domains, eg. CPFB platform account can only modify CPF forms. For OGP's platforms, they can modify any form. |
Problem
Closes FRM-1244, FRM-1242, FRM-1243
Solution
This PR implements
isPlatform
boolean property in theapiToken
object.Ideally, authorisation would be via OAuth to allow for more fine-grained permissions, but for the purposes of the MVP, we are going with this simplified approach. Inspiration was taken from GoGovSG's admin API which allows FormSG to create shortlinks on behalf of GoGovSG admins.
/api/public/v1
), the /public/v1 route uses a middleware checkisPlatformApiUser
inauth.middlewares.ts
. If the API user is a platform, and has provided a userEmail, it looks for the user associated with the userEmail and setsreq.session.user
as that userNew platform API endpoints:
POST /api/public/v1/admin/forms/:formId/webhooksettings
to retrieve webhook settings.PATCH /api/public/v1/admin/forms/:formId/webhooksettings
to update webhook settingsMore details can be found in the APIs design doc (WIP)
Breaking Changes
Future Improvements (TODOs):
Tests
POST /api/public/v1/admin/forms/:formId/webhooksettings
to retrieve webhook settingsPATCH /api/public/v1/admin/forms/:formId/webhooksettings
to update webhook settingsDeploy Notes
New environment variables:
PLATFORM_API_RATE_LIMIT
: Per-minute, per-IP, per-instance request limit for platform APIs