feat: validate token in sticker-catalogue and sticker-award#167
Conversation
scottgerring
force-pushed
the
feat/auth-everything
branch
2 times, most recently
from
48888c1 to
1597889
Compare
scottgerring
force-pushed
the
feat/auth-everything
branch
2 times, most recently
from
0f74093 to
74f42be
Compare
| @@ -0,0 +1,77 @@ | |||
| import { test, expect } from '@playwright/test'; | |||
|
|
|||
| test.describe('API Authentication Requirements', () => { | |||
There was a problem hiding this comment.
Added tests to validate
| // This makes tokens transparent (signed JWTs) instead of encrypted JWEs. | ||
| // Required for services like sticker-award and sticker-catalogue that validate | ||
| // tokens using the JWKS endpoint. | ||
| options.DisableAccessTokenEncryption(); |
There was a problem hiding this comment.
@jeastham1993 we talked about doing this anyway right?
There was a problem hiding this comment.
@scottgerring no, I think we need it still? Otherwise downstream services can't decode the JWT to do RBAC. Longer term I think we look to remove (backlog issue) but for the purposes of stability I think we leave it disabled for now. We use https in production which provides a layer of encryption right?
scottgerring
force-pushed
the
feat/auth-everything
branch
2 times, most recently
from
5e9f62b to
9a9ac87
Compare
scottgerring
force-pushed
the
feat/auth-everything
branch
from
9a9ac87 to
38b47c6
Compare
| # OAUTH_ISSUER: The issuer claim expected in JWT tokens (matches what user-management sets) | ||
| # Note: OpenIddict adds trailing slash to issuer, so we must include it here | ||
| # OAUTH_JWKS_URL: Internal Docker network URL to fetch JWKS for token validation | ||
| OAUTH_ISSUER: ${DEPLOYMENT_HOST_URL}/ |
There was a problem hiding this comment.
Do you think we look into OpenIddict config to try and fix it? Or just live with it?
There was a problem hiding this comment.
i don't think it's really broken, i think we just need to be actively consistent. This seems to be the easier way around - i stuffed about trying to get dotnet to not append /, but ultimately I don't care which way it goes, as long as they are all the same!
| // This makes tokens transparent (signed JWTs) instead of encrypted JWEs. | ||
| // Required for services like sticker-award and sticker-catalogue that validate | ||
| // tokens using the JWKS endpoint. | ||
| options.DisableAccessTokenEncryption(); |
There was a problem hiding this comment.
@scottgerring no, I think we need it still? Otherwise downstream services can't decode the JWT to do RBAC. Longer term I think we look to remove (backlog issue) but for the purposes of stability I think we leave it disabled for now. We use https in production which provides a layer of encryption right?
|
|
||
| [tasks."aws:deploy:local"] | ||
| description = "Deploy to AWS (build container locally)" | ||
| depends = ["//:env:setup", "//shared:aws:deploy:local", "//user-management:aws:deploy:local"] |
There was a problem hiding this comment.
Same as above, why is user-mgmt removed?
There was a problem hiding this comment.
Because it makes the dev experience miserable basically:
if you say mise aws:deploy:local from the top level project, it deploys each service, and each service triggers a redeploy of user-management. because in this case it is packaging and pushing a local container, it triggers a task rollout N times on user-service, which is rubbish.
I think it is better to make the deploy quick and rational and have any users who are being clever enough to go into a sub-service and explicitly deploy it know they need to have deployed user-service
2 participants
Add token validation to sticker-catalogue and sticker-award. Does not validate permissions - e.g. we don't check that only a user can manipulate their own resources. For now. I want to get the broad strokes in and stabilised first.