Add user to context after login and signup #3088
Conversation
internal/access/login.go
Outdated
| func Login(c RequestContext, loginMethod authn.LoginMethod, keyExpiresAt time.Time, keyExtension time.Duration) (authn.LoginResult, error) { | ||
| return authn.Login(c.Request.Context(), c.DBTxn, loginMethod, keyExpiresAt, keyExtension) |
There was a problem hiding this comment.
I would really like to remove this access.Login function, but I suspect that is a controversial change.
If we remove this function we'd have the api handler calling the authn.Login directly, which removes one hop when tracing the code. The access.Login function does nothing except pull two values out of RequestContext, which could easily be done by the caller without adding additional lines.
There was a problem hiding this comment.
I think that's actually a good idea, login doesn't really have the concept of an "access check" anyway.
internal/access/login.go
Outdated
| func Login(c RequestContext, loginMethod authn.LoginMethod, keyExpiresAt time.Time, keyExtension time.Duration) (authn.LoginResult, error) { | ||
| return authn.Login(c.Request.Context(), c.DBTxn, loginMethod, keyExpiresAt, keyExtension) |
There was a problem hiding this comment.
I think that's actually a good idea, login doesn't really have the concept of an "access check" anyway.
So that we can add it to the request context for logging.
The TODOs here suggested that we might be able to remove this field entirely, but it looks like the associated issue was closed and we may still need it. I suspect we should keep this field instead of trying to infer it from some other data, so that we can use it in other cases in the future. For example, we may have to invalidate passwords and force someone to create a new one, and this field would work great for that purpose. The RequiresUpdate method is removed in favor of returning the value on the existing struct. This also removes the need to query for the user and credentials a second time.
So that logging middleware can include the userID in the log message for the request.
dnephin
force-pushed
the
dnephin/add-user-to-context-after-login
branch
from
720c809
to
2746fdb
Compare
|
I pushed one more commit that adds API test coverage for login with a temp password, to make sure I did not mess up anything with the refactor. |
Summary
Primarily this PR sets
RequestContext.Authenticated.Userafter login and signup. Once added to the request context it will appear in the API request log. All other requests already have this value set. We don't have any way to test our logs right now, so I tested this manually on a branch that included #3073, and I saw the userId set on the log line for login.To accomplish this goal I did a bit of refactoring to expose the user we had already fetched from the database. I tried to split that refactor into smaller commits, so view by individual commit if the full diff doesn't read well.
As part of this change I removed the
LoginMethod.RequiresUpdatemethod, which had a TODO on it to be removed. The issue the TODO links to is already closed.Related Issues
Along with the changes in #3073, this PR resolves #3031