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

Discord Provider #13

Merged
merged 5 commits into from
Jan 10, 2019
Merged

Discord Provider #13

merged 5 commits into from
Jan 10, 2019

Conversation

l1n
Copy link

@l1n l1n commented Jan 9, 2019

@ploxiln
Copy link
Owner

ploxiln commented Jan 9, 2019

I'd rather the import path not be renamed from github.com/bitly/oauth2_proxy to github.com/ploxiln/oauth2_proxyin this PR. It may be a bit awkward, but you can clone my repo to$GOPATH/src/github.com/bitly/oauth2_proxy`, or if you already have the bitly repo there you can add my repo as another remote and check out my master branch, etc. I'll probably do the renaming soon - but separately.

Other than that, there's just some minor whitespace/formatting issues.

README.md Outdated Show resolved Hide resolved
@l1n
Copy link
Author

l1n commented Jan 10, 2019

Fair enough, reverting that commit 👍 I didn't mean to add it in this pull request since it wasn't part of the Discord provider.

gofmt is indeed picky
@ploxiln
Copy link
Owner

ploxiln commented Jan 10, 2019

Thanks!

(I pushed up one last gofmt tweak ... the trick is gofmt -w providers/*.go)

@l1n
Copy link
Author

l1n commented Jan 10, 2019

I apologize for not speaking Go too natively, my last Go project was a year or so ago 😅 thank you for your help

p.ValidateURL = p.ProfileURL
}
if p.Scope == "" {
p.Scope = "identify email connections"
Copy link
Owner

Choose a reason for hiding this comment

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

looks like "connections" is not needed

Copy link
Owner

Choose a reason for hiding this comment

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

this and some other little refinements could be in a follow-up pull request if someone cares enough :)

(e.g. removing parts of DiscordUserInfo which are unused, maybe checking Verified property ...)

@ploxiln ploxiln merged commit 89eeb57 into ploxiln:master Jan 10, 2019
@l1n
Copy link
Author

l1n commented Jan 10, 2019

It isn't needed right now, but I'd like to extend from the user and email that are by default provided to the proxied host at some point: similar to the way that mod_shib provides displayName https://tcg.stanford.edu/?p=131

brianv0 pushed a commit to brianv0/oauth2_proxy_old that referenced this pull request Jan 17, 2019
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.

2 participants