Replace MAX_BYTE_SIZE constant with setting

[gitlab-djensen]

Status

READY

Migrations

NO

Description

The MAX_BYTE_SIZE constant did not allow for customization, which
is necessary for cases where legitimate SAML responses are larger
than 250,000 bytes. This replaces the constant with a setting, which
has a default value of 250,000 bytes, but can be customized like
any other setting.

This was motivated by #598, which was motivated by https://gitlab.com/gitlab-org/gitlab/-/issues/329053.
This follows the existing pattern used for encode_raw_saml.

Related PRs

List related PRs against other branches:

branch	PR
Introduce MAX_BYTE_SIZE	link
Introduce inflate option to disable SAML message inflation	link

Todos

• Tests
• Documentation

Deploy Notes

Nothing noteworthy.

Steps to Test or Reproduce

Outline the steps to test or reproduce the PR here.

git fetch
git checkout gitlab-djensen:gitlab-djensen-use-setting-for-max-byte-size
bundle install;
ruby -Itest test/settings_test.rb

Impacted Areas in Application

List general components of the application that this PR will affect:

• Settings
• SamlMessage

[gitlab-djensen]

@dblessing would you mind doing the initial review on this whenever you have some time?

P.S. I wasn't able to get the tests to run because of dependency problems on my machine. If you could confirm you're seeing green on the tests that would be excellent.

[dblessing]

@gitlab-djensen Thanks for opening the PR. Looks good to me, with my limited knowledge of the ruby-saml code. Let's see what the maintainers think.

[gitlab-djensen]

@pitbulk this PR is ready for your review, whenever you might have some time.

GitLab is hoping you will release a new version of this gem containing this fix, because MAX_BYTE_SIZE was a breaking change for implementations with large messages (example).

[johnnyshields]

👍 looks good to merge

[gitlab-djensen]

@pitbulk friendly ping, hopefully this is an easy review 🙂

[gitlab-djensen]

@pitbulk thanks for reviewing! I just marked both of your comments as resolved. Should we run the workflow next? (It's telling me "First-time contributors need a maintainer to approve running workflows.")

[gitlab-djensen]

Strangely, the workflow ran anyway. There were a bunch of failures because settings was still optional in some places where it is now required. I just pushed 2 commits that make settings a required argument in multiple places, which makes this PR a breaking change. (We could avoid the breaking change by leaving settings as an optional argument and calling OneLogin::RubySaml::Settings.new when settings is not provided. But that approach is not currently used anywhere in the gem, so it would be a new pattern.) I'll keep working on this until the tests are green.

[pitbulk]

We need to avoid breaking changes. If settings is not provided to the decode_raw_saml method, makes sense to keep using the default value, otherwise, the settings object should be there.

I don't want to force settings parameters on main constructors.

[gitlab-djensen]

@pitbulk gotcha, refactored to be non-breaking (by calling OneLogin::RubySaml::Settings.new when settings is not passed). Tests are green. How's it looking now?

[johnnyshields]

@pitbulk this looks good to me.

[gitlab-djensen]

@pitbulk friendly ping, prompted by a colleague with an affected client. Does this look good for merge?

[johnnyshields]

Also, after merge, let's get an gem release please :) Lots of great improvements recently.

[pitbulk]

Sorry I will be back from PTO on 6 and take care of all the pending PRs