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

PHP SDK: Use VaaS HTTP API #678

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

PHP SDK: Use VaaS HTTP API #678

wants to merge 30 commits into from

Conversation

lennartdohmann
Copy link
Contributor

No description provided.

@unglaublicherdude
Copy link
Member

Hey. I don't know what's better for you. I can also make a proposal PR if you like that more. I would rewrite the Authenticator stuff like that:

enum Suit: string
{
    case CLIENT_CREDENTIALS = 'client_credentials';
    case PASSWORD = 'password';
}

interface IAuthenticatorOptions {
}

class ResourceOwnerPasswordGrantAuthenticatorOptions implements IAuthenticatorOptions {
    public string $grandType = 'password';
    public function __construct(public string $clientId, public string $username, public string $password) {}
}

class ClientCredentialsAuthenticatorOptions implements IAuthenticatorOptions {
    public string $grandType = 'client_credentials';
    public function __construct(public string $clientId, public string $clientSecret) {}
}


class someAuthenticator {
    private function tokenRequestToForm(IAuthenticatorOptions $options): string
    {
        return match(true) {
            $options instanceof ResourceOwnerPasswordGrantAuthenticatorOptions => http_build_query([
                'client_id' => $options->clientId,
                'username' => $options->username ?? throw new InvalidArgumentException(),
                'password' => $options->password ?? throw new InvalidArgumentException(),
                'grant_type' => $options->grandType
            ]),
            $options instanceof ClientCredentialsAuthenticatorOptions => http_build_query([
                'client_id' => $options->clientId,
                'client_secret' => $options->clientSecret ?? throw new InvalidArgumentException(),
                'grant_type' => $options->grandType,
            ])
        };
    }
}

With that change you don't need the knowledge for what grandType does need what fields, because the classes are specific.

php/examples/VaasExample/GetVerdictByFile.php Outdated Show resolved Hide resolved
php/src/vaas/Authentication/GrantType.php Outdated Show resolved Hide resolved
php/src/vaas/Authentication/Authenticator.php Outdated Show resolved Hide resolved
php/src/vaas/Vaas.php Show resolved Hide resolved
php/src/vaas/Options/VaasOptions.php Outdated Show resolved Hide resolved
php/src/vaas/Readme.md Outdated Show resolved Hide resolved
php/src/vaas/Sha256.php Outdated Show resolved Hide resolved
php/tests/vaas/Sha256Test.php Outdated Show resolved Hide resolved
@unglaublicherdude
Copy link
Member

This PR is allot of work. Thank you for tackling it!

@lennartdohmann lennartdohmann marked this pull request as ready for review December 16, 2024 11:12
Co-authored-by: Matthias Simonis <[email protected]>
The benefit is, that you don't have to do error prone validation of arbitrary in some options-class. The authenticators know the fields they need, so there is no need to validate that because you cannot construct an object without them.
@unglaublicherdude
Copy link
Member

Since I find it to complicated to explain my proposal about the Authenticator stuff, I made a PR to your PR-Branch.

@unglaublicherdude
Copy link
Member

unglaublicherdude commented Dec 18, 2024

I also created a PR for my comments on the ForXXX Options.

#682

lennartdohmann and others added 5 commits December 18, 2024 15:41
…or_proposal

Refators the Authenntication stuff
* adds fromVaasOptions function for the forXXXOptions
* removes the default() function as it is not used anywhere
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.

4 participants