link of oauth account to nomal account

[qgp9]

This is for a feature to link an oauth account to a normal account.
There were many way what I could imagine for this.

Specially, Link informations could be stored in

1. user/accounts/each-normal-account-name.yaml
2. user/plugin/login-oauth.yaml
3. user/config/accounts.yaml

Each has own pros and cons, but I chosen (3) with some reasons (curious? :-) )

The format of accounts.yaml should be

linked_accounts:
    facebook:
        18918274928374928: any-normal-username

Also I removed $user->authenticate($password), because $user->authenticated = true would be enough here and I think that there is no difference in security-wise.
( Frankly, it was hard to implement this while keeping $user->authenticate($password) :-) )

Any comments?

[Vivalldi]

One thing that really jumps out to me is that there is nothing to create a link. It's great that you can check for links but those links won't exist unless you have some way to merge those (user side).

The next thing that sticks out to me is that you removed the

} else {
    $authenticated = $user->authenticate($password);
}

and the reason you had issues with that is you we had the else statement in the wrong "place." It was originally with the if (!$user->exists()) { statement but now follows a seperate statement.

I love the concept and would love to see what else you can do to implement this.

[qgp9]

@Vivalldi
Thank you for comments. I like to say about second one first.

Actually, I recognised the actual location of else. In the beginning I removed the else block and added next. So this diff screen is just github's favor :)
So, my actual changes are like this

        if (!$user->exists()) {
            // Create the user
            $user = $this->createUser([
                'id'       => $data['id'],
                'fullname' => $data['fullname'],
                'username' => $username,
                'email'    => $data['email'],
                'lang'     => $language,
            ]);
            // $authenticated = true;
            // $user->authenticated = true;
            $user->save();
        // } else {
            // $authenticated = $user->authenticate($password);
        }
        // Check linked user
        $linked_username = $this->grav['config']->get('accounts.linked_accounts.'.$username);
        if( $linked_username ){
            $linked_user = User::load($linked_username);
            if( $linked_user->exists() ){
                $user = $linked_user;
            }
        }
        // flag authenticated
        if( $user->exists() ){
            $authenticated = true;
            $user->authenticated = true;
        }

So still I think that $authenticated = $user->authenticate($password); is not necessarily.

1. Actual password is in real just id. Will there be any security issues even if we don't use it ?
2. In practical, Is there any functional difference between $user->authenticate($password) and $user->authenticated = true?

Since I'm not quite good for internals of Grav, I'm not quite sure but my current understanding is that "no" for both. Even in 'first visiting and creating account', $user->authenticate($password) is not being used but just $user->authenticated = true.

[qgp9]

@Vivalldi
And about first comment as second.

If I understood it correctly, you are pointing out that there is no routines to create a link between oauth and normal account in config/accounts.yaml.

I think you saw it correctly and this was actually intended by the first design.

1. If user don't do anything ( say, if user don't make "links" ), then everything should be same as before. Just independent accounts.
2. If user makes links explicitly, then linked oauth accounts will be forwarded to linked account while login
3. This only should work while oauth login but not while usual login. ( this is automatically done thanks to original login-oauth algorithms )
   So with my idea, there shouldn't be any routine to create actual link yet.

Of course, it will be great if there are any user interfaces to create 'links' and it can be done with

1. with GUI in admin panel
2. with plugin command
3. linking of normal account while oauth login with password
   But any of 1,2,3 will need another big step and also at another place of codes :).

[Sommerregen]

I jump into the discussion here, too, since I was heavily involved in this plugin. First of all, I like your idea @qgp9 , but I have serious security concerns about removing the authentication procedure in L342. It is there for purpose. In case a user exists, it should check if the requested password is the same as the stored user ones. As long as user id = password you are right to skip it, but better to keep it in-place.

Another question: Why not add an option directly in the users account file like oauth: true followed by some options to link the account to the oauth provider. There exists a User::find method we can use (https://github.com/getgrav/grav/blob/develop/system/src/Grav/Common/User/User.php#L64).

[qgp9]

@Sommerregen
If you want to keep L342 to emphasis possible security issues on future developments, I agree with it. I'll update code in that manner.

And the reason I choose /usr/config/accounts.yml is that.

1. Certainly oauth: true will not be enough because actual oauth id has no physical relation with a linked account. Rather 'facebook.id: 618726312131inany_user_name.yaml` can solve this.
2. Even though it is possible, and I agree that it looks clear but
   1. I'm not an expert of grav internal but I believe config.accounts.facebook... will be certainly faster then User::find
   2. In the management side, this linking should be done by admin or oauth user but MUST not by linked_account user. Because we don't have yet interface to set link by oauth user, only the admin is a right person/account to do it by requests. In this sense, if we store link information in each_user.yaml , then it is hard to manage them. ( Not literally, we have many tools in command line; grep, perl, even php :-) ). After all, a centralised list in accounts.yaml would be much easier to manage and reduces management mistakes.
   3. In this phase, this is not so serious, but similarly your concerns on security, the 'link information' in each_user.yaml could be compromised easily with user form interfaces (it's not exists yet), so I think that it would be better to keep those information in secure.

[qgp9]

@Sommerregen I have committed new codes. How is it?

[Sommerregen]

Hi @qgp9,

your PR looks good so far. @Vivalldi Are you here to check?

You are right config.accounts.facebook is faster than User::find. However things can be cached though. It's just a design issue and can be addressed later on if required. Just out of curiosity. At the time when I develop parts of this plugin, I noticed that the oauth user id seems to be not something known by the user. I know for Facebook and for Twitter it seems there are websites out there to extract the id, but how do you address it general? What is your workflow, or what should a user do, if he or she wants to use oauth for his or her Grav account?

[qgp9]

@Sommerregen

Yes, I agree with the speed issues. I didn't count it seriously too :).

And also I got same a problem about id. Yes, the id is different from what user know, even not universal but per APP ( as I know ). But fortunately I could add email scope on oauth and am using email information.
That will be my next contribution after this :)

p.s. also we have full-name but I don't like use it as an identifier.

[Vivalldi]

I like the new changes @qgp9 . I am currently unavailable and will test in about 5 hours. But it looks like it's coming along. And regarding the first/last name identification, you are right in that it's best to not use that for obvious security reasons.

I think however you should try to implement @Sommerregen's suggestion of using User:find. Though as he stated, it's merely a design issue that I may take a look at after I review the functionality tonight.

[Sommerregen]

@qgp9 I ask because if we merge your PR, a workflow has to be added to the README.md. At least we have to know for all oAuth provider how to get out the user id, so that it can be added to a possible linked_accounts.yaml.

Maybe it's good to know what @rhukster thinks about the changes and ideas.

[rhukster]

I'll have to read over this PR in detail and examine the actual use case. The validity and security issues concern me most. Ie, how can you be sure the user should be linked, how is this validated?

[qgp9]

@Sommerregen
Unfortunately, I heard that facebook doesn't provide username like paul.jang.31542(mine) with their API.
I got an idea about the workflow.
If we go for an email as a identifier, I can PR first for an email scope of facebook and a method to get an email information before this. Then the workflow will be more smooth.
Also we can add 'email to account linking' rather than 'numeric id to account linking' for a convenience, but I don't know this is a good way.

Actually there are indirect way to get a real username. Since Facebook redirect https://www.facebook.com/<numeric-username> to https://www.facebook.com/<real-username>, we can make small code for that. But I don't think this is a manageable method.

In conclusion, my suggestion is that

1. implementation of an email acquisition routine first.
2. add this 'linking' routine.

[qgp9]

@rhukster

I think there is no concrete automatic ways to verify an user or hard to do.
However if we think usual procedure of account management with Grav, certainly the email could be a key to verify accounts.

For example , one can request ( by offline ) same email address.

Basically, normal account user can't do nothing with linked oauth account. But possibly an facebook user can request with a fake information to get link to other's normal account.
Again only way above offline confirm by admin could be an email.

But in this stage, everything should be done, and could be done manually by admin.
It's a big reason why I split link information in the separated location ( config/accounts.yaml )

[qgp9]

@rhukster

Or an strict mode to check an identity of email addresses?

[Vivalldi]

@qgp9 I'm not sure what file I need to create to test this. and what to put in that file

[qgp9]

@Vivalldi

The file is grav_root/usr/config/accounts/accounts.yaml contains

linked_accounts:
    facebook:
        1821738295723904: qgp9

[Sommerregen]

@qgp9 Authenticating against usernames and email addresses, where a user is not involved concerns me a bit. What OAuth makes special, is that a user can connect a social media account to a service and can disconnect it at any time. It is done by the user and by the knowledge what kind of security risks it can bring.

In your approach @qgp9 however the admin or whoever is responsible has to take care of the right "linking" and that everything is fine.

What I'd like to see is an extension in the user account information page: an oauth entry, where you can connect your account to your social media accounts. This has the advantage, that (1) the user is authenticated against Grav, (2) it is initiated by the user and (3) the connection can be established without the user has to know the oauth id.

This works because once the user hits the "connect" button and logins into his/her social media account and grant access to the request made by Grav, we automatically get the oauth id and know which user belongs this id.

If you browse through the code, you can see that this plugin already does this to authenticate a user (but without storing the credentials).

[Vivalldi]

@Sommerregen I agree with you on that. One thing I was thinking was that the user page in that admin panel could be edited to allow the user to link themselves. (presuming an option is set to true). The other thing I was thinking was have a "linking" specific route. This route would require that the user be logged into their account based on a username and then when they visit the route they are given the option to log into an OAuth provider.

@qgp9 Your code functions as it should though putting my id in the file caused me to realize the same thing that Benny has mentioned.

[qgp9]

@Sommerregen @Vivalldi

Basically, I totally agree with you. It would be better if users has own decision and handling.
However there are two issues.

1. I think that the field operation will not work in secure. If normal users are allowed to manage their own oAuth id then
   1. One need to know own oAuth ID even though we can add a logic to give a hint based on email.
   2. Still one can add a link to an arbitrary oAuth by fake. Of course he/she can do nothing with that oAuth account but you know, this is a basic fishing technic.
2. So only possible and secure way will be @Vivalldi 's second idea.
   1. oAuth user log-in
   2. oAuth user is allowed to try to connect to a normal account in some page.
   3. There oAuth user is asked to auth by the password of a normal user. ( also with username in advance)
   4. Normal users are allowed to unlink whenever.
   5. If the best place is the accounts/oauthuserid.yaml, then we need to very careful on security. I think best way is that we just use access field but I have no idea about that policy of Grav for that .

So, now another issue in my mind is that,

1. without user info page, just one route to ask liking will be rather easier. but somewhat wried, isn't it? :)
2. User info page , or in Admin page, we need many more. I believe it's not so difficult but time consuming. By the way, I haven't experience for the admin page, is that easy to add fields or functionality there with some api, hook or blueprint?
3. Frankly, my those naive algorithms came with an idea that the Grav might not be designed for a big community or huge user group but only for friendly writer group. It's why I delegated an authority to admins.

[qgp9]

@Sommerregen
Sorry, I may misunderstood your logic :)

[Sommerregen]

Hi @qgp9 ,

no problem. If something is unclear, just ask 😄

1. First thing: the user must be authenticated against the system. This can be done via the Login plugin. This ensures, that the user is known to the Grav system (recognized via a session cookie) and the user provided the right credentials to login.

2. If this is the case, the user can access an OAuth authentication route, e.g., /oauth/connect/service:facebook. A callback is set with this or a special Grav route (which includes the name or a unique token). The user is now redirected to the OAuth service.

3. The user log in into the OAuth service and grant access. The user is redirected to the Grav page.

4. At this point Grav knows the user (session cookie) and the OAuth id for the service. It stores this id in the users account file. The user is redirected to the main page (or a success page, don't matter).

5. Besides the OAuth authentication route, there exists also a disconnect route , e.g., /oauth/disconnect/service:facebook, which basically deletes the service id from the user account file.

Both connect and disconnect an OAuth provider can be added as a field in the users account blueprint: one need to extend the blueprint and programmatically add the content (a list of OAuth providers with links to connect and disconnect the services with the current status). That's not a big deal, but requires a little CSS for styling, too (since no form field exists for it yet).

If the user log outs, then the session cookie is invalidated and the OAuth authentication connect/disconnect route will be unavailable. This way, no additional config file is needed, all information are stored in the respective user account yamls, the user don't need to know their OAuth account ids, the user has the full control, and the identification and authentication are secure.

Since a user (except the admin or whoever have the rights) cannot modify the user accounts data of other users this approach is secure for multiple users, too. If someone have access to the account data, then believe me, you have other problems.

[happywebio]

Hi!

I am not good enough to contribute to the code unfortunately but if you need help testing the code, running tests, etc. I would love to help (-: