From b01f7d5bafee2f2f7e362ec1d3c7fd21c0093f2a Mon Sep 17 00:00:00 2001 From: Deon George Date: Thu, 13 Mar 2025 23:23:56 +1100 Subject: [PATCH] Attribute cleanup and optimisation in preparation to support attribute tags, HomeController return casting --- app/Classes/LDAP/Attribute.php | 67 +++++-------------- app/Classes/LDAP/Attribute/Binary.php | 2 +- app/Classes/LDAP/Attribute/Internal.php | 2 +- .../LDAP/Attribute/KrbPrincipalKey.php | 2 +- app/Classes/LDAP/Attribute/KrbTicketFlags.php | 2 +- app/Classes/LDAP/Attribute/Password.php | 2 +- app/Http/Controllers/HomeController.php | 65 +++++++++--------- app/Ldap/Entry.php | 26 +++---- app/Traits/MD5Updates.php | 4 +- .../attribute/binary/jpegphoto.blade.php | 2 +- .../views/components/attribute/rdn.blade.php | 2 +- resources/views/update.blade.php | 4 +- 12 files changed, 77 insertions(+), 103 deletions(-) diff --git a/app/Classes/LDAP/Attribute.php b/app/Classes/LDAP/Attribute.php index a2cecbae..225f99a1 100644 --- a/app/Classes/LDAP/Attribute.php +++ b/app/Classes/LDAP/Attribute.php @@ -17,31 +17,27 @@ class Attribute implements \Countable, \ArrayAccess, \Iterator protected string $name; private int $counter = 0; - protected ?AttributeType $schema = NULL; - - /* - # Source of this attribute definition - protected $source; - */ - - // Current and Old Values - protected Collection $values; - // Is this attribute an internal attribute - protected bool $is_internal = FALSE; + protected(set) bool $is_internal = FALSE; // Is this attribute the RDN? - protected bool $is_rdn = FALSE; + public bool $is_rdn = FALSE; // MIN/MAX number of values - protected int $min_values_count = 0; - protected int $max_values_count = 0; + protected(set) int $min_values_count = 0; + protected(set) int $max_values_count = 0; // RFC3866 Language Tags + /* @deprecated use $values/$values_old when playing with language tags */ protected Collection $lang_tags; + // The schema's representation of this attribute + protected(set) ?AttributeType $schema; + // The old values for this attribute - helps with isDirty() to determine if there is an update pending - protected Collection $oldValues; + protected(set) Collection $values_old; + // Current Values + public Collection $values; /* # Has the attribute been modified @@ -97,9 +93,10 @@ class Attribute implements \Countable, \ArrayAccess, \Iterator public function __construct(string $name,array $values) { $this->name = $name; - $this->values = collect($values); + $this->values_old = collect($values); + + $this->values = collect(); $this->lang_tags = collect(); - $this->oldValues = collect($values); $this->schema = (new Server) ->schema('attributetypes',$name); @@ -132,18 +129,10 @@ class Attribute implements \Countable, \ArrayAccess, \Iterator 'hints' => $this->hints(), // Can this attribute be edited 'is_editable' => $this->schema ? $this->schema->{$key} : NULL, - // Is this an internal attribute - 'is_internal' => isset($this->{$key}) && $this->{$key}, - // Is this attribute the RDN - 'is_rdn' => $this->is_rdn, // We prefer the name as per the schema if it exists 'name' => $this->schema ? $this->schema->{$key} : $this->{$key}, // Attribute name in lower case 'name_lc' => strtolower($this->name), - // Old Values - 'old_values' => $this->oldValues, - // Attribute values - 'values' => $this->values, // Required by Object Classes 'required_by' => $this->schema?->required_by_object_classes ?: collect(), // Used in Object Classes @@ -153,17 +142,6 @@ class Attribute implements \Countable, \ArrayAccess, \Iterator }; } - public function __set(string $key,mixed $values): void - { - switch ($key) { - case 'value': - $this->values = collect($values); - break; - - default: - } - } - public function __toString(): string { return $this->name; @@ -260,13 +238,8 @@ class Attribute implements \Countable, \ArrayAccess, \Iterator */ public function isDirty(): bool { - return ($this->oldValues->count() !== $this->values->count()) - || ($this->values->diff($this->oldValues)->count() !== 0); - } - - public function oldValues(array $array): void - { - $this->oldValues = collect($array); + return ($this->values_old->count() !== $this->values->count()) + || ($this->values->diff($this->values_old)->count() !== 0); } /** @@ -292,7 +265,7 @@ class Attribute implements \Countable, \ArrayAccess, \Iterator public function render_item_old(int $key): ?string { - return Arr::get($this->old_values,$key); + return Arr::get($this->values_old,$key); } public function render_item_new(int $key): ?string @@ -306,14 +279,10 @@ class Attribute implements \Countable, \ArrayAccess, \Iterator * @param string $tag * @param array $value * @return void + * @deprecated */ public function setLangTag(string $tag,array $value): void { $this->lang_tags->put($tag,$value); } - - public function setRDN(): void - { - $this->is_rdn = TRUE; - } } \ No newline at end of file diff --git a/app/Classes/LDAP/Attribute/Binary.php b/app/Classes/LDAP/Attribute/Binary.php index 4f84e14d..56be884c 100644 --- a/app/Classes/LDAP/Attribute/Binary.php +++ b/app/Classes/LDAP/Attribute/Binary.php @@ -7,6 +7,6 @@ use App\Classes\LDAP\Attribute; /** * Represents an attribute whose values are binary */ -class Binary extends Attribute +abstract class Binary extends Attribute { } \ No newline at end of file diff --git a/app/Classes/LDAP/Attribute/Internal.php b/app/Classes/LDAP/Attribute/Internal.php index a2848c31..bdfddcf5 100644 --- a/app/Classes/LDAP/Attribute/Internal.php +++ b/app/Classes/LDAP/Attribute/Internal.php @@ -11,7 +11,7 @@ use App\Classes\LDAP\Attribute; */ abstract class Internal extends Attribute { - protected bool $is_internal = TRUE; + protected(set) bool $is_internal = TRUE; public function render(bool $edit=FALSE,bool $old=FALSE,bool $new=FALSE): View { diff --git a/app/Classes/LDAP/Attribute/KrbPrincipalKey.php b/app/Classes/LDAP/Attribute/KrbPrincipalKey.php index 8d00fd2c..a68acb2d 100644 --- a/app/Classes/LDAP/Attribute/KrbPrincipalKey.php +++ b/app/Classes/LDAP/Attribute/KrbPrincipalKey.php @@ -26,7 +26,7 @@ final class KrbPrincipalKey extends Attribute public function render_item_old(int $key): ?string { - $pw = Arr::get($this->oldValues,$key); + $pw = Arr::get($this->values_old,$key); return $pw ? str_repeat('*',16) : NULL; diff --git a/app/Classes/LDAP/Attribute/KrbTicketFlags.php b/app/Classes/LDAP/Attribute/KrbTicketFlags.php index 0ed9386c..e52a0bbb 100644 --- a/app/Classes/LDAP/Attribute/KrbTicketFlags.php +++ b/app/Classes/LDAP/Attribute/KrbTicketFlags.php @@ -11,7 +11,7 @@ use Illuminate\Support\Collection; * Represents an attribute whose value is a Kerberos Ticket Flag * See RFC4120 */ -class KrbTicketFlags extends Attribute +final class KrbTicketFlags extends Attribute { private const DISALLOW_POSTDATED = 0x00000001; private const DISALLOW_FORWARDABLE = 0x00000002; diff --git a/app/Classes/LDAP/Attribute/Password.php b/app/Classes/LDAP/Attribute/Password.php index ea4b8a1c..d66af252 100644 --- a/app/Classes/LDAP/Attribute/Password.php +++ b/app/Classes/LDAP/Attribute/Password.php @@ -87,7 +87,7 @@ final class Password extends Attribute public function render_item_old(int $key): ?string { - $pw = Arr::get($this->oldValues,$key); + $pw = Arr::get($this->values_old,$key); return $pw ? (((($x=$this->hash($pw)) && ($x::id() !== '*clear*')) ? sprintf('{%s}',$x::shortid()) : '').str_repeat('*',16)) : NULL; diff --git a/app/Http/Controllers/HomeController.php b/app/Http/Controllers/HomeController.php index 3e7a756a..4bb2d7ab 100644 --- a/app/Http/Controllers/HomeController.php +++ b/app/Http/Controllers/HomeController.php @@ -2,7 +2,6 @@ namespace App\Http\Controllers; -use Illuminate\Contracts\View\View; use Illuminate\Http\Request; use Illuminate\Support\Arr; use Illuminate\Support\Collection; @@ -44,8 +43,6 @@ class HomeController extends Controller /** * Debug Page - * - * @return \Illuminate\Contracts\Foundation\Application|\Illuminate\Contracts\View\Factory|\Illuminate\Contracts\View\View */ public function debug() { @@ -56,10 +53,10 @@ class HomeController extends Controller * Create a new object in the LDAP server * * @param EntryAddRequest $request - * @return \Illuminate\Contracts\Foundation\Application|\Illuminate\Contracts\View\Factory|\Illuminate\Contracts\View\View + * @return View * @throws InvalidUsage */ - public function entry_add(EntryAddRequest $request) + public function entry_add(EntryAddRequest $request): \Illuminate\View\View { if (! old('step',$request->validated('step'))) abort(404); @@ -92,15 +89,15 @@ class HomeController extends Controller * * @param Request $request * @param string $id - * @return \Closure|\Illuminate\Contracts\View\View|string + * @return \Illuminate\View\View */ - public function entry_attr_add(Request $request,string $id): string + public function entry_attr_add(Request $request,string $id): \Illuminate\View\View { $xx = new \stdClass; $xx->index = 0; $x = $request->noheader - ? (string)view(sprintf('components.attribute.widget.%s',$id)) + ? view(sprintf('components.attribute.widget.%s',$id)) ->with('o',Factory::create($id,[])) ->with('value',$request->value) ->with('loop',$xx) @@ -109,7 +106,7 @@ class HomeController extends Controller return $x; } - public function entry_create(EntryAddRequest $request) + public function entry_create(EntryAddRequest $request): \Illuminate\Http\RedirectResponse { $key = $this->request_key($request,collect(old())); @@ -156,7 +153,7 @@ class HomeController extends Controller ->withFragment($o->getDNSecure()); } - public function entry_delete(Request $request) + public function entry_delete(Request $request): \Illuminate\Http\RedirectResponse { $dn = Crypt::decryptString($request->dn); @@ -196,7 +193,7 @@ class HomeController extends Controller ->with('success',[sprintf('%s: %s',__('Deleted'),$dn)]); } - public function entry_export(Request $request,string $id) + public function entry_export(Request $request,string $id): \Illuminate\View\View { $dn = Crypt::decryptString($id); @@ -215,10 +212,10 @@ class HomeController extends Controller /** * Render an available list of objectclasses for an Entry * - * @param string $id - * @return mixed + * @param Request $request + * @return Collection */ - public function entry_objectclass_add(Request $request) + public function entry_objectclass_add(Request $request): Collection { $oc = Factory::create('objectclass',$request->oc); @@ -242,7 +239,7 @@ class HomeController extends Controller ]); } - public function entry_password_check(Request $request) + public function entry_password_check(Request $request): Collection { $dn = Crypt::decryptString($request->dn); $o = config('server')->fetch($dn); @@ -265,10 +262,10 @@ class HomeController extends Controller * Show a confirmation to update a DN * * @param EntryRequest $request - * @return \Illuminate\Contracts\Foundation\Application|\Illuminate\Contracts\View\Factory|\Illuminate\Contracts\View\View|\Illuminate\Foundation\Application|\Illuminate\Http\RedirectResponse + * @return \Illuminate\Http\RedirectResponse|\Illuminate\View\View * @throws ObjectNotFoundException */ - public function entry_pending_update(EntryRequest $request) + public function entry_pending_update(EntryRequest $request): \Illuminate\Http\RedirectResponse|\Illuminate\View\View { $dn = Crypt::decryptString($request->dn); @@ -277,6 +274,8 @@ class HomeController extends Controller foreach ($request->except(['_token','dn','userpassword_hash','userpassword']) as $key => $value) $o->{$key} = array_filter($value,fn($item)=>! is_null($item)); + // @todo Need to handle incoming attributes that were modified by MD5Updates Trait (eg: jpegphoto) + // We need to process and encrypt the password if ($request->userpassword) { $passwords = []; @@ -312,8 +311,10 @@ class HomeController extends Controller * @param EntryRequest $request * @return \Illuminate\Http\RedirectResponse * @throws ObjectNotFoundException + * @todo When removing an attribute value, from a multi-value attribute, we have a ghost record showing after the update + * @todo Need to check when removing a single attribute value, do we have a ghost as well? Might be because we are redirecting with input? */ - public function entry_update(EntryRequest $request) + public function entry_update(EntryRequest $request): \Illuminate\Http\RedirectResponse { $dn = Crypt::decryptString($request->dn); @@ -368,9 +369,9 @@ class HomeController extends Controller * * @param Request $request * @param Collection|null $old - * @return \Illuminate\Contracts\Foundation\Application|\Illuminate\Contracts\View\Factory|View + * @return \Illuminate\View\View */ - public function frame(Request $request,?Collection $old=NULL): View + public function frame(Request $request,?Collection $old=NULL): \Illuminate\View\View { // If our index was not render from a root url, then redirect to it if (($request->root().'/' !== url()->previous()) && $request->method() === 'POST') @@ -385,7 +386,9 @@ class HomeController extends Controller // If we are rendering a DN, rebuild our object $o = config('server')->fetch($key['dn']); - foreach (collect(old())->except(['dn','_token']) as $attr => $value) + + // @todo We need to dynamically exclude request items, so we dont need to add them here + foreach (collect(old())->except(['dn','_token','userpassword_hash']) as $attr => $value) $o->{$attr} = $value; return match ($key['cmd']) { @@ -407,7 +410,7 @@ class HomeController extends Controller /** * This is the main page render function */ - public function home(Request $request) + public function home(Request $request): \Illuminate\View\View { // Did we come here as a result of a redirect return count(old()) @@ -421,11 +424,11 @@ class HomeController extends Controller * * @param ImportRequest $request * @param string $type - * @return \Illuminate\Contracts\Foundation\Application|\Illuminate\Contracts\View\Factory|\Illuminate\Contracts\View\View|\Illuminate\Foundation\Application + * @return \Illuminate\View\View * @throws GeneralException * @throws VersionException */ - public function import(ImportRequest $request,string $type) + public function import(ImportRequest $request,string $type): \Illuminate\View\View { switch ($type) { case 'ldif': @@ -461,9 +464,9 @@ class HomeController extends Controller /** * LDAP Server INFO * - * @return \Illuminate\Contracts\Foundation\Application|\Illuminate\Contracts\View\Factory|\Illuminate\Contracts\View\View + * @return \Illuminate\View\View */ - public function info() + public function info(): \Illuminate\View\View { return view('frames.info') ->with('s',config('server')); @@ -507,10 +510,10 @@ class HomeController extends Controller * * @note Our route will validate that types are valid. * @param Request $request - * @return \Illuminate\Contracts\Foundation\Application|\Illuminate\Contracts\View\Factory|\Illuminate\Contracts\View\View + * @return \Illuminate\View\View * @throws InvalidUsage */ - public function schema_frame(Request $request) + public function schema_frame(Request $request): \Illuminate\View\View { // If an invalid key, we'll 404 if ($request->type && $request->key && (! config('server')->schema($request->type)->has($request->key))) @@ -536,9 +539,9 @@ class HomeController extends Controller * Return the image for the logged in user or anonymous * * @param Request $request - * @return mixed + * @return \Illuminate\Http\Response */ - public function user_image(Request $request) + public function user_image(Request $request): \Illuminate\Http\Response { $image = NULL; $content = NULL; @@ -556,4 +559,4 @@ class HomeController extends Controller return response($image) ->header('Content-Type',$content); } -} +} \ No newline at end of file diff --git a/app/Ldap/Entry.php b/app/Ldap/Entry.php index da54cb17..212504a5 100644 --- a/app/Ldap/Entry.php +++ b/app/Ldap/Entry.php @@ -97,7 +97,7 @@ class Entry extends Model if ((! $this->objects->get($key)) && $value) $this->objects->put($key,Factory::create($key,[])); - $this->objects->get($key)->value = $this->attributes[$key]; + $this->objects->get($key)->values = collect($this->attributes[$key]); return $this; } @@ -155,7 +155,10 @@ class Entry extends Model $x->addValue($value); } else { - $this->objects->put($key,Attribute\Factory::create($key,Arr::wrap($value))); + $o = Attribute\Factory::create($key,[]); + $o->values = collect(Arr::wrap($value)); + + $this->objects->put($key,$o); } } @@ -164,31 +167,30 @@ class Entry extends Model * * @return Collection */ - public function getAttributesAsObjects(): Collection + private function getAttributesAsObjects(): Collection { $result = collect(); - foreach ($this->attributes as $attribute => $value) { - // If the attribute name has language tags + foreach ($this->attributes as $attribute => $values) { + // If the attribute name has tags $matches = []; if (preg_match('/^([a-zA-Z]+)(;([a-zA-Z-;]+))+/',$attribute,$matches)) { $attribute = $matches[1]; // If the attribute doesnt exist we'll create it - $o = Arr::get($result,$attribute,Factory::create($attribute,[])); - $o->setLangTag($matches[3],$value); + $o = Arr::get($result,$attribute,Factory::create($attribute,Arr::get($this->original,$attribute,[]))); + $o->setLangTag($matches[3],$values); } else { - $o = Factory::create($attribute,$value); + $o = Factory::create($attribute,Arr::get($this->original,$attribute,[])); } if (! $result->has($attribute)) { // Set the rdn flag - if (preg_match('/^'.$attribute.'=/i',$this->dn)) - $o->setRDN(); + $o->is_rdn = preg_match('/^'.$attribute.'=/i',$this->dn); - // Store our original value to know if this attribute has changed - $o->oldValues(Arr::get($this->original,$attribute,[])); + // Store our new values to know if this attribute has changed + $o->values = collect($values); $result->put($attribute,$o); } diff --git a/app/Traits/MD5Updates.php b/app/Traits/MD5Updates.php index 2e9a413f..522619ba 100644 --- a/app/Traits/MD5Updates.php +++ b/app/Traits/MD5Updates.php @@ -11,8 +11,8 @@ trait MD5Updates { public function isDirty(): bool { - foreach ($this->values->diff($this->oldValues) as $key => $value) - if (md5(Arr::get($this->oldValues,$key)) !== $value) + foreach ($this->values->diff($this->values_old) as $key => $value) + if (md5(Arr::get($this->values_old,$key)) !== $value) return TRUE; return FALSE; diff --git a/resources/views/components/attribute/binary/jpegphoto.blade.php b/resources/views/components/attribute/binary/jpegphoto.blade.php index e23c01eb..60d2e99e 100644 --- a/resources/views/components/attribute/binary/jpegphoto.blade.php +++ b/resources/views/components/attribute/binary/jpegphoto.blade.php @@ -2,7 +2,7 @@ - @foreach (($old ? $o->old_values : $o->values) as $value) + @foreach (($old ? $o->values_old : $o->values) as $value) @switch ($x=$f->buffer($value,FILEINFO_MIME_TYPE)) @case('image/jpeg') diff --git a/resources/views/components/attribute/rdn.blade.php b/resources/views/components/attribute/rdn.blade.php index d08a2218..961f7de9 100644 --- a/resources/views/components/attribute/rdn.blade.php +++ b/resources/views/components/attribute/rdn.blade.php @@ -1,6 +1,6 @@ - @foreach($o->values as $value) + @foreach(($o->values->count() ? $o->values : ['']) as $value) @if($edit)
- @foreach ($o->getAttributesAsObjects()->filter(fn($item)=>$item->isDirty()) as $key => $oo) + @foreach ($o->getObjects()->filter(fn($item)=>$item->isDirty()) as $key => $oo) - @for($xx=0;$xx<$x;$xx++)
+ {{ $oo->name }}