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

WIP for testing: SAML Config Refactor #2953

Merged
merged 9 commits into from
Aug 9, 2018
Merged

Conversation

lcatlett
Copy link
Contributor

Fixes #2950 #2949

Changes proposed:

  • WIP for testing: Refactors acquia config to remove problematic functions in favor dynmically generating base config

@ba66e77
Copy link
Contributor

ba66e77 commented Jul 16, 2018

Is this going to subsume any of the other SAML PR's?

@mikemadison13
Copy link
Contributor

@ba66e77 i think if this one goes in we will no longer need:
#2949
#2948

@mikemadison13
Copy link
Contributor

#2924 is still needed i believe

$config['certdir'] = "/var/www/simplesamlphp/cert/";
$config['metadatadir'] = "/var/www/simplesamlphp/metadata";
$config['baseurlpath'] = 'simplesaml/';
$config['loggingdir'] = '/var/www/simplesamlphp/log/';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

there are 2 } here at lines 68/69 and only 1 is needed (fatal)

$cached_id = '';
}

return $cached_id;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

there are 3 } here and only 1 is needed (fatal)

@lcatlett
Copy link
Contributor Author

lcatlett commented Jul 18, 2018

closed in error - can you clarify why #2924 is still needed @mikemadison13?

@lcatlett lcatlett closed this Jul 18, 2018
@lcatlett lcatlett reopened this Jul 18, 2018
@mikemadison13
Copy link
Contributor

@lcatlett looking again, it's not i don't think. looks like you covered the multi-site stuff in this. sorry!

@lcatlett
Copy link
Contributor Author

@ba66e77 This PR should subsume any other SAML config PRs.

$creds_json = file_get_contents('/var/www/site-php/' . getenv('AH_SITE_NAME') . '/creds.json');
elseif (getenv('AH_SITE_ENVIRONMENT')) {
// Set ACE ad ACSF sites based on hosting database and site name.
$config['certdir'] = "/mnt/www/html/{$_ENV['AH_SITE_GROUP']}.{$_ENV['AH_SITE_ENVIRONMENT']}/simplesamlphp/cert/";
Copy link
Contributor

Choose a reason for hiding this comment

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

/mnt/www/html/{$_ENV['AH_SITE_GROUP']}.{$_ENV['AH_SITE_ENVIRONMENT']}/simplesamlphp/ is pointing to the simplesamlphp symlink in the docroot from symlinkDocrootToLibDir() right? Using a relative path works and would make non-environment specific, i.e. $config['certdir'] = 'cert/';. Same for $config['metadatadir'] on the next line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rjgwiz I believe the simplesamlphp_auth module has a note that it requires absolute paths for those files - have you found that not to be the case?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lcatlett yea - in our project we have been using relative path for over a year. Currently on simplesamlphp v1.15.4

@ba66e77
Copy link
Contributor

ba66e77 commented Jul 31, 2018

@mikemadison13, you had requested changes on this PR. Have they all been addressed at this point?

@mikemadison13
Copy link
Contributor

@ba66e77 yes i think we are good now!

@ba66e77 ba66e77 merged commit 312d471 into acquia:9.x Aug 9, 2018
danepowell added a commit that referenced this pull request Aug 24, 2018
danepowell added a commit that referenced this pull request Aug 24, 2018
* Revert "Updating CHANGELOG.md and setting version for 9.1.2."

This reverts commit 5e52325.

* Revert "Minor code review docs update (#3013)"

This reverts commit 25e3887.

* Revert "SAML Config Refactor (#2953)"

This reverts commit 312d471.

* Revert "Improve template/README.md (#3004)"

This reverts commit 316967d.

* Revert "Multisite setup enhancements and bugfixes (#2997)"

This reverts commit d0da339.
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.

4 participants