-
Notifications
You must be signed in to change notification settings - Fork 149
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
server: refactor: introduce AuthUser
interface
#838
base: master
Are you sure you want to change the base?
Conversation
Note that there seems to be an issue with formatting in this project... I'm using VS Code along its Java extension to develop and it often feels like reorganizing or cleaning up imports. I'd be in favor of keeping the cleanup as part of these changes. It would be good to look into making auto-formatting part of the build step, maybe as a gradle plugin of some kind? |
server/src/main/java/org/eclipse/openvsx/security/DefaultAuthUser.java
Outdated
Show resolved
Hide resolved
Rebased my branch. |
@Service | ||
public class AuthUserFactory { | ||
|
||
public AuthUser createAuthUser(String providerId, OAuth2User oauth2User) { | ||
return new DefaultAuthUser( | ||
oauth2User.getName(), | ||
oauth2User.getAttribute("avatar_url"), | ||
oauth2User.getAttribute("email"), | ||
oauth2User.getAttribute("name"), | ||
oauth2User.getAttribute("login"), | ||
providerId, | ||
oauth2User.getAttribute("html_url") | ||
); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HeikoBoettger this class should make it easier to customize AuthUser
instances. Right now it is hardcoded but you can later make this factory depend on some Spring configuration to customize attribute access.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paul-marechal I am not so familiar with spring, do you know how to map the "login" in the spring configuration? I only know about the "spring.security.oauth2.client.provider.my-oauth-provider.user-name-attribute=name". May be short hint somewhere in the openvsx documentation on how to customize the attribute access would be good.
Great work by the way.
@HeikoBoettger Can you share which additional identity provider you were able to implement using this PR? |
@amvanbaren Somehow I lost my comment. The PR didn't help completely, what is missing is basically a mapping of the login-attribute. One could add a GitLab specific mapping to the code, what I did based on this merge request adding a completely new generic provider type which uses additional setting from the application.yaml file defining for the web-interface what provider to use for the login-link and how to map the login (supporting mapping any of the attributes in the AuthUserFactory). If the code could be changed to use spring.security.oauth2.client.provider.[providerId].user-name-attribute / providerDetails.userInfoEndpoint.userNameAttributeName as described in https://docs.spring.io/spring-security/reference/servlet/oauth2/login/core.html#oauth2login-sample-application-config. Instead of login I could abuse the GitHub provider entry. To fully support other providers the UI would require a change to select the login method and the AuthToken inside the UserData class need to be contain a map from provider to token:
Or the code gets simplified to just store an AuthToken for one provider at a time. I don't know why there are two fields in the UserData when there is only one provider-String. |
@paul-marechal Is it possible to rebase this branch on to the latest master? |
The |
Replace the references to `OAuth2User` by `AuthUser`. This allows downstream extenders to more easily contribute alternative OAuth2 providers: If the expected data is stored in different attributes it will be possible to bridge it by implementing the proper `AuthUser`.
8f65be8
to
0e07da9
Compare
Introduce a configuration to define attribute names to use when mapping attributes from an auth provider.
Allow configuring the auth for arbitrary providers (other than github).
Ok so I initially didn't want to bother implementing the configuration of attributes because it adds a decent amount of complexity... I changed my mind figuring it might make someone else's life easier. The configuration may look like this: ovsx:
auth:
attribute-names:
gitlab:
avatar-url: awdawd
email: awdawd
full-name: awdawd
login-name: awdawd
provider-url: awdawd We don't need to define mappings for |
@paul-marechal Thanks for providing the configuration. I'll try to configure Google OAuth using this PR. |
I want to apologize for forgetting to transpose code to handle Second, there's still a need to manually edit the webui code to change the OAuth2 authorization endpoint used from This PR is slowly growing bigger than what we required on our fork so I hope this will help. |
Yes, the login url is hardcoded in the webui: openvsx/webui/src/extension-registry-service.ts Lines 27 to 29 in 0f5315d
The openvsx/server/src/main/java/org/eclipse/openvsx/UserAPI.java Lines 72 to 77 in 0f5315d
|
I implemented the dynamic redirection based on a server configuration, I didn't encounter a CORS error? ovsx:
auth:
provider: azure openvsx/server/src/main/java/org/eclipse/openvsx/UserAPI.java Lines 75 to 80 in 1abe659
|
Using the latest commit I can now assert that no more code changes are required to configure a custom provider (following our use cases) as I was able to completely configure authentication to Azure using only the server spring config. I initially forgot to include a few modifications... |
@paul-marechal Just to give an early feedback. I compared your changes with the modifications I did on my local branch I was thinking of contributing and it's more or less the same. One thing I wonder is, do you use the githubToken to store the token in the database (UserData)? The TokenService also contains a switch block with the providers. |
I didn't find it required to store anything in |
Replace the references to
OAuth2User
byAuthUser
. This allows downstream extenders to more easily contribute alternative OAuth2 providers: If the expected data is stored in different attributes it will be possible to bridge it by implementing the properAuthUser
.