Issue #6346: Remote fields username and password not showing up in REST docs - Pulp

After a discussion on IRC with @dkliban, fao89, and bmbouter, we decided that pulp should use SecretCharFields for write-only data and to either document (or validate) that serializers shouldn't use the write_only option on fields.

Actions

Copy link

Updated by bmbouter over 3 years ago

I'm +1 on using SecretCharField. What would happen with a bindings user:

Use the bindings to read() the an endpoint whose serializer is using SecretCharField, say as field password
Call an update() on the bindings object to update another attribute asdf

Is there any possibility that while setting asdf the value of password is also changed? Or does it require an explicit update of the password field itself? What about PUT versus PATCH calls?

Actions

Copy link

Updated by dkliban@redhat.com over 3 years ago

It looks like if you take the FileRemote object returned by the read() or create() method and pass it to the update() method, the client_cert field gets updated to a new value. This will be true for any SecretCharField.



        (Pdb) file_remote
        {'ca_cert': None,
         'client_cert': 'b3226466e6d9c43c7058f69e1ff41daaf688cd223c084faa3e28202813ecff28',
         'client_key': None,
         'download_concurrency': 20,
         'name': 'bar25',
         'policy': 'immediate',
         'proxy_url': None,
         'pulp_created': datetime.datetime(2020, 3, 25, 17, 9, 13, 631685, tzinfo=tzlocal()),
         'pulp_href': '/pulp/api/v3/remotes/file/file/e42eea0d-e583-45ce-bdca-342169385cb2/',
         'pulp_last_updated': datetime.datetime(2020, 3, 25, 17, 9, 13, 631703, tzinfo=tzlocal()),
         'tls_validation': True,
         'url': 'https://repos.fedorapeople.org/pulp/pulp/demo_repos/test_file_repo/PULP_MANIFEST'}
        (Pdb) fileremotes.update(file_remote.pulp_href, file_remote)
        {'task': '/pulp/api/v3/tasks/ec6a2bb6-4ce4-4a0e-ab1c-62d37aecfd27/'}
        (Pdb) file_updated_remote = fileremotes.read(file_remote.pulp_href)
        (Pdb) file_updated_remote
        {'ca_cert': None,
         'client_cert': '7fc2a6b69d81c4581eac98454217b173a8b23256eefad0bb9eaabe199d8baae8',
         'client_key': None,
         'download_concurrency': 20,
         'name': 'bar25',
         'policy': 'immediate',
         'proxy_url': None,
         'pulp_created': datetime.datetime(2020, 3, 25, 17, 9, 13, 631685, tzinfo=tzlocal()),
         'pulp_href': '/pulp/api/v3/remotes/file/file/e42eea0d-e583-45ce-bdca-342169385cb2/',
         'pulp_last_updated': datetime.datetime(2020, 3, 25, 17, 10, 37, 747011, tzinfo=tzlocal()),
         'tls_validation': True,
         'url': 'https://repos.fedorapeople.org/pulp/pulp/demo_repos/test_file_repo/PULP_MANIFEST'}

Actions

Copy link

#10

Updated by fao89 over 3 years ago

write_only=True

pulp-2to3-migration:

    validate = serializers.BooleanField(
        help_text=_('If ``True``, migration cannot happen without successful validation '
                    'of the Migration Plan'),
        required=False,
        default=False,
        write_only=True
    )
    dry_run = serializers.BooleanField(
        help_text=_('If ``True``, performs validation of a Migration Plan only, no migration is '
                    'run.'),
        required=False,
        default=False,
        write_only=True
    )

pulp-certguard:

    ca_certificate = serializers.FileField(
        help_text=_("The Certificate Authority certificate."),
        write_only=True
    )

pulp_container:

    content_units = serializers.ListField(
        help_text=_('A list of content units to operate on.'),
        write_only=True,
        required=False
    )
    source_repository = serializers.HyperlinkedRelatedField(
        help_text=_('A URI of the repository to copy content from.'),
        queryset=models.ContainerRepository.objects.all(),
        view_name='repositories-container/container-detail',
        label=_('Repository'),
        write_only=True,
        required=False,
    )
    source_repository_version = NestedRelatedField(
        help_text=_('A URI of the repository version to copy content from.'),
        view_name='versions-detail',
        lookup_field='number',
        parent_lookup_kwargs={'repository_pk': 'repository__pk'},
        queryset=RepositoryVersion.objects.all(),
        write_only=True,
        required=False,
    )

    containerfile = serializers.FileField(
        help_text=_(
            "An uploaded Containerfile that should be used to run buildah."
        ),
        required=False,
        write_only=True,
    )

pulpcore:

    relative_path = serializers.CharField(
        help_text=_("Path where the artifact is located relative to distributions base_path"),
        validators=[fields.relative_path_validator],
        write_only=True,
    )
    username = serializers.CharField(
        help_text='The username to be used for authentication when syncing.',
        write_only=True,
        required=False,
        allow_null=True,
    )
    password = serializers.CharField(
        help_text='The password to be used for authentication when syncing.',
        write_only=True,
        required=False,
        allow_null=True,
    )
    add_content_units = serializers.ListField(
        help_text=_('A list of content units to add to a new repository version. This content is '
                    'added after remove_content_units are removed.'),
        write_only=True,
        required=False
    )
    remove_content_units = serializers.ListField(
        help_text=_("A list of content units to remove from the latest repository version. "
                    "You may also specify '*' as an entry to remove all content. This content is "
                    "removed before add_content_units are added."),
        write_only=True,
        required=False
    )
    file = serializers.FileField(
        help_text=_("A chunk of the uploaded file."),
        write_only=True,
    )

    sha256 = serializers.CharField(
        help_text=_("The SHA-256 checksum of the chunk if available."),
        required=False,
        allow_null=True,
        write_only=True,
    )
    file = FileField(
        help_text=_(
            "An uploaded file that should be turned into the artifact of the content unit."
        ),
        required=False,
        write_only=True,
    )
    repository = DetailRelatedField(
        help_text=_(
            "A URI of a repository the new content unit should be associated with."
        ),
        required=False,
        write_only=True,
        queryset=Repository.objects.all(),
    )

Actions

Copy link

#11

Updated by bmbouter over 3 years ago

I see the write_only usage falling into two categories.

write_only for secrecy¶

These are fields that are unsafe to be re-shown to the user. These are the ones that should have a solution similar to SecretCharField that shows you the sha256 when viewed, and refuses to overwrite data if you're submitting the sha256 of the data already stored.

The only ones I think that fall in this category are: Remote.username, Remote.password

write_only for correctness¶

These are all the others, and I think these should be viewable but only allowed to set once. These would be all other fields.

Actions

Copy link

#12

Updated by daviddavis over 3 years ago

I think there's also a third category: a field that is not actually stored in the database. The fields on MigrationPlanRunSerializer[0] are an example.

In such cases, perhaps we just roll a write only serializer to be used for POST/PUT/PATCH (but not GET) and not set the write_only option on these fields.

[0] https://git.io/Jv9EU

Actions

Copy link

#13

Updated by bmbouter over 3 years ago

daviddavis wrote:

I think there's also a third category: a field that is not actually stored in the database. The fields on MigrationPlanRunSerializer[0] are an example.

In such cases, perhaps we just roll a write only serializer to be used for POST/PUT/PATCH (but not GET) and not set the write_only option on these fields.

[0] https://git.io/Jv9EU

Agreed, sounds great.

Actions

Copy link

#14