persist oauth2 authorize request approval in session #411
Conversation
PR Verification Succeeded: Coverage >= |
| if remember { | ||
| _ = mw.saveApprovedRequest(ctx, request) | ||
| } | ||
|
|
There was a problem hiding this comment.
error is ignored here, maybe add a log
| if v, ok := ctx.Request.PostForm[oauth2.ParameterUserApproval]; ok { | ||
| approved, _ = strconv.ParseBool(v[len(v)-1]) | ||
| } | ||
| if !approved { | ||
| return | ||
| } |
There was a problem hiding this comment.
why we need to check this? I don't see approved is used later in this function
| for _, r := range approvedRequests { | ||
| if request.ClientId == r.ClientId && | ||
| request.RedirectUri == r.RedirectUri && |
There was a problem hiding this comment.
Do we need to check redirect URL? What's the scenario where we have multiple saved requests with same client ID but different redirect URL?
Approval is usually bond to client. Maybe instead of saving the entire request, we could save a map of [client ID]->[approved scopes]?
In addition, our common experience is that user is only asked about scopes once and such decision is saved somewhere across sessions. So maybe this need to be saved somewhere else instead of session?
…ation can implement the storage mechansim as desired.
|
I made the following changes in the latest commit:
In the auth service example, an in-memory implementation is provided. |
| type Approval struct { | ||
| ClientId string | ||
| RedirectUri string | ||
| Scopes utils.StringSet | ||
| } | ||
|
|
||
| type ApprovalStore interface { | ||
| SaveApproval(c context.Context, user security.Account, a *Approval) error | ||
| LoadApprovalsByClientId(c context.Context, user security.Account, clientId string) ([]*Approval, error) | ||
| } |
There was a problem hiding this comment.
I suggest Approval to contain both client ID and user identifier (ID or username or both). And ApprovalStore to have:
SaveApproval(context.Context, *Approval) error
LoadApprovals(context.Context, opts...ApprovalLoadOptions) ([]*Approval, error)
where ApprovalLoadOptions is func(ApprovalLoadOption) or simply func(Approval).
The reason to use generic interface is to avoid future interface changes.
| if mw.approvalStore == nil { | ||
| return oauth2.NewInternalError("approval store is not available") | ||
| } |
There was a problem hiding this comment.
Since we allow approval store is nil, maybe just return nil)
| if !r.Approved { | ||
| return oauth2.NewInternalError("attempting to save unapproved request") | ||
| } |
There was a problem hiding this comment.
If user denied the requested scopes, should we store this decision?
There was a problem hiding this comment.
I don't think that's necessary because application will continue to ask for approval in that case, so it doesn't matter if the negative decision is saved or not.
| userAccount, ok := u.Principal().(security.Account) | ||
| if !ok { | ||
| return oauth2.NewInternalError("can't save approval without user account") | ||
| } |
There was a problem hiding this comment.
the "principal" might be just an username or ID, depending on the authentication config which is configured by service.
We could use security.GetUsername()
| if request.ClientId == a.ClientId && | ||
| request.RedirectUri == a.RedirectUri && | ||
| request.Scopes.Equals(a.Scopes) { | ||
| return true | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
The approvals are loaded by user and client ID, we should either check both (if we don't trust approval store) or not check.
|
|
||
| type ApprovalStore interface { | ||
| SaveApproval(c context.Context, a *Approval) error | ||
| LoadUserApprovalsByClientId(c context.Context, opts ...ApprovalLoadOptions) ([]*Approval, error) |
There was a problem hiding this comment.
Since we use options, The function should be renamed to LoadUserApprovals or LoadApprovals. I also suggest keep interface, save and load function consistent
Description
This PR is for #405
Application is responsible for providing features that allows users to manage their approvals.
Type of Change
Checklist