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.

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

WIP: New Wine GDPR review #9

Open
wants to merge 21 commits into
base: client/newwine
Choose a base branch
from
Open

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