Art-of-WiFi / UniFi-API-client

A PHP API client class to interact with Ubiquiti's UniFi Controller API
MIT License
1.09k stars 217 forks source link

Add option to pass 2FA on login #195

Closed mauro-baptista closed 6 months ago

mauro-baptista commented 11 months ago

Issue:

Some of the customers are trying to log in using accounts with 2FA, which just throws a generic login error.

Solution:

Pass the token as an argument at the time of the login.

Note:

malle-pietje commented 11 months ago

Thanks, will look into this when I find the time. Your final remark touches one of own planned changes which will affect backwards compatibility; use of Exceptions instead of triggering errors. Maybe something for a 2.0 release…

mauro-baptista commented 11 months ago

Thanks for the quick reply. Ok, take your time, and let me know if you need anything else.

thib3113 commented 11 months ago

@mauro-baptista where did you get the field ubic_2fa_token ? on my unifi the field is token . Is it an alias ? ( working only on some consoles ? )

mauro-baptista commented 11 months ago

@thib3113 I got it from the payload sent in the 2FA.

Maybe the better approach here would send an array in the login, and then merge it with user and pwd, so it is more flexible.

Screenshot 2023-07-20 at 11 39 11@2x

thib3113 commented 11 months ago

@mauro-baptista when I log on my unifios console, the payload is like :

{
  "username":"thib3113",
  "password":"<password>",
  "token":"123456",
  "rememberMe":true
}

( UDM Pro ) .

That's why I'm asking :) . Is the 2Fa field different between unifios and unifi ?


Doing tests, on my side . Passing token or ubic_2fa_token produce different behavior .

token : success

<object with user informations>

failed

{
    "code": "AUTHENTICATION_FAILED_INVALID_CREDENTIALS",
    "message": "Invalid username or password",
    "level": "debug"
}

ubic_2fa_token : failed with http status (even with correct totp) 499* :

{
  "data": {
    "required": "2fa",
    "authenticators": [
      {
        "blocked_reason": null,
        "created": "<date>",
        "email": "<accountEmail>",
        "id": "<uuid>",
        "last_success": null,
        "status": "active",
        "type": "email"
      },
      {
        "codes_left": 10,
        "created": "<date>",
        "id": "<uuid>",
        "last_success": null,
        "status": "backup",
        "type": "backup_code"
      },
      {
        "created": "<date>",
        "id": "<uuid>",
        "last_success": "<date>",
        "name": "Authenticator app",
        "status": "active",
        "type": "totp"
      }
    ],
    "public_key_credential_request_options": null,
    "user": {
      "default_mfa": "<uuid>"
    },
    "mfaCookie": "UBIC_2FA=<base64cookie>"
  },
  "code": "MFA_AUTH_REQUIRED",
  "message": "MFA token required to authenticate to SSO",
  "level": "debug"
}

*499 is the status code when 2FA is needed


So, this field maybe differ between unifios console and unifi ?

thib3113 commented 11 months ago

Ok, so I've done more searches ... And it seems it's token on unifios, and ubic_2fa_token else .

They're not aliases .. so we can't use ubic_2fa_token on each side, or token .

I tested with :

mauro-baptista commented 11 months ago

Ok. Thanks to go deep on it. I only have one network to test it here on my side.

Maybe a best generic approach here would be, instead of sending a token, we can send an array that will be merged with user and pwd?

Kind like send an $options = [];

And then it can be passed the index needed to proceed with the 2FA: $options['form'] => ['token' => 1234567];

What do you think? Could it work?

Note: I'm make it as $options, so we can also make an index that will throw the raw response in case of an exception. Just an idea.

thib3113 commented 11 months ago

@mauro-baptista Can't tell you about this . on my lib (unifi-client in nodejs), I'm taking the 2FAtoken . And then generate the body to send . ( but, at this stage, I know if it's an unifios or not ) ...

So my opinion was to reuse the unifios check : https://github.com/Art-of-WiFi/UniFi-API-client/blob/a3e42643e65908919cebcb32abb9fb99416a4d26/src/Client.php#L181-L184 . And add the corresponding 2FA field (token or ubic_2fa_token) ?


About the idea of the options I think this is too overkill ... first, their is not lot of "options" available to login ( maybe 2FA and rememberme ? but the lib doesn't return the content so ... not a good idea ) . About an option to throw error, it seems strange to me to do this only on one function ? I think it can be implemented in a new major of this lib, but will be strange actually .

I understand the "need" . but finally, the program can't do lot of things after a 2FA error ... just warn the user ?

( I'm not the developper of this lib, and not an "actual" php developper ... so, my opinion is just my opinion )

mauro-baptista commented 11 months ago

Ok. I will take a look at the unifios check and try to integrate it. Thanks for this idea.

About the option, indeed, it should be on the class level, not in a single method, but maybe the idea to have an array that will be merged in the form in the login wouldn't be.

And the error is a matter of a better UX. As of now both errors for wrong pwd and invalid 2FA are the same, so I cannot show the correct error. That is why I would like to have better error handling, but I understand it will be taken care in the next major release.

I will explore the idea you gave and send some commits on Monday. Thanks.

malle-pietje commented 6 months ago

Because many applications that use this API client class do not have the ability to request the user the 2FA code (think of captive portals where the guest authorizations are performed through the API) I'm not sure this is going to work that well.

Although I really appreciate this discussion, I suggest we close this PR for now.