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

Get rid of custom laravel auth app #2755

Closed
5 of 6 tasks
patcon opened this issue May 16, 2022 · 5 comments · Fixed by #2813
Closed
5 of 6 tasks

Get rid of custom laravel auth app #2755

patcon opened this issue May 16, 2022 · 5 comments · Fixed by #2813
Labels
tooling Tooling, automation and CI to support development.

Comments

@patcon
Copy link
Contributor

patcon commented May 16, 2022

This has come up as a nice-to-have, so that:

  • we have one less app to maintain,
  • we have more feature parity with production auth server, and
  • we don't need to disable auth in e2e tests and leave security-critical code paths unexercised

To Do

Bonus: This brings us closer to #2572

cc: @petertgiles

@petertgiles
Copy link
Contributor

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:

  • Remove Laravel auth
  • Remove all Laravel auth specific code flows and tests
  • Add mock auth server
  • Integrate mock auth server in dev environment
  • Integrate mock auth server in tests
  • Document use

@patcon
Copy link
Contributor Author

patcon commented May 16, 2022

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 #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 #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.)

@tristan-orourke
Copy link
Member

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.

@patcon
Copy link
Contributor Author

patcon commented May 17, 2022

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 #2716 is ready.

@tristan-orourke tristan-orourke added the tooling Tooling, automation and CI to support development. label May 18, 2022
@tristan-orourke
Copy link
Member

Please add your planning poker estimate with ZenHub @patcon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling Tooling, automation and CI to support development.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants