Skip to content

MODLOGSAML-199: Remove legacy token support#196

Open
steveellis wants to merge 7 commits intomasterfrom
MODLOGSAML-199-remove-legacy-token-support
Open

MODLOGSAML-199: Remove legacy token support#196
steveellis wants to merge 7 commits intomasterfrom
MODLOGSAML-199-remove-legacy-token-support

Conversation

@steveellis
Copy link
Copy Markdown
Contributor

https://folio-org.atlassian.net/browse/MODLOGSAML-199

  • Add @Deprecated to SamlConfiguration.useSecureTokens.
  • Remove all calls to legacy /token endpoint.
  • Let /callback and /callback-with-expiry when called return expiring token cookies (they should already).
  • Remove dependence on useSecureTokens because we no longer need a boolean flag to determine if legacy tokens should be used on the /callback endpoint (see MODLOGSAML-192 for background).
  • Remove IdpLegacyTest because it works only with the old tokens.
  • Rename mock_..._legacy.json in mock files to _callback etc since /callback isn't "legacy" or deprecated anymore.
  • Keep useSecureTokens in mocks since it is likely to be present for the time being in configuration.
  • Fix DAO tests since they grab config directly out JSON response. Use @JsonIgnore to keep from being written.
  • Keep calls to old /token endpoint in mocks but change response to 404.

- Made useSecureTokens @deprecated and removed get and set to prevent re-serialization
- Removed useSecureTokens from schema
- Removed many _legacy tests
- Forced /callback and /callback-with-expiry to return expiring tokens

TODO
- Remove legacy json data
- Find all references to /token and remove
…llback isn't legacy anymore.

Marking `useSecureTokens` as @JsonIgnore to fix DAO tests of new config and ensure that
property is no longer persisted to db config.
@steveellis steveellis changed the title MODLOGSAML-99: Remove legacy token support MODLOGSAML-199: Remove legacy token support Feb 18, 2025
Comment thread src/main/java/org/folio/rest/impl/SamlAPI.java
Comment thread src/test/resources/after_regenerate.json Outdated
Comment thread src/test/resources/mock_content_no_keystore.json Outdated
Comment thread src/test/resources/mock_multiple_user_tenant.json Outdated
Comment thread src/test/resources/mock_one_user_tenant.json Outdated
Comment thread src/test/resources/mock_tokenresponse.json
Comment thread descriptors/ModuleDescriptor-template.json
public void callbackEndpointTestDeprecatedUseSecureTokens() {
// Ensure that when the deprecated property useSecureTokens is present in the configuration, things
// still work as expected.
mock.setMockContent("mock_content_callback.json");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

After using an new mock configuration,
here mock.setMockContent("mock_content_callback.json"); ,
it is necessary to use
dataMigrationHelper.dataMigrationCompleted(vertx, context, false);
to overwrite the data in the database.
Otherwise the data of
public void setUp(TestContext context)
are used.

mock.setMockContent("mock_content_legacy.json");
public void putConfigurationWithCallback() {
mock.setMockContent("mock_content_callback.json");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

After using an new mock configuration,
here mock.setMockContent("mock_content_callback.json"); ,
it is necessary to use
dataMigrationHelper.dataMigrationCompleted(vertx, context, false);
to overwrite the data in the database.
Otherwise the data of
public void setUp(TestContext context)
are used.

Copy link
Copy Markdown
Contributor

@barbaraloehle barbaraloehle left a comment

Choose a reason for hiding this comment

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

After using an new mock configuration in the class SamlAPITest
the method
dataMigrationHelper.dataMigrationCompleted(vertx, context, false);
has to be added to overwrite the data in the database.

This has further to be applied to the methods:
public void putConfiguration()
--> public void putConfiguration(context)
and
private void testCallbackErrorCases(String callbackUrl, String relayState, ???
String cookie) -->
private void testCallbackErrorCases(String callbackUrl, String relayState, ???
String cookie, TestContext context)

@barbaraloehle
Copy link
Copy Markdown
Contributor

barbaraloehle commented Mar 7, 2025

In the description of MODLOGSAML-199 the following is mentioned :
"Use /saml/callback by default when enabling mod-login-saml for a tenant the first time."
How can this be garanteed?
Eeventually this could be garanteed by generating a configuration with "saml.callback": "callback" in the class TenantRefAPI using the method

  Future<Integer> loadData(TenantAttributes attributes, String tenantId,
      Map<String, String> headers, Context vertxContext) {
    log.info("TenantRefAPI::loadData");
    return super.loadData(attributes, tenantId, headers, vertxContext)
        .compose(result -> {
          **if (attributes.getModuleFrom() == null) {               
            return Future.succeededFuture(0);
          }**
          return configurationsMigration(headers, vertxContext);
        });
  }

@sonarqubecloud
Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

1 similar comment
@sonarqubecloud
Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

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