-
Notifications
You must be signed in to change notification settings - Fork 401
Use config array instead of object for Auth plugins #5620
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
base: dev
Are you sure you want to change the base?
Changes from all commits
71379da
5c0a492
de19999
8980019
242c25a
7e21c77
1a65b99
1d8746b
d356131
28cf6b4
24f412d
af668be
9abba25
f165d99
68e998c
e81a514
f519c7e
ed4e014
8775658
e8e53a2
69c0c60
44cb3de
3d01265
02510d8
2ffc761
87338ac
6a3eba5
170e81e
d122970
0dd88d7
4d7f3e4
c33c539
d949534
41ef080
960caa4
50f8329
441f7da
d01cbb7
861a889
3b97a93
7c7546f
67f63f8
101a171
efc5f44
576ac47
ccea50b
305b4eb
25a4688
488570d
43622d2
189323d
d61fd64
6407fde
d98e35c
c5797d9
872e4c2
9d4a7d6
fd4430a
6af3049
1a62470
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -70,7 +70,7 @@ abstract class AbstractBase implements | |||||||
| /** | ||||||||
| * Configuration settings. | ||||||||
| * | ||||||||
| * @var \VuFind\Config\Config | ||||||||
| * @var array | ||||||||
| */ | ||||||||
| protected $config = null; | ||||||||
|
|
||||||||
|
|
@@ -101,7 +101,7 @@ abstract class AbstractBase implements | |||||||
| * exception if the configuration is invalid. | ||||||||
| * | ||||||||
| * @throws AuthException | ||||||||
| * @return \VuFind\Config\Config | ||||||||
| * @return array | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should also be nullable to match the property.
Suggested change
|
||||||||
| */ | ||||||||
| public function getConfig() | ||||||||
| { | ||||||||
|
|
@@ -145,7 +145,7 @@ public function clearLoginState() | |||||||
| /** | ||||||||
| * Set configuration. | ||||||||
| * | ||||||||
| * @param \VuFind\Config\Config $config Configuration to set | ||||||||
| * @param array $config Configuration to set | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably makes sense to make this consistent with the getter.
Suggested change
|
||||||||
| * | ||||||||
| * @return void | ||||||||
| */ | ||||||||
|
|
@@ -450,9 +450,8 @@ public function getPolicyConfig(string $type): array | |||||||
| { | ||||||||
| $policy = []; | ||||||||
| $config = $this->getConfig(); | ||||||||
| $authConfig = isset($config->Authentication) | ||||||||
| ? $config->Authentication->toArray() | ||||||||
| : []; | ||||||||
| $authConfig = $config['Authentication'] | ||||||||
| ?? []; | ||||||||
|
Comment on lines
+453
to
+454
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This fits on a single line now:
Suggested change
|
||||||||
| /* Map settings to the policy array, e.g.: | ||||||||
| * | ||||||||
| * password_minimum_length or username_minimum_length => minLength | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -88,7 +88,7 @@ public function create($request) | |||||
| { | ||||||
| // When in privacy mode, don't create an Alma account and delegate | ||||||
| // further code execution to the parent. | ||||||
| if ($this->getConfig()->Authentication->privacy) { | ||||||
| if ($this->getConfig()['Authentication']['privacy']) { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need a fallback for safety here:
Suggested change
|
||||||
| return parent::create($request); | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -71,7 +71,7 @@ public function clearLoginState(); | |||||
| /** | ||||||
| * Set configuration. | ||||||
| * | ||||||
| * @param \VuFind\Config\Config $config Configuration to set | ||||||
| * @param array $config Configuration to set | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to make this consistent with AbstractBase:
Suggested change
|
||||||
| * | ||||||
| * @return void | ||||||
| */ | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -107,8 +107,8 @@ public function __construct(\Laminas\Session\Container $container) | |||||||
| protected function validateConfig() | ||||||||
| { | ||||||||
| if ( | ||||||||
| !isset($this->config->ChoiceAuth->choice_order) | ||||||||
| || !strlen($this->config->ChoiceAuth->choice_order) | ||||||||
| !isset($this->config['ChoiceAuth']['choice_order']) | ||||||||
| || !strlen($this->config['ChoiceAuth']['choice_order']) | ||||||||
|
Comment on lines
+110
to
+111
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be simplified:
Suggested change
|
||||||||
| ) { | ||||||||
| throw new AuthException( | ||||||||
| 'One or more ChoiceAuth parameters are missing. ' . | ||||||||
|
|
@@ -120,7 +120,7 @@ protected function validateConfig() | |||||||
| /** | ||||||||
| * Set configuration; throw an exception if it is invalid. | ||||||||
| * | ||||||||
| * @param \VuFind\Config\Config $config Configuration to set | ||||||||
| * @param $config Configuration to set | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Type got lost here:
Suggested change
|
||||||||
| * | ||||||||
| * @throws AuthException | ||||||||
| * @return void | ||||||||
|
|
@@ -130,7 +130,7 @@ public function setConfig($config) | |||||||
| parent::setConfig($config); | ||||||||
| $this->strategies = array_map( | ||||||||
| 'trim', | ||||||||
| explode(',', $this->getConfig()->ChoiceAuth->choice_order) | ||||||||
| explode(',', $this->getConfig()['ChoiceAuth']['choice_order']) | ||||||||
| ); | ||||||||
| } | ||||||||
|
|
||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,7 +35,6 @@ | |
| use Lmc\Rbac\Identity\IdentityInterface; | ||
| use Lmc\Rbac\Mvc\Identity\IdentityProviderInterface; | ||
| use Psr\Log\LoggerAwareInterface; | ||
| use VuFind\Config\Config; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is still being used on line 206 -- that's the reason the tests are failing. |
||
| use VuFind\Cookie\CookieManager; | ||
| use VuFind\Db\Entity\UserEntityInterface; | ||
| use VuFind\Db\Service\AuditEventServiceInterface; | ||
|
|
@@ -204,7 +203,7 @@ protected function makeAuth(string $method): AuthInterface | |
| throw new \Exception("Illegal authentication method: $method"); | ||
| } | ||
| $auth = $this->pluginManager->get($method); | ||
| $auth->setConfig(new Config($this->config)); | ||
| $auth->setConfig($this->config); | ||
| return $auth; | ||
| } | ||
|
|
||
|
|
||
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.
This needs to be nullable since it's initialized to null.