Skip to content

WIP: New Wine GDPR review#9

Open
yanniboi wants to merge 21 commits intoclient/newwinefrom
client/newwine-dev
Open

WIP: New Wine GDPR review#9
yanniboi wants to merge 21 commits intoclient/newwinefrom
client/newwine-dev

Conversation

@yanniboi
Copy link
Contributor

No description provided.

@yanniboi yanniboi changed the title New Wine GDPR review WIP: New Wine GDPR review Apr 30, 2018
'default' => NULL,
'description' => 'The ID of consent agreement\'s default revision.',
),
'title' => array(
Copy link
Contributor

Choose a reason for hiding this comment

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

Presumably title should be revisionable also?

'title' => t('Grant Any Consent'),
),
'grant own consent' => array(
'title' => t('Grant Own Consent'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be a permission? Presumably all users should be able to grant their own consent?

$info = array();
$properties = &$info['gdpr_consent_agreement']['properties'];

$properties['id'] = array(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should revision_id also be exposed?

$element[$delta] = array(
'#type' => 'html_tag',
'#tag' => 'p',
'#value' => t('User Consent ID: @entity', array('@entity' => $item['target_id'])),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want a second formatter that includes who registered the consent + notes etc?

'#default_value' => $notes,
'#weight' => 10,
'#description' => '',
'#access' => user_access('grant any consent', $user),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also check the target user isn't themselves?

$rand = new GDPRUtilRandom();
$value = "anon_" . $rand->string(4);
// If the value is too long, tirm it.
if (isset($max_length) && strlen($value) > $max_length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Surely $max_length should be passed into $rand->string()?

*/
$plugin = array(
'handler' => array(
'class' => 'GDPRSanitizerDefault',
Copy link
Contributor

Choose a reason for hiding this comment

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

A 4 character random password is not a good idea...

/**
* Defines a utility class for creating random data.
*/
class GDPRUtilRandom {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a shame Drupal 8's components aren't available separately :(

/**
* Class for storing GDPR default sanitizer definition.
*/
class GDPRSanitizerDefault {
Copy link
Contributor

Choose a reason for hiding this comment

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

abstract?

* The sanitized input.
*/
public function sanitize($input, $field = NULL) {
return $input;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be either an abstract or throw an exception? Returning the input is probably dangerous for mis-configuration...

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.

2 participants