Closed patcon closed 2 years ago
I'm 100% in favour of removing Laravel auth and setting up a mock auth server for running tests instead. That would be super cool if it was just a docker image and we had to host minimal code.
I'm a little worried about the scope of this issue as defined. Changing the use of x5c and the introspection endpoints may be important but shouldn't be considered here, I think. We should be selecting a mock auth server based on it's ability to mimic our real auth server flow, not changing our real auth server flow to behave like our mock. If we want to implement some best practices that would also make it easier to mock let's set it as a blocker before starting this - not mix it in.
Here's my suggested scope:
Thanks Peter!
Changing the use of x5c
Just to draw attention to it, but did you see the email comment from our SiC contact in the other issue, about x5c being just a convenience? imho we shouldn't be using it -- it doesn't offer anything (simply a convenience to not do some cryptographic operations) and just restricts our options here.
From Doug in https://github.com/GCTC-NTGC/gc-digital-talent/issues/2716
The “x5c” property contains the same public key represented by the “n” and “e” properties, just presented as an X.509 certificate for any software out there that might prefer the older format. It is completely redundant and ignored by most software I’ve worked with.
My research (with links in https://github.com/GCTC-NTGC/gc-digital-talent/issues/2716) indicate that it's rarely provided, and we are less likely to find an existing mock auth server supporting it
We should be selecting a mock auth server based on it's ability to mimic our real auth server flow, not changing our real auth server flow to behave like our mock.
Maybe let's sync up and discuss? I worry I've poorly explained myself here. More than anything else, I feel this is an example of outside software (the mock server) helping us understand a misstep. (The conditional around the introspection endpoint is a different rationale.)
If we want to implement some best practices that would also make it easier to mock let's set it as a blocker before starting this - not mix it in.
I agree, this seems like a best practice. I've set #2716 as a blocker for this issue.
introspection endpoint is landed in upstream release, so we're good to migrate to the mock server (with not loss in functionality) as soon as https://github.com/GCTC-NTGC/gc-digital-talent/issues/2716 is ready.
Please add your planning poker estimate with ZenHub @patcon
This has come up as a nice-to-have, so that:
To Do
https://github.com/GCTC-NTGC/gc-digital-talent/issues/2716navikt/mock-oauth2-server
auth/
won't be mounted inside container. refaxa-group/oauth2-mock-server
(no spike)later goal: remove special-case condition inapi
for mock server tokens, for skipping check of/introspection
endpoint: https://github.com/navikt/mock-oauth2-server/pull/256done
Bonus: This brings us closer to https://github.com/GCTC-NTGC/gc-digital-talent/issues/2572
cc: @petertgiles