fix(JWT): Generate tenant claims and validate them. #914
Conversation
jimsynz
force-pushed
the
fix/validate-tenant-claim
branch
from
95aa5e3 to
686f599
Compare
|
Yeah we need to find way to not have tenant set to nil. We also need to be sure that this method isn't requiring that all non multi tenant tokens explicitly have that key and is set to nil because that will log out all users for all non multi tenant usages of AA on upgrade. |
lib/ash_authentication/jwt/config.ex
Outdated
| end | ||
| end | ||
|
|
||
| defp multitenancy_required?(user_resource) do |
There was a problem hiding this comment.
It's not about if the token is multi tenant, but if the user is multi tenant. When we generate the token we need to set the tenant into it, but only if the tenant option is set when generating the token. You can have resources that support being accessed both with a tenant and without.
There was a problem hiding this comment.
Actually I think we shouldn't worry about any of it. If the tenant option is set (to anything other than nil) in this code, then we write the tenant into the claims. If there is a tenant claim when validating, or if the tenant option is set, then we validate that the two match.
There was a problem hiding this comment.
Nope, I'm wrong. We do want to check if the user (but not the token) supports multitenancy, and if so then we add and validate the tenant. That might make it easier because in those cases it actually makes sense to add a nil tenant claim if it is nil.
There was a problem hiding this comment.
Actually I think I just realised that we can just make it optional whether we add it to the config if required.
jimsynz
force-pushed
the
fix/validate-tenant-claim
branch
from
686f599 to
94f25f5
Compare
lib/ash_authentication/jwt/config.ex
Outdated
| defp maybe_add_tenant_claim(cfg, nil), do: cfg | ||
|
|
||
| defp maybe_add_tenant_claim(cfg, tenant) do | ||
| Config.add_claim( |
There was a problem hiding this comment.
So the main thing missing here is that if there is a tenant claim, and no opts[:tenant] then it is invalid. How to we add this?
There was a problem hiding this comment.
Alright, I remember the simpler answer. its "does the user resource support multi tenancy", and if so there is a tenant claim that must match, nil or otherwise.
|
Ah, my bad, I didn't see that this PR also added a user with multi tenancy, and merged a different PR that did. I will alter this PR to use the new one from the other PR |
zachdaniel
force-pushed
the
fix/validate-tenant-claim
branch
from
94f25f5 to
771c4e6
Compare
I don't like that it always places a
"tenant" => nilclaim in the token.