-
Notifications
You must be signed in to change notification settings - Fork 81
Move SAML configurations to a table of their own #558
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
Conversation
4fafd99 to
1e425e6
Compare
b72808d to
e5cb759
Compare
|
yeehaw, integration tests giving green lights :) |
e3db276 to
5375946
Compare
5375946 to
417876a
Compare
91a1a11 to
15a30b1
Compare
|
Tests are failing on clone 🤔 |
PVince81
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the POST/PUT potential mixup the rest is only minor comments.
Note: I only have very limited knowledge of this app and don't have an env to test it
| */ | ||
| class Version5000Date20211025124248 extends SimpleMigrationStep { | ||
|
|
||
| private const IDP_CONFIG_KEYS = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dejavu from SAMLSettings, or are we duplicating these on purpose to not class-load the other class before running the migration ? (please answer in a PHPDoc that clarifies if applicable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two lists are not exactly the same. lib/SAMLSettings.php contains more entries. I'm not sure if this is expected or not. @blizzz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is sort of duplicate for we cannot rely on new classes on migration as old ones may have been autoloaded. Also, when new keys are added to SAMLSettings, they are not required in the Migration class for they cannot have been added in the past. Therefore this snapshot is also reasonable.
@CarlSchwan thanks for pointing out the differences! They should be there indeed.
There is also an existing issue with security-sloWebServerDecode which is IdP-sensitive but was always read from the first config. So, tasks:
- fix security-sloWebServerDecode – set globally, but IdP sensitive
- migrate sp-x509cert, sp-name-id-format, sp-privateKey, security-sloWebServerDecode
- evaluate reading of sp-name-id-format in AdminSettings getForm (before, it took the first from config).
15a30b1 to
41cba52
Compare
|
Tested locally and for me the migration wasn't run :/ I'll see if I'm able to find there the bug is |
3c4ddaf to
f877a1f
Compare
Found the issue: providerIds was not in the oc_appconfig table for me. The UI worked correctly for me since in that case getAppConfig for it used the default value of 1, but the migration didn't handle this case. Fixed now :) |
f877a1f to
5c4eaca
Compare
CarlSchwan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into details in the code, fixed a few small issues and this looks fine to me now :)
|
Found one more small issue: this PR change the min-version from 21 to 22 because of the executeQuery/executeStatement. This changes should be either reverted or indicated inside the Changelog/appinfo.xml file |
9827528 to
370d7ab
Compare
see the comment above… |
- adds user_saml_configurations table and migrates existing configuration - Controller methods are added since appconfig endpoints cannot be used anymore. THIS IS A BREAKING CHANGE. - Frontend code is adjusted to use new endpoints. - security-sloWebServerDecode was changed from global to provider specific setting. It being global seemed to be unintended. A migration path is yet missing. Signed-off-by: Arthur Schiwon <[email protected]>
Signed-off-by: Arthur Schiwon <[email protected]>
Signed-off-by: Arthur Schiwon <[email protected]>
Signed-off-by: John Molakvoæ <[email protected]>
Signed-off-by: Carl Schwan <[email protected]>
- it tests only the first configuration, others were not taken into account - the configuration check is also only needed when SAML auth is actually happening Signed-off-by: Arthur Schiwon <[email protected]>
Signed-off-by: Arthur Schiwon <[email protected]>
Signed-off-by: Arthur Schiwon <[email protected]>
Signed-off-by: Arthur Schiwon <[email protected]>
Signed-off-by: Arthur Schiwon <[email protected]>
Signed-off-by: Arthur Schiwon <[email protected]>
- wrongly used way to set value attribute Signed-off-by: Arthur Schiwon <[email protected]>
Signed-off-by: Arthur Schiwon <[email protected]>
b6d57c5 to
e514fed
Compare
- Switch to new configuration setup, see nextcloud#558
- Switch to new configuration setup, see nextcloud#558 Signed-off-by: Giuliano Mele <[email protected]>
- Switch to new configuration setup, see #558 Signed-off-by: Giuliano Mele <[email protected]>
- Switch to new configuration setup, see nextcloud#558 Signed-off-by: Giuliano Mele <[email protected]>
- Switch to new configuration setup, see #558 Signed-off-by: Giuliano Mele <[email protected]>
- Switch to new configuration setup, see #558 Signed-off-by: Giuliano Mele <[email protected]>
- Switch to new configuration setup, see #558 Signed-off-by: Giuliano Mele <[email protected]>
- Switch to new configuration setup, see #558 Signed-off-by: Giuliano Mele <[email protected]>
- Switch to new configuration setup, see #558 Signed-off-by: Giuliano Mele <[email protected]>
- Switch to new configuration setup, see #558 Signed-off-by: Giuliano Mele <[email protected]>
- Switch to new configuration setup, see #558 Signed-off-by: Giuliano Mele <[email protected]>
- Switch to new configuration setup, see #558 Signed-off-by: Giuliano Mele <[email protected]>
- Switch to new configuration setup, see #558 Signed-off-by: Giuliano Mele <[email protected]>
anymore. THIS IS A BREAKING CHANGE.
setting. It being global seemed to be unintended.
set security-sloWebServerDecode to each configurationkeep as is, the setting had only effect on the first configuration. Doing anything else might result in different behaviour than before, this should be avoided.occ commands
reserves and returns an new ID for the new SAML configuration. Not necessary to use, but if useful if there is a unknown number of confiugrations. Example:
Deletes the config identified by the provided ID. No output on success.
Returns all configuraitons or the one optionally specified by the --providerId parameter. Support the usual output formats (plain, json or json_pretty as of now). Example:
Sets the specified configuration parameters for the given SAML provider. See
--helpfor a complete list of configuration parameters. On success, the output is empty. Example: