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

Refactor authentication provider JSON configuration #6694

Open
poikilotherm opened this issue Feb 26, 2020 · 4 comments
Open

Refactor authentication provider JSON configuration #6694

poikilotherm opened this issue Feb 26, 2020 · 4 comments
Labels

Comments

@poikilotherm
Copy link
Contributor

poikilotherm commented Feb 26, 2020

This is related to #6515, #6679 and of course #5974.

To configure an authentication provider in Dataverse, you need to send a JSON document to the Admin API. Example for OAuth2.

Within the document, the real options for your provider are encoded in a string:

"factoryData":"type: github | userEndpoint: NONE | clientId: FIXME | clientSecret: FIXME",

This is not very convenient in terms of validation, readability and extending. With upcoming changes to provide more configuration options for an authentication provider like what to override, what can be trusted etc etc, this should be changed to sth. more sophisticated.

The different providers are also very inconsistent in what you can do with them. For example, OAuth2 providers can have a customizable title, while the Shibboleth provider cannot.

I propose to switch to something like this:

{
    "id":"github",
    "type":"github",
    "enabled":true,
    "title": "GitHub",
    "description": {
       "en_US": "This is Github.",
       "de_DE": "Das ist Github."
    },
    "logo": "<base64 encoded logo>",
    "options": {
       "client_id": "my-client-id",
       "client_secret": "my-client-secret"
    }
}

Please find a complete JSON Schema (in draft-07 notation) for details, unambiguity and precision in my gist. It can be used to play for example on https://jsonschemalint.com or any other draft-07 compliant editor/validator.

Notes:

  1. You might have noticed, that this includes a necessary change to AuthenticationServiceBean to change the concept of AuthenticationProviderFactory to provide a factory for every provider we offer instead of grouping the OAuth2 ones. The benefit is the reduced complexity and increased unambiguity.
  2. Also removed is the subtitle option. It has never been used and should be replaced with description. That could offer a better approach for things like those we do for ORCID, but in a more general fashion and more flexible.

This is most likely to trigger the UI team, as we might decide to actually implement that description and logosupport... 😉 Triggering @TaniaSchlatter @djbrooke @scolapasta

I think this is from a technical perspective a smaller issue. It requires some refactoring, but the auth flow is untouched. It's mostly about how the configuration flies in and which providers are built.

@pdurbin
Copy link
Member

pdurbin commented Feb 26, 2020

@poikilotherm yes, I've always felt factoryData is kind of gross with its pipes and such. A full JSON doc, as you've proposed, would be much cleaner. Awesome that you're thinking along the lines of a JSON schema. 🎉

I guess I have a few questions:

  • Why change the UI at all? Can we leave it alone? And just have this refactoring affect the backend?
  • Would you store the information in the database differently? I assume so. If so, would you write SQL upgrade script to migrate the old settings to the new format?
  • When is the right time to do this refactoring? Please consider the QA effort. 😄

@poikilotherm
Copy link
Contributor Author

Ad UI) Sure! We can simply leave it alone and change the UI at a later point in time. I just thought this would be the right moment to introduce this functionality. If you folks tend to leave that stuff out of the game, we shouldn't include it in the first place (so remove it from the schema) to avoid unsupported properties staying around again (like subtitle did).

Ad DB) These are some minor changes, but yes, I will of course append a Flyway script.

Ad Time) Well this is kind of a blocker for things like configurable provider options regarding trust etc. So this should be done first... Happy to assist with everything except Shibboleth (although that one is mostly untouched).

@djbrooke
Copy link
Contributor

djbrooke commented Mar 2, 2020

@poikilotherm it would be helpful to move this forward without an UI changes, especially if this is blocking other work. Thank you!

poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Mar 16, 2020
…onProviderConfigurationException as we remove the current factory pattern, remove invalid catch in Admin API. IQSS#6694
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Mar 16, 2020
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Mar 16, 2020
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Mar 16, 2020
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Mar 16, 2020
…n converter from Jackson to JSR 353 JSON-P world including tests. IQSS#6694
@poikilotherm poikilotherm added this to In Focus (go production) in Forschungszentrum Jülich Apr 3, 2020
@poikilotherm
Copy link
Contributor Author

poikilotherm commented Apr 3, 2020

Had a VC with @qqmyers and @scolapasta yesterday about this issue.

Summary:

  • Switch from Jackson to JSON-B from Java EE 8 / Jakarta EE 8, but keep using new tech. Dataverse 5 is on the horizon, so lets do this breaking change.
  • Don't do i18n in the configuration. Focus on Bundle files to include anything of this. Title, description etc is either a string in one language or a key that can be looked up.
  • Refactor the way how providers are loaded in one big chunk.
  • Go ahead with Testcontainers, might serve as an example to look at for further discussion.
  • @qqmyers mentioned that we should have a better process for decisions as a community to keep everyone informed, involved and engaged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants