tests: Revise OAuth2 tests#3795
Merged
polarathene merged 11 commits intodocker-mailserver:masterfrom Jan 20, 2024
Merged
Conversation
Better documented, easier flow and separation of concerns via Caddy. The python code had additional noise related to setting up a basic API which is abstracted away via `Caddyfile` config that's dedicated to this task.
Caddyfile can use an Access Token instead of a JWT. Much smaller and correct for this OAuth2 configuration. This new value has been documented inline. Likewise the `sub` field returned is not important to this test. `email_verified` is kept as it may be helpful for further coverage testing. The actual test-case has better assertions for success and failure by checking for Dovecot logs we expect instead of netcat response. `oauth2` to `auth` for the Caddy container hostname is not necessary, just a more generic subdomain choice.
This way is more flexible and doesn't require modifying the `Caddyfile` directly, while still easy to use. Additionally simplifies understanding the Caddyfile to maintainers by removing the `route` directive that was required to ensure a deterministic order of vars.
Since this is the only intended usage, might as well have it respond with the full file content.
Caddyfile route for `/imap/` now accepts any subpath to support handling both `xoauth2` and `oauthbearer` subpaths. Both SASL mechanisms represent the same information, with `XOAUTH2` being a common mechanism to encounter defined by Google, whilst `OAUTHBEARER` is the newer variant standardized by RFC 7628 but not yet as widely adopted. The request to `/userinfo` endpoint will be the same, only the `credentials` value to be encoded differs. Instead of repeating the block for a similar route, this difference is handled via the Caddyfile `map` directive. We match the path context (_`/xoauth2` or `/oauthbearer`, the `/imap` prefix was stripped by `handle_path` earlier_), when there is a valid match, `sasl_mechanism` and `credentials` map vars are created and assigned to be referenced by the later `respond` directive. --- Repeat the same test-case logic, DRY with log asserts extracted to a common function call. This should be fine as the auth method will be sufficient to match against or a common failure caught.
Separate test cases and additional comment on creating the same base64 encoded credentials via CLI as an alternative to running Caddy. Added a simple `compose.yaml` for troubleshooting or running the container for the `/imap/xoauth2` / `/imap/oauthbearer` endpoints.
`reverse_proxy` was a bit more convenient, but the additional internal ports weren't really relevant. It also added noise to logging when troubleshooting. The `import` directive with Snippet blocks instead is a bit cleaner, but when used in a single file snippets must be defined prior to referencing them with the `import` directive. --- `compose.yaml` inlines the examples, with slight modification to `localhost:80`, since the Caddyfile examples `auth.example.test` is more relevant to the tests which can use it, and not applicable to troubleshooting locally outside of tests.
polarathene
commented
Jan 19, 2024
georglauterbach
previously approved these changes
Jan 19, 2024
Member
georglauterbach
left a comment
There was a problem hiding this comment.
LGTM 👍🏼
I am not too deep into OAUTH2, but might use it as well (later) when we have something like OIDC going. These changes look very good, and the additional information in the form of comments is appreciated!
casperklein
previously approved these changes
Jan 19, 2024
7e14df3
Co-authored-by: Georg Lauterbach <[email protected]>
georglauterbach
approved these changes
Jan 20, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This is a follow up to the original feature PR #3480
Each commit provides commit message for extra context. Changes have been staged into iterations towards the full review diff (so there's a little bit of extra noise there). Should be easy to follow if the full diff is too much.
Overview
MTA-STSin future.XOAUTH2(Googles widely adopted implementation) andOAUTHBEARER(the newer variant standardized by RFC 7628 in 2015).curlagainst it with the two query string parameters for dynamic values.compose.yamlfor troubleshooting outside of a running test.If time would permit, I'd extend the tests further for Dovecot configuration related to this feature. Caddy keeps the interactions fairly simple and transparent for communicating expectations to maintainers along with assistance in troubleshooting.
Extra info
If needing to troubleshoot, adding the following (global options) to the
Caddyfilefor more helpful information:{ # Enables logging without omitting sensitive data # https://caddyserver.com/docs/caddyfile/directives/log # https://caddyserver.com/docs/caddyfile/options#log-credentials servers { log_credentials } # Slightly better output for console viewing: # https://caddyserver.com/docs/caddyfile/directives/log#format-modules log { format console { # This is a Go time format string, these values are not random but intentional: time_format "15:04:05" } } }Type of change
Checklist:
CHANGELOG.md