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

Feature/make OIDC generic #15

Merged
merged 33 commits into from
Sep 29, 2021

Conversation

yoh1496
Copy link
Contributor

@yoh1496 yoh1496 commented Aug 17, 2021

This PR contains below features.

Generic OpenID Connect Plugin

  • Added OIDCTokenHandler class.

    • With OIDCTokenHandler class, you can parse id_token (JWT).
    • OIDCTokenHandler.createFromOIDCConfigurationURL loads IdP configuration from specified OpenID Connect Discovery 1.0 configuration endpoint ( https://openid.net/specs/openid-connect-registration-1_0.html ) and initialize OIDCTokenHandler.
    • This class tires to parse id_token with io.jsonwebtoken.
  • Added OIDCAuthPluginBase class.

    • This class is abstract. By inheriting and overriding this class, you can customize how AuthenticatedIdentity is generated and how AuthPlugin verifies passed client_id is trusted.
  • Added GenericOIDCAuthPlugin class.

    • Sub-class of OIDCAuthPluginBase.
  • Added OIDCPluginLoader class

    • This implements AuthPlugin interface.
    • Load properties file and initialize multiple instances.

Test codes

This PR contains some test codes.

  • Initializing OIDCTokenHandler test with OpenID Connect Discovery configuration URL mock.
  • Verifing valid id_token and invalid id_token.

Copy link
Member

@tochi-y tochi-y left a comment

Choose a reason for hiding this comment

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

@yoh1496 Thanks for your modification. I'm in the middle of a review, but I'd like to leave a comment. Action is not required.

// IdToken contains wrong signature
throw OidcPluginException.INVALID_ID_TOKEN.create("ID Token sig value is invalid");
} catch (Exception e) {
throw OidcPluginException.INVALID_ID_TOKEN.create(e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

Due to an existing OidcPluginException problem, the exception information is not chained here. We would like to deal with it in the future. I will create an issue.

Copy link
Member

@tochi-y tochi-y left a comment

Choose a reason for hiding this comment

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

@yoh1496 I added comments. Please check them, thanks.

@Override
public ArrayList<AuthPlugin> loadInstances() {
ArrayList<AuthPlugin> result = new ArrayList<AuthPlugin>();
Properties props = new Properties();
Copy link
Member

Choose a reason for hiding this comment

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

Can you extract a private method from line 53 to line 86? The following code is for example.

    private Properties loadConfigurationFile() {
        String configFilename = System.getProperty("io.personium.configurationFile",
                "personium-unit-config.properties");
        try {
            return loadFromClasspath(configFilename);
        } catch (IOException e) {
            log.info("IOException while loading: " + configFilename, e);
        }
        try {
            return loadFromLocalfile(configFilename);
        } catch (IOException e) {
            log.info("IOException while loading: " + configFilename, e);
        }
        log.info("Properties file cannot be loaded: " + configFilename);
    }

* Test that isProviderClientIdTrusted returns true if aud(client_id) in claims is trusted.
*/
@Test
public void isProviderClientIdTrusted_returns_true_if_aud_is_trusted_() {
Copy link
Member

Choose a reason for hiding this comment

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

Can you write the method name according to the content?

@tochi-y tochi-y merged commit 82793c9 into personium:develop Sep 29, 2021
Current Work automation moved this from In progress (today) to Done Sep 29, 2021
tochi-y added a commit that referenced this pull request Sep 29, 2021
@yoh1496
Copy link
Contributor Author

yoh1496 commented Sep 30, 2021

Thank you for merging.

By the way, can the argument key of Property.getProperty takes regular expression?

-                String accountNameKey = props.getProperty(propPrefix + ".accountNameKey", "username");
+                String accountNameKey = props.getProperty(propPrefix + "\\.accountNameKey$", "username");

You applied this change like above, is that correct?

@tochi-y
Copy link
Member

tochi-y commented Oct 1, 2021

@yoh1496 Thank you for pointing it out. I also noticed that mistake and corrected it with the next commit.
5f249d1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Current Work
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants