Support for m.login.token
#2014
Conversation
|
TAP failures: SyTest 611 and 612 are failing in master as well. |
There was a problem hiding this comment.
Overall this is an excellent PR — bonus points for unit tests! — and I'm sorry that it's taken me so long to get around to reviewing it.
I think some of the new copyright headers probably need fixing (to 2021) and to ensure the account hasn't been deleted in the meantime (as per the review comment) but other than that it looks good.
Only other thought I have is around the use of CURRENT_TIMESTAMP in the SQL queries, mostly because if the database is running on a different machine and the clocks drift, this might result in a bug that's otherwise very difficult to track down. I don't know if @kegsay has an opinion on this either way.
The user doesn't necessarily have it in PATH.
…gin.sso.
For login token:
* m.login.token will require deleting the token after completeAuth has
generated an access token, so a cleanup function is returned by
Type.Login.
* Allowing different login types will require parsing the /login body
twice: first to extract the "type" and then the type-specific parsing.
Thus, we will have to buffer the request JSON in /login, like
UserInteractive already does.
For SSO:
* NewUserInteractive will have to also use GetAccountByLocalpart. It
makes more sense to just pass a (narrowed-down) accountDB interface
to it than adding more function pointers.
Code quality:
* Passing around (and down-casting) interface{} for login request types
has drawbacks in terms of type-safety, and no inherent benefits. We
always decode JSON anyway. Hence renaming to Type.LoginFromJSON. Code
that directly uses LoginTypePassword with parsed data can still use
Login.
* Removed a TODO for SSO. This is already tracked in matrix-org#1297.
* httputil.UnmarshalJSON is useful because it returns a JSONResponse.
This change is intended to have no functional changes.
This adds full lifecycle functions for login tokens: create, query, delete.
Thank you. Pushed new version.
Done. It seems
If you have multiple machines in the Polylith, you'll have that problem anyway. Or if there's a hot standby that takes over. But since you don't use On that topic... In other places, you use |
|
Also fixed localpart/MXID confusion in the |
Yes, I think that's probably worth looking into.
True, but in that case the obvious place to look is arguably the machines running the polylith components and probably not the machines running the databases :-)
Many of those cases are from before my time so I don't know the exact reason for that, although I suspect it's just to do with speeding up pulling things from the database without implicit conversions. On a codepath like this I don't suppose it makes too much difference either way. A common pattern of ours is to treat the database as though it's as noddy as possible, which is why you don't really find anything like p.s. please avoid force-pushes wherever possible, it makes it very difficult to tell what's changed or whether previous merges were kept! |
Will do, thanks. I thought GitHub would keep track of things, but I noticed the review notes became inaccessible. Lesson learned. In general, do you want a final rebase-fixup pass after review to tidy up the log, or would you just merge it with the fixup commits separately? |
We squash-merge all PRs anyway down into single commits, so no harm really in just adding more commits as needed. |
| // called after authorization has completed, with the result of the authorization. | ||
| // If the final return value is non-nil, an error occurred and the cleanup function | ||
| // is nil. | ||
| func LoginFromJSONReader(ctx context.Context, r io.Reader, accountDB AccountDatabase, userAPI UserInternalAPIForLogin, cfg *config.ClientAPI) (*Login, LoginCleanupFunc, *util.JSONResponse) { |
There was a problem hiding this comment.
This has a messy function signature. I'm surprised we need a cleanup function.
There was a problem hiding this comment.
This is just to ensure cleanup after completeAuth. With more refactoring, that relationship could surely be inversed so that the cleanup isn't needed. I wanted to keep the diff small. Let me know if you want more.
There was a problem hiding this comment.
Some more rationale on this code: I'm trying to keep this function non-mutating. It's just parsing JSON and returning a struct. For tokens, we need to (1) parse, (2) create an access token, (3) remove the login token. And (3) shouldn't happen if (2) fails. However, I also wanted to avoid type-specific switches happening later in Login. Hence the idea of simply returning a cleanup function along with the Login (which is just the user and device identities).
The first alternative I can come up with is making Login an interface, and have type-specific implementations. That seemed overkill and a larger change, but certainly doable.
|
(@neilalexander) I think a merge with master went awry here. I think something had updated the PR with two master commits, but I'm not sure. Either way, it seems to have overruled my local merge commit on pull. Since we had this discussion before (and I'm still not friends with GitHub PRs): I'll leave them be until told otherwise. |
|
@tommie As long as the changes are still present and haven't been reverted anywhere, I wouldn't worry about the merge commits — it is normal to get a merge commit when you pull in remote changes from a divergent head. We will squash merge everything down into a single commit when we merge anyway. |
There was a problem hiding this comment.
Overall LGTM. There's 2 minor things I want to see addressed but they shouldn't block the merging of this PR:
- It's impossible to opt-out of
m.login.tokenlogic. For servers which don't do this, we should bail much much earlier but we don't. Also, we don't advertise that we support this login form on GET /login. - The login token generation code is duplicated across postgres/sqlite. We should factor this out to a shared package.
|
Sorry I'm late.
Note that login-token authentication is a no-op if nothing generates that token. But I understand if you want to reduce the attack surface for servers that don't need it. I'm guessing you mean this early failure should be done in
AFAICT, the spec doesn't advertise it as a login method. It does advertise I assumed this was because clients can't really do anything with the knowledge of
I agree, though it doesn't matter much how the token is generated as long as it's difficult to guess. (There's no need to be consistent between database engines.) I see you merged this to main. Do you want me to write a fix for the above? |
|
Yes, it would be nice to get those fixes in at some point. |
Given that we shouldn't advertise
This seems to have been fixed in 9f4a39e. |
Some refactorings to support more than one authentication mechanism. Lots of database plumbing for storing the login tokens. One small fix to a utility script.
sytest-whitelistas specified in docs/sytest.mdThis is a heavily modified fork of #1374. Closes #403. Pre-requisite for resolving #1297.
Signed-off-by:
Tommie Gannert <[email protected]>