Skip to content

Refactor Microsot Auth into a testable class with expanded error handling#1500

Open
barakiva wants to merge 5 commits into
LibreBooking:developfrom
barakiva:auth-microsoft
Open

Refactor Microsot Auth into a testable class with expanded error handling#1500
barakiva wants to merge 5 commits into
LibreBooking:developfrom
barakiva:auth-microsoft

Conversation

@barakiva

Copy link
Copy Markdown
Contributor

Summary

Web/microsoft-auth.php previously handled only the success path of
Microsoft's OAuth 2.0 authorization code flow. Per the Microsoft identity
platform docs, the callback endpoint receives two distinct response shapes:

  • Success: ?code=...&state=...
  • Error: ?error=...&error_description=...

Error responses were silently ignored — the page would redirect to
Web with no error header, causing a confusing failure
downstream with no diagnostic information.

Changes

  • Extract callback logic into MicrosoftOAuthCallback (lib/Application/Authentication/)
    with a MicrosoftOAuthCallbackResult value object representing the two
    outcome cases. This makes the code more testable.
  • Handle error responses from Azure: redirect to home and log the error/description
  • unit tests covering success, URL encoding, error with/without description,
    error-takes-precedence-over-code, and missing params
  • Add workflow_dispatch trigger to CI workflows to allow manual runs on forks so contributors can make sure PRs will pass CI before submitting to upstream

Not addressed in this PR

  • state parameter verification — the Microsoft docs recommend verifying the
    returned state matches the value sent in the authorization request as a
    CSRF mitigation. This would require storing the state server-side before the
    redirect, which is a larger change.

Any feedback on testing & class design would be appreicated.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the Microsoft OAuth2 callback endpoint into a dedicated handler/value-object pair to support both success and error callback shapes, improve diagnostics, and make the logic unit-testable.

Changes:

  • Extract callback parsing/redirect resolution into MicrosoftOAuthCallback + MicrosoftOAuthCallbackResult.
  • Log and redirect on Azure error responses rather than silently failing.
  • Add unit tests for success/error/missing params and URL encoding; enable workflow_dispatch for CI workflows.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
Web/microsoft-auth.php Uses the new callback handler and adds error logging before redirecting.
lib/Application/Authentication/MicrosoftOAuthCallback.php Introduces the callback handler and result value object for success/error outcomes.
tests/Application/Authentication/MicrosoftOAuthCallbackTest.php Adds unit tests covering success, encoding, and error/missing-params behavior.
.github/workflows/phpunit.yml Adds manual workflow trigger (workflow_dispatch).
.github/workflows/lint-and-analyse-php.yml Adds manual workflow trigger (workflow_dispatch).
.github/workflows/integration-tests.yml Adds manual workflow trigger (workflow_dispatch).
.github/workflows/docs.yml Adds manual workflow trigger (workflow_dispatch).

Comment thread Web/microsoft-auth.php Outdated
Comment thread Web/microsoft-auth.php Outdated
Comment thread lib/Application/Authentication/MicrosoftOAuthCallback.php Outdated
Comment thread lib/Application/Authentication/MicrosoftOAuthCallback.php Outdated
Comment thread lib/Application/Authentication/MicrosoftOAuthCallback.php
Comment thread lib/Application/Authentication/MicrosoftOAuthCallback.php
Comment thread tests/Application/Authentication/MicrosoftOAuthCallbackTest.php
Comment thread .github/workflows/lint-and-analyse-php.yml Outdated
Comment thread .github/workflows/integration-tests.yml Outdated
Comment thread .github/workflows/docs.yml Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@barakiva

Copy link
Copy Markdown
Contributor Author

Applied all code reivew suggestions and unit tests all pass.

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.

2 participants