fix: reject oauth logins when neither token references a user id - #106
fix: reject oauth logins when neither token references a user id#106FrenchGithubUser wants to merge 2 commits into
Conversation
006ef16 to
14dc33c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #106 +/- ##
==========================================
+ Coverage 84.58% 84.60% +0.01%
==========================================
Files 13 13
Lines 902 903 +1
Branches 163 164 +1
==========================================
+ Hits 763 764 +1
Misses 85 85
Partials 54 54
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
| # Neither localpart_path nor fq_uid_path is configured, so the token | ||
| # references no user id. The login must be rejected instead of trusting | ||
| # the client-supplied username (which would allow authing as any user). |
There was a problem hiding this comment.
Maybe we should be guarding this at the config level instead of waiting for the code to just error out with no real reason why? 🤔 Any reasons why you don't think it's a good idea?
There was a problem hiding this comment.
yes we can do this check at the config level, but we also need to keep one at the runtime level in case the path provided returns None for the given token
| - At least one of `jwt_validation` or `introspection_validation` must be defined. | ||
| - `expose_metadata_resource` must be an object with `name` field. The object will be exposed at `/_famedly/login/{expose_metadata_resource.name}`. | ||
|
|
||
| At least one of the tokens (the JWT or the introspection response) must reference a user id via its `localpart_path` or `fq_uid_path`. If neither token provides a user id, authentication is rejected, even when `username_type` would otherwise allow deriving the user id from the client-supplied username, otherwise a holder of any valid token could authenticate as an arbitrary user. |
There was a problem hiding this comment.
| At least one of the tokens (the JWT or the introspection response) must reference a user id via its `localpart_path` or `fq_uid_path`. If neither token provides a user id, authentication is rejected, even when `username_type` would otherwise allow deriving the user id from the client-supplied username, otherwise a holder of any valid token could authenticate as an arbitrary user. | |
| A user id must be provided by at least one of the tokens (the JWT or the introspection response) via its `localpart_path` or `fq_uid_path`. If neither token provides a user id, authentication is rejected. |
Actually, this could probably be rolled into that WARNING about 20 lines above since some of that is now invalid
There was a problem hiding this comment.
yea indeed, I had to rebase because my branch wasn't up to date after making changes, and I missed that
|
|
||
| **WARNING**: It is possible to configure in such a way that authentication would always fail. If `username_type` is `null`, and `localpart_path` and `fq_uid_path` are also not specified, no user id data can be sourced, thus also leading to failure. | ||
| But if `username_type` is `null`, but either `localpart_path` or `fq_uid_path` is provided, the authentication process can continue. | ||
| **WARNING**: A user id must be provided by at least one of the tokens (the JWT or the introspection response) via its `localpart_path` or `fq_uid_path`. If neither token provides a user id, authentication is rejected. |
There was a problem hiding this comment.
I think it loses something by just replacing it out right. Some of it didn't make sense but some of it still does. Let's see here.
At least one of the tokens (the JWT or the introspection response) must reference a user id via its
localpart_pathorfq_uid_path. If neither token provides a user id, authentication is rejected, even whenusername_typewould otherwise allow deriving the user id from the client-supplied username, otherwise a holder of any valid token could authenticate as an arbitrary user.
Was what you had originally. Let's roll back to what the warning said originally and work from that
| **WARNING**: A user id must be provided by at least one of the tokens (the JWT or the introspection response) via its `localpart_path` or `fq_uid_path`. If neither token provides a user id, authentication is rejected. | |
| **WARNING**: It is possible to configure in such a way that authentication would always fail. If neither `localpart_path` nor `fq_uid_path` are specified in any config section, then no user id data can be sourced for validation leading to failure. | |
| If `username_type` is `null`, but either `localpart_path` or `fq_uid_path` is provided, the authentication process can continue. |
What about something like that? Basically I think the only part that changed from the original is that the reference to username_type being null changed to username_type being any which means it can be left out. The important bit there is that the localpart_path or fq_uid_path must be present to validate a user id. Without them is guaranteed failure, right?
There was a problem hiding this comment.
There are already tests for bad data, but not missing data. So, I have a test request that I do not see in the file.
Can you do a copy() on the default_claims() and remove the existing "urn:messaging:matrix:localpart" and "urn:messaging:matrix:mxid". This way they can resolve to None. I'm not to concerned which *-path config thing you use to try and source one of them, figure either one should do the trick. We just want to make sure that what you are changing is what the gap was before. Does that make sense?
2 participants
SYN-125