E527 Add possibility to get additional user data (additional scope) by VladimirZaets · Pull Request #163 · go-pkgz/auth · GitHub
[go: up one dir, main page]
Skip to content

Add possibility to get additional user data (additional scope) #163

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.

Sign up for GitHub

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

Jump to bottom
Merged
umputun merged 3 commits into from
Sep 10, 2023
Merged

Add possibility to get additional user data (additional scope) #163

umputun merged 3 commits into from
Sep 10, 2023

Conversation

@VladimirZaets
Copy link
Contributor
@VladimirZaets VladimirZaets commented May 4, 2023

Hi,
I added the possibility to get additional customer information (instead of just username, avatar and id).

Use case:
I use this library in my project and want to simplify user authorization as much as possible. My application is require to have at least email to proceed of using full functionality of application, and I don't see any reason to limit it if customer already provide it in one of the providers and authorize to use this data in application.

Backward compatibility
The solution is backward compatible. I saw that library still not released and we can legally broke contracts, but to avoid additional work for customers I didn't change API, just add new public method to use.

All additional fields will go under attributes section. Example:

{
"name":"Volodymyr Zaiets",
"id":"github_368140cb0b52a8d0758818421b75b40aa6d8e25f",
"picture":"http://localhost:8080/avatar/",
"aud":"freehands",
"attrs":{"avatar_url":"https://avatars.githubusercontent.com/u/8035672?v=4","email":"vzaets@adobe.com"}
}

@VladimirZaets VladimirZaets requested a review from umputun as a code owner May 4, 2023 20:49
@VladimirZaets VladimirZaets mentioned this pull request May 10, 2023
Closed
@umputun
Copy link
Member
umputun commented May 22, 2023

could you pls address linter warnings?

@VladimirZaets
Copy link
Contributor Author
VladimirZaets commented May 24, 2023 •
edited
Loading

@umputun done.

BTW regarding BearerTokenHook type.
I see it executed synchronously

if p.bearerTokenHook != nil && tok != nil {
       p.Logf("[DEBUG] pass bearer token %s, %s", p.Name(), tok.TokenType)
       p.bearerTokenHook(p.Name(), u, *tok)
}

I want to clarify for myself, do we look on it like on real hook, I mean we doesn't worry what happens inside, just notify and forget or we treat it like a part of auth workflow?

For me, this type a little bit wrong in both cases.
In case of just "notify and forget" I believe we need to execute it asynchronously and doesn't block response for the user (that hook callback can contain some have operations"

In case of "part of workflow", we should respect the errors from that callback. So the type should looks like type BearerTokenHook func(provider string, user token.User, token oauth2.Token) error
With this response error we will be able to fail auth process and return "internal server error". Thats can be needed as an example in the case when we want to save the user to db or call any other remote services and that operation fails. In this case we may want to fail auth process as a whole. (like atomic operation complete fulll or fail)

if p.bearerTokenHook != nil && tok != nil {
       p.Logf("[DEBUG] pass bearer token %s, %s", p.Name(), tok.TokenType)
       err := p.bearerTokenHook(p.Name(), u, *tok)
       
       if err !=nil {
              rest.SendErrorJSON(w, r, p.L, http.StatusInternalServerError, err, "failed to _____")
       }
}

WDYT?

@umputun
Copy link
Member
umputun commented Sep 10, 2023

For some reason, GH never sent me an update about this change and also has not run actions on it. I just discovered it, and have found a minor comment lint issue and fixed it. Going to merge.

Regarding BearerTokenHook - i believe this supposed to be a "part of workflow" but we better should check with the original author (@nbys) of the custom oauth2 server

I'm going to merge it, but feel free to open a discussion about p.bearerTokenHook error check or submit a PR addressing it

@umputun umputun merged commit ef1a340 into go-pkgz:master Sep 10, 2023
umputun added a commit that referenced this pull request Sep 10, 2023
@umputun
add test for providers with custom scopes for #163
1dc2abe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@umputun umputun Awaiting requested review from umputun umputun is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@VladimirZaets @umputun
0