fix: don't delete the session when a resource requires a tenant - #1198
Open
bars0udin wants to merge 1 commit into
Open
fix: don't delete the session when a resource requires a tenant#1198bars0udin wants to merge 1 commit into
bars0udin wants to merge 1 commit into
Conversation
`retrieve_from_session/3` iterates every authenticated resource for the otp_app and deletes the session key of each one it can't resolve a user for. For a multitenant resource with `global? false`, a request that carries no tenant fails with `Ash.Error.Invalid.TenantRequired` before the query reaches the data layer, so the session is discarded without anything having been learned about whether it was valid. Only delete the session key when the lookup was actually able to run.
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.
The failure mode
Plug.Helpers.retrieve_from_session/3iterates every authenticated resource for the otp_app and deletes the session key of each one it can't resolve a user for. For a multitenant resource withglobal? false, a request that carries no tenant fails withAsh.Error.Invalid.TenantRequired— the query is rejected byAsh.Actions.Read.validate_multitenancy/1before it reaches the data layer — and the perfectly good session is deleted anyway.Who it affects
Any app with a tenant-scoped authenticated resource plus a pipeline that doesn't set an Ash tenant: an admin/staff area, a webhook endpoint, a health check, or simply a second authenticated resource whose pipeline is tenant-less. Users of the multitenant resource are silently signed out by requests to those routes.
Note that
retrieve_from_session/3reads the tenant fromAsh.PlugHelpers.get_tenant(conn)only, so this bites whenever the plug pipeline hasn't set one —assign_new_resources/4has asession["tenant"]fallback, but the plug path does not.Reproduction
Two authenticated resources, one of them multitenant and not
global?; a request through a pipeline that sets no tenant. The multitenant resource's session key is gone afterwards even though the subject is untouched. The added test uses the existingExample.MultiTenantUserWithWebAuthnfixture, which is alreadymultitenancy do strategy :context end.The fix
Only delete the session key when the lookup was actually able to run. If the resource declares multitenancy, isn't
global?, and this request has no tenant, the failure says nothing about the session, so leave it alone. Genuinely absent or unresolvable subjects are still cleared, as covered by the second added test.