-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
connector: add RefreshConnector interface #702
connector: add RefreshConnector interface #702
Conversation
9dd281c
to
2f3f12d
Compare
87a4f9e
to
f1e45b0
Compare
Tests added. Manually tested LDAP and GitHub. cc @rithujohn191 ready for review |
|
||
// If a user has updated their username, that will be reflected in the | ||
// refreshed token. | ||
identity.Username = p.Username |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the identity.Username
the only field that can be updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, this is a bug, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think that practically this is the only field that could be updated. Email can't be because it's used as the ID and this doesn't deal with groups. Will expand the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. Might be helpful to add some more comments.
Email string `json:"email"` | ||
} | ||
|
||
func (c *githubConnector) user(ctx context.Context, client *http.Client) (user, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be helpful to add some comments to describe user and teams functions
f1e45b0
to
6980920
Compare
comments addressed. |
lgtm |
This is a work in progress to add a RefreshConnector interface so refresh token requests query the upstream identity provider before returning.
Lots of testing needed.
Closes #693
Updates #678