Skip to content
This repository has been archived by the owner on Apr 2, 2019. It is now read-only.

Fetch userinfos with access token (fixes #80) #89

Merged
merged 9 commits into from
Jan 26, 2018

Conversation

leplatrem
Copy link
Collaborator

@leplatrem leplatrem commented Jan 19, 2018

Fixes #80

  • hard coded manual testing
  • clean up interfaces and flow
  • refactor notion of Claims to notion of UserInfos
  • find an efficient way to distinguish access token from id tokens
  • cache userinfos
  • update docs and workflows...

Stretch goals (another PR if necessary)

  • update example with two distinct APIs
  • check with IT/security team about using userinfos to «validate» the access token (or add call to authorize ?)
  • Rename the config field jwtIssuer to something more generic like identifyProvider

@leplatrem leplatrem changed the title [WIP] Fetch userinfos with access token Fetch userinfos with access token Jan 24, 2018
@leplatrem leplatrem changed the title Fetch userinfos with access token Fetch userinfos with access token (fixes #80) Jan 25, 2018
authn/authn.go Outdated
// identity provider.
func NewAuthenticator(idP string) (Authenticator, error) {
if !strings.HasPrefix(idP, "https://") {
return nil, fmt.Errorf("Identity provider %q not supported or has bad format", idP)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this error message would be better as, fmt.Errorf("Identify provider %q does not use the https scheme", idP). That is clearer what the failure/rejection reason is.

}
return userinfo, nil
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

documentation for FetchUserInfo?

authn/openid.go Outdated
return nil, err
}
uri := config.UserInfoEndpoint
log.Debugf("Fetch user info from %s", uri)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this debug log line?

Copy link

@mostlygeek mostlygeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an excellent refactor. I found the new interfaces and encapsulation makes the logic really easy to follow. Even without a deep understanding of the the protocols or context it is very readable.

Other than a few nitpicks +1

@leplatrem leplatrem merged commit df7d851 into master Jan 26, 2018
@leplatrem leplatrem deleted the 80-access-token-userinfos branch January 26, 2018 08:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants