datamweb / shield-oauth

OAuth for CodeIgniter Shield
https://www.shield-oauth.codeigniter4.ir/
MIT License
61 stars 16 forks source link

Bug: Does not respect Shield !registration config #181

Closed evansharp closed 1 week ago

evansharp commented 2 weeks ago

PHP Version

8.3.2

CodeIgniter4 Version

latest

Shield Version

latest

Shield OAuth Version?

dev-develop

Which operating systems have you tested for this bug?

Linux

Which server did you use?

apache

Database

11.5.2-MariaDB-ubu2204

Did you add customize OAuth?

No.

What happened?

Setting public bool $allowRegistration = false; in \App\Config\Auth does not prevent users from creating new accounts via an oauth login.

Steps to Reproduce

  1. Toggle registration in shield
  2. Login via Google oauth with a google account previously unregistered in the app.
  3. New account is created and user is redirected according to config.

Expected Output

Redirect back to login view with error message about no new registrations being allowed.

Anything else?

I was surprised to discover no easy hooks in oauth for modifying behaviour before and after certain actions. If there were some I would probably have patched this bug myself.

Seems like implementing a few hooks into the oauth flow would be extremely useful for customization and extension. For example, I would like to filter new user registrations by Google domain OU. A hook when the response is received from Google but before the new user is created would give me the opportunity to do this smoothly.

datamweb commented 2 weeks ago

Shield OAuth, there is a separate configuration file that allows you to enable or disable user registration. This setup gives you better control and management over enabling or disabling registration for various drivers(google,gthub,yahoo and ...).

https://github.com/datamweb/shield-oauth/blob/700760dccde62114117e4ead0d259afcb3192092/src/Config/ShieldOAuthConfig.php#L42-L47

Seems like implementing a few hooks into the oauth flow would be extremely useful for customization and extension. For example, I would like to filter new user registrations by Google domain OU. A hook when the response is received from Google but before the new user is created would give me the opportunity to do this smoothly.

If you think the explanation above doesn't fully address your question, there might have been a misunderstanding on my part. Please feel free to provide more details, or, if possible, consider submitting a PR. This would give me a clearer view of the context and help in addressing your needs more effectively.

evansharp commented 2 weeks ago

No, this is very helpful, thank you.

Would it make sense to have the library also respect that system-wide configuration though?

evansharp commented 2 weeks ago

... the docs do not mention if usage of this configuration file is by ' the normal CI way'.

Does this class get copied into \App\Config as with most libraries?

datamweb commented 2 weeks ago

Would it make sense to have the library also respect that system-wide configuration though?

Your question is very valid and makes perfect sense. If setting public bool $allowRegistration = false; is disabled at the Shield system level, it’s logical that registration through shield-oauth or other drivers should also be disabled. This approach ensures that security settings are consistently enforced across the entire system, creating better alignment among the various components.

Does this class get copied into \App\Config as with most libraries?

Yes, this is covered in the documentation. Please run the following command:

php spark make:oauthconfig
evansharp commented 1 week ago

this is covered in the documentation

ahh yes, sorry. I had read the docs so many times I miss it.

Your question is very valid and makes perfect sense.

Would you like me to PR a change to the default config that grabs the shield value?

evansharp commented 1 week ago

I've also discovered that if using a config file, it is not possible to also set the client_id for a provider in .env. This should be changed - .env is a far safer place for API keys.

public array $oauthConfigs = [
        'github' => [
            'client_id'     => 'Get it from GitHub',
            'client_secret' => 'Get it from GitHub',

            'allow_login'    => false,
            'allow_register' => false,
        ],
        'google' => [
            //'client_id'     => 'Get it from Google',              // in the .env
            //'client_secret' => 'Get it from Google',

            'allow_login'    => true,
            'allow_register' => false,
        ],
        // 'yahoo' => [
        //     'client_id'     => 'Get it from Yahoo',
        //     'client_secret' => 'Get it from Yahoo',

        //     'allow_login'    => true,
        //     'allow_register' => true,
        // ],
    ];

I commented out the client_id field of the Google array, thinking that the .env values would set those fields when processed. Instead I get an exception that the client_id is not set.

My understanding of the .env. variables is that \App\Config classes were created first and then the .env could override/ combine with them.

The workaround was to not set a config class and to see the allow_register value I wanted in the .env as well.

datamweb commented 1 week ago

Would you like me to PR a change to the default config that grabs the shield value?

Yes, please feel free to submit a pull request. I'll review it as soon as possible. Thank you!

I've also discovered that if using a config file, it is not possible to also set the client_id for a provider in .env. This should be changed - .env is a far safer place for API keys.

For storing sensitive values like client_idand client_secret, you can safely use the .env file. Follow these steps:

Define values in .env

ShieldOAuthConfig.github.client_id=your_github_client_id
ShieldOAuthConfig.github.client_secret=your_github_client_secret

ShieldOAuthConfig.google.client_id=your_google_client_id
ShieldOAuthConfig.google.client_secret=your_google_client_secret