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

Made issueby customizable and with feedback #33

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

jeremy379
Copy link
Owner

@jeremy379 jeremy379 commented Dec 17, 2024

Changed

New configuration key "issueBy" allowing to choose how the issueBy field is populated.

Options are:

  • "laravel-url", use laravel feature from "url('/')"
  • "auto-detect" (defaultà : use $_SERVER to detect host and scheme
  • Any other string will be used "as is".

@alecpl
Copy link
Contributor

alecpl commented Dec 17, 2024

Here's some comments:

  • it should be "issuedBy" not "issueBy"
  • the value in the DiscoveryController and IdTokenResponse must be the same.
  • in the IdTokenController we have $currentRequestService which we probably should use (as it was before my changes there), but I'm not sure about how to get the request in the DiscoveryController.

@bbredewold
Copy link
Contributor

With @alecpl remarks, it looks good to me :)

@jeremy379
Copy link
Owner Author

Thanks for your feedback, I've applied some changed

  • Extracted the retrieval of the value in a dedicated class
  • Using back the CurrentRequestService by default but with a fallback on $_SERVER value (opt-in via configuration)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants