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

added support for retrieving userProfile from profileEndpoint #7

Merged
merged 1 commit into from
Jul 14, 2019
Merged

Conversation

Iblis
Copy link
Contributor

@Iblis Iblis commented Jul 10, 2019

I hope this contains everything needed to be a good looking PR for wicked.
Normally I would also try to provide tests but this project does not have tests set up yet and this was too much of a change from my perspective.

I added a library (openid-client) that handles the userInfo call for me. Sounds like overhead, but maybe this library comes in handy when implementing full support for OIDC?`At least, this was my thougt here.
Also, I had to use an older version of openid-client since it requires node 12 as of v3,0 onwards.

The changes in this PR worked for me but I

  • didn't test for regressions with oauth2 (retrieve profile form accessToken) because I don't have a pure oauth2 server available right now
  • did my test with rc4 because this is what I am currently running and I had to customize the containers for self-signed ssl certs and did not create new ones for latest version yet.
  • did some shenaningans to the container creation locally because wicked.env:${PORTAL_ENV_TAG} did not contain my changes to package.json/node_modules

Of course, if there is anything missing here which I should have applied to the PR just let me know.

@CLAassistant
Copy link

CLAassistant commented Jul 10, 2019

CLA assistant check
All committers have signed the CLA.

@DonMartin76
Copy link
Member

Thanks a lot! I will review and come back to you.

Copy link
Member

@DonMartin76 DonMartin76 left a comment

Choose a reason for hiding this comment

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

What I noticed here is that the profile which retrieved from the userinfo endpoint (which is most probably OIDC compliant) will still run through the createDefaultProfile endpoint, which does a mapping based on the settings for the auth method. This is probably not really necessary and can be slightly confusing. It could possibly be documented in the kickstarter? What would your take on that be?

Other than that, this looks good. I just need to do the regression testing with ADFS/OAuth2 with JWT profiles, and then I would accept and merge this.

@Iblis
Copy link
Contributor Author

Iblis commented Jul 10, 2019

Ah, I thought that was intended. I know of OIDC profiles that do have different claims for what is normally known as given_name and family_name. So still being able to define which field is supposed to be name/firstname and so on sounds like a good option to me.
Even the customIdField is something where someone might like to use something else then the OIDC subject?

@DonMartin76
Copy link
Member

All good, merging, will land in a rc.7! Thanks a great bunch!

@DonMartin76 DonMartin76 merged commit dd98143 into apim-haufe-io:master Jul 14, 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.

3 participants