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

User: ListGroupMemberships #5

Merged

Conversation

simongottschlag
Copy link
Contributor

@simongottschlag simongottschlag commented Jan 18, 2021

Changes:

  • clients/user.go: Added ListGroupMemberships to list user transitive memberOf
  • clients/user_test.go: Added tests for ListGroupMemberships
  • auth/claims.go: Changed from base64.StdEncoding. to base64.RawStdEncoding. to fix error decoding access token. This is because JWTs are not padded which StdEncoding is expecting.

Any and all feedback, help, suggestions, edits etc is welcome!

@simongottschlag
Copy link
Contributor Author

@manicminer is pagination supported by Golang / base or is it something that needs to be implemented? Not sure how many groups needs to be created to actually test it.

Copy link
Owner

@manicminer manicminer left a comment

Choose a reason for hiding this comment

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

@simongottschlag Thanks for the PR, this is great! I just have one niggly request on the method name.

For pagination, there's no capability at present but I can look to add this quickly as it's essential and I'm sure to run into issues myself any time now without it.

clients/users.go Outdated Show resolved Hide resolved
clients/users_test.go Outdated Show resolved Hide resolved
@manicminer manicminer added the enhancement New feature or request label Jan 18, 2021
@simongottschlag
Copy link
Contributor Author

@manicminer I will update the name tomorrow. Used the same as they have in the graphrbac client because that's what I've been using.

I will create a new issue for pagination.

@simongottschlag simongottschlag changed the title User: GetMemberGroups User: ListGroupMemberships Jan 19, 2021
@simongottschlag
Copy link
Contributor Author

@manicminer I've updated the method names.

@@ -30,7 +31,7 @@ func ParseClaims(token *oauth2.Token) (claims Claims, err error) {
return
}
jwt := strings.Split(token.AccessToken, ".")
payload, err := base64.StdEncoding.DecodeString(jwt[1])
payload, err := base64.RawStdEncoding.DecodeString(jwt[1])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@manicminer the reason RawStdEncoding is required is because JWTs are not padded, and StdEncoding expects padding.

https://www.rfc-editor.org/rfc/rfc7515.html#section-2

   Base64url Encoding
      Base64 encoding using the URL- and filename-safe character set
      defined in Section 5 of RFC 4648 [RFC4648], with all trailing '='
      characters omitted (as permitted by Section 3.2) and without the
      inclusion of any line breaks, whitespace, or other additional
      characters.  Note that the base64url encoding of the empty octet
      sequence is the empty string.  (See Appendix C for notes on
      implementing base64url encoding without padding.)

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks this makes sense. I didn't hit any encoding issues in testing presumably because my JWT payloads were short (minimal set of claims).

Copy link
Contributor Author

@simongottschlag simongottschlag Jan 19, 2021

Choose a reason for hiding this comment

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

@manicminer I think you have something in 1/4 to not get padding. 👍

@manicminer manicminer merged commit 4f05863 into manicminer:main Jan 19, 2021
manicminer added a commit that referenced this pull request Jan 19, 2021
@simongottschlag simongottschlag deleted the feature/list-transitivememberof branch January 19, 2021 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants