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.

Sign up for GitHub

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

Jump to bottom

[5.8] Added native support for attribute encryption #29663

Closed
wants to merge 1 commit into from
Closed

[5.8] Added native support for attribute encryption #29663

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 56 additions & 1 deletion src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,13 @@ trait HasAttributes
*/
protected $casts = [];

/**
* The attributes that should be encrypted and decrypted on get and set.
*
* @var array
*/
protected $encrypt = [];

/**
* The attributes that should be mutated to dates.
*
Expand Down Expand Up @@ -195,6 +202,10 @@ protected function addCastAttributesToArray(array $attributes, array $mutatedAtt
if ($attributes[$key] && $this->isCustomDateTimeCast($value)) {
$attributes[$key] = $attributes[$key]->format(explode(':', $value, 2)[1]);
}

if ($attributes[$key] instanceof Arrayable) {
$attributes[$key] = $attributes[$key]->toArray();
}
}

return $attributes;
Expand Down Expand Up @@ -280,6 +291,36 @@ protected function getArrayableRelations()
return $this->getArrayableItems($this->relations);
}

/**
* Determine whether an attribute will be encrypted.
*
* @return bool
*/
protected function hasEncryption($key)
{
return in_array($key, $this->encrypt);
}

/**
* Encrypt attribute's value.
*
* @return void
*/
public function encryptAttribute($key, $value)
{
$this->attributes[$key] = is_null($value) ? null : encrypt($value);
Copy link
Contributor

Choose a reason for hiding this comment

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

@JustCarty This will break when the illuminate/database package is used outside of a full Laravel framework application.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this the actual problem why previous attempts were not endorsed?

  • This would mean that illuminate/database would need a dependency to illuminate/encryption
  • you can't use encrypt() which calls app() which you also can't use here; you would need to directly access the container

Don't forget this package needs to work without Laravel, too.

Maybe a package "on top" would be better suited, say illuminate/database-encryption which depends on illuminate/database and illuminate/encryption and provides some trait or other means to spice up selected models; then illuminate/encryption would only need to provide some hook system to "make" it work, if the current infrastructure isn't enough.

Note: no idea about the feasibility, this was just a quick thought coming to my mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fair to put it on composer suggest and throw an exception saying you should install illuminate/encryption to use this feature. Optional dependencies exist in a lot of places. Migrations has one with doctrine, for instance.

Copy link
Contributor

@X-Coder264 X-Coder264 Aug 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this should be merged to Eloquent in any shape or form as this is totally out of scope of what an ORM should be responsible for. Eloquent has (more than) enough responsibilities with the accessor and mutator "magic" as is already IMHO.

If this really needs to be merged though I'd implement it the way @GrahamCampbell suggested, by adding a static property to the Model class for the encrypter and setting it in the framework booting process and then to check in these methods if it isn't set to do nothing (or throw an exception).

}

/**
* Decrypt attribute's value.
*
* @return mixed
*/
public function decryptAttribute($value)
{
return is_null($value) ? null : decrypt($value);
}

/**
* Get an attribute array of all arrayable values.
*
Expand Down Expand Up @@ -315,7 +356,8 @@ public function getAttribute($key)
// get the attribute's value. Otherwise, we will proceed as if the developers
// are asking for a relationship's value. This covers both types of values.
if (array_key_exists($key, $this->attributes) ||
$this->hasGetMutator($key)) {
$this->hasGetMutator($key) ||
$this->hasEncryption($key)) {
return $this->getAttributeValue($key);
}

Expand Down Expand Up @@ -353,6 +395,13 @@ public function getAttributeValue($key)
return $this->castAttribute($key, $value);
}

// If the user has added the $encrypt array to their model, then we are
// going to perform the decryption of that value before returning it to
// the user.
if ($this->hasEncryption($key)) {
return $this->decryptAttribute($value);
}

// If the attribute is listed as a date, we will convert it to a DateTime
// instance on retrieval, which makes it quite convenient to work with
// date fields without having to create a mutator for each property.
Expand Down Expand Up @@ -569,6 +618,12 @@ public function setAttribute($key, $value)
return $this->setMutatedAttributeValue($key, $value);
}

// Does the user want this attribute to be encrypted? Yes? Well, then
// encrypt the attribute before we save it.
if ($this->hasEncryption($key)) {
return $this->encryptAttribute($key, $value);
}

// If an attribute is listed as a "date", we'll convert it from a DateTime
// instance into a form proper for storage on the database tables using
// the connection grammar's date format. We will auto set the values.
Expand Down