Skip to content
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

Allow configuring scopes and extraUserInfo #98

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joscha-alisch
Copy link

This adds a new method to add providers that allows to configure additional scopes and a hook that is called with the authenticated HTTP client. This is a draft to resolve issue #97 .

Usage is like this for the GitHub provider for example, which would fetch the orgs/teams a user is in and attach them to the user.Token.

service.AddProviderWithOptions("github", "cid", "csecret", []string{"read:org"}, func(c *http.Client, u token.User) token.User {
    gh := github.NewClient(c)
    t, _, _ := gh.Teams.ListUserTeams(context.Background(), &github.ListOptions{})
    
    var teams []string
    for _, team := range t {
	    teams = append(teams, fmt.Sprintf("%s:%s", team.Organization.GetLogin(), team.GetName()))
    }
    
    u.SetSliceAttr("teams", teams)
    return u
})

@joscha-alisch joscha-alisch marked this pull request as draft October 6, 2021 10:23
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1311457085

  • 4 of 17 (23.53%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-4.8%) to 90.734%

Changes Missing Coverage Covered Lines Changed/Added Lines %
auth.go 4 17 23.53%
Totals Coverage Status
Change from base Build 1201515370: -4.8%
Covered Lines: 235
Relevant Lines: 259

💛 - Coveralls

@umputun
Copy link
Member

umputun commented Oct 11, 2021

it would be nice to cover the change with tests. almost 5% coverage decrease by itself is not such a problem, however not having any tests for extra functionality - is a problem.

@joscha-alisch
Copy link
Author

Certainly :) this was not intended as a ready-to-merge PR, but rather as a first iteration to get feedback on the direction. Hence the draft PR.

I'll go ahead and add tests.

@umputun
Copy link
Member

umputun commented Oct 30, 2021

@joscha-alisch I'm not sure if you expect me to review the PR as it is marked as draft

@joscha-alisch
Copy link
Author

joscha-alisch commented Oct 31, 2021

No, i Just didn't have time to add the tests yet. Will let you know when it's ready :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants