Conversation
TODO: - Migrate to creating or fetching MO2 API key from graphql - Convert all queries to graphql - Collections?
| void validateTokens(const NexusOAuthTokens& tokens); | ||
| bool persistTokens(const NexusOAuthTokens& tokens); | ||
| bool clearTokens(); | ||
|
|
||
| void onSSOKeyChanged(const QString& key); | ||
| void onSSOStateChanged(NexusSSOLogin::States s, const QString& e); | ||
| void onTokensReceived(const NexusOAuthTokens& tokens); | ||
| void onOAuthStateChanged(NexusOAuthLogin::State s, const QString& message); |
There was a problem hiding this comment.
Should we not still have the equivalent of these methods for the API key?
I think these are currently doing a mix for both?
| bool APIUserAccount::isValid() const | ||
| { | ||
| return !m_key.isEmpty(); | ||
| return !m_accessToken.isEmpty() && !m_apiKey.isEmpty(); |
There was a problem hiding this comment.
I think this was probably meant to be an OR
| return !m_accessToken.isEmpty() && !m_apiKey.isEmpty(); | |
| return !m_accessToken.isEmpty() || !m_apiKey.isEmpty(); |
| bool NXMAccessManager::ensureFreshToken() | ||
| { | ||
| if (!m_tokens) { | ||
| log::warn("nexus: no OAuth tokens available"); | ||
| return false; | ||
| } | ||
|
|
||
| if (!m_tokens->accessToken.isEmpty()) { | ||
| if (!m_tokens->isExpired()) { | ||
| return true; | ||
| } | ||
|
|
||
| const auto refreshed = refreshTokensBlocking(*m_tokens); | ||
| if (!refreshed) { | ||
| return false; | ||
| } | ||
|
|
||
| setTokens(*refreshed); | ||
| GlobalSettings::setNexusOAuthTokens(*refreshed); | ||
| return true; | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
We should add a m_refreshing guard to avoid having multiple API calls refreshing the token simultaneously, we could end up saving the wrong token or weird shenanigans.
| if (!m_tokens.accessToken.isEmpty()) { | ||
| if (!data.contains("sub")) { | ||
| setFailure(HardError, QObject::tr("Bad response")); | ||
| return; | ||
| } | ||
|
|
||
| const QString id = data.value("sub").toString(); | ||
| const QString name = data.value("name").toString(); | ||
| const auto roles = data.value("membership_roles").toArray(); | ||
| QStringList validRoles = {"premium", "lifetimepremium", "supporter"}; | ||
| bool premium = false; | ||
| for (auto role : roles) { | ||
| QString roleVal = role.toString(); | ||
| if (validRoles.contains(roleVal)) { | ||
| premium = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| const int id = data.value("user_id").toInt(); | ||
| const QString key = data.value("key").toString(); | ||
| const QString name = data.value("name").toString(); | ||
| const bool premium = data.value("is_premium").toBool(); | ||
| if (m_tokens.accessToken.isEmpty()) { |
There was a problem hiding this comment.
if (m_tokens.accessToken.isEmpty()) was already checked a few lines above, unless it can have changed in the mentime?
There was a problem hiding this comment.
This can probably be folded into an 'else'. We can only reach this point assuming at least one access token or api key is available. So some of these checks are to determine which flow we're using. Of course, if we separate these flows further that isn't needed.
| void NXMAccessManager::clearTokens() | ||
| { | ||
| m_validator.cancel(); | ||
| m_tokens.reset(); | ||
| emit credentialsReceived(APIUserAccount()); | ||
| } |
There was a problem hiding this comment.
Should we revoke the tokens as well?
There was a problem hiding this comment.
Well, they will also expire after a while but I suppose that's probably a good idea.
|
@Al12rs A couple questions. The original code (as well as Vortex when I checked it) indicated there would be a 'token_type' attribute, but when I was getting the values from the response I didn't see that anywhere. Does this still get sent? Also, are there API limits with OAuth / GraphQL? I didn't see any headers like the V1 API includes. But if they do still exist it would be good to make sure we're accounting for them. |
|
From @Al12rs on discord:
@aglowinthefield 's original PR was based on 2.5.2 and old Qt so was probably mostly accurate for the time. I'll admit I didn't look that much into how those classes had changed over the past several Qt versions. That being said, there's certainly some Qt version checking we don't need to maintain, and I'll take a look at the QOAuth2AuthorizationCodeFlow stuff when I have time. |
|
I wasn't sure how long the token would last but it looks like it expired now and I ran into issues on startup so let me look at this and fix it up. I'll try to clean up the 'login' class considering the above. |
|
We're still doing CI on 6.7.3 - I'm using functions added in 6.9.0, so mob will need to be updated to 6.11.0. |
e9eb128 to
66e1fd2
Compare
|
Noting original ticket here: #2276 You will need to add PKCE if you haven't already: https://modding.wiki/en/api/oauth2-guide |
|
@Pickysaurus Well the original PR implemented it manually, but implementing PKCE is the default behavior in Qt as of 6.8. This PR functions, I've been using it. As of now the applicaition ID is set to default to 'modorganizer2' though it can be set by an environment variable. If you want to use something else let us know. modorganizer/src/nexusoauthconfig.cpp Line 40 in 72bce4c |
- Moved all OAuth code to NXMAccessManager - Removed unnecessary manual auth / refresh code - Ensured we initialize / refresh token on startup - Use OAuth flow to call API endpoints
72bce4c to
87135c1
Compare
- Consolidate API header function - Extend OAuth request functions - Remove unneeded function - Wait for auth to finish before doing validate - Fallback refresh as autorefresh is inconsistent - Better queueing of auth / validate
| bool GlobalSettings::nexusOAuthTokens(NexusOAuthTokens& tokens) | ||
| { | ||
| // If legacy credential key exists and is not set in the new credentials, | ||
| // insert it into the current tokens. In all cases, clear the old credential | ||
| // store once parsed. | ||
| const auto legacyRaw = getWindowsCredential(NexusLegacyCredentialKey); | ||
| if (!legacyRaw.isEmpty()) { | ||
| tokens.apiKey = legacyRaw; | ||
| setWindowsCredential(NexusLegacyCredentialKey, ""); | ||
| } | ||
| const auto raw = getWindowsCredential(NexusOAuthCredentialKey); | ||
| const auto parsed = parseStoredTokens(raw); | ||
| if (!parsed && legacyRaw.isEmpty()) { | ||
| return false; | ||
| } else if (parsed && legacyRaw.isEmpty()) { | ||
| tokens = *parsed; | ||
| } else if (parsed) { | ||
| if (!parsed->apiKey.isEmpty()) { | ||
| tokens = *parsed; | ||
| } else { | ||
| tokens.accessToken = parsed->accessToken; | ||
| tokens.refreshToken = parsed->refreshToken; | ||
| tokens.expiresAt = parsed->expiresAt; | ||
| tokens.tokenType = parsed->tokenType; | ||
| tokens.scope = parsed->scope; | ||
| setNexusOAuthTokens(tokens); | ||
| } | ||
| } else { | ||
| setNexusOAuthTokens(tokens); | ||
| } |
There was a problem hiding this comment.
This function is very confusing. From the name I would have no idea it is doing something with the API key.
We should definitely not use OAuth if we are also talking about API key.
I would suggest using nexusAuthCredentials if we have to refer to both OAuth token and API key.
But to be honest, I don't know why we would not have two separate methods. I would have one for API key and one for the token. We can leave the API key where it was before, why migrate it. Unless I'm missing something.
There was a problem hiding this comment.
I don't know. You end up just requiring multiple credential calls or separate storage either way, so you're adding complexity in one direction or the other. But at the end of the day I think I just rolled with the original PR removing the API key altogether initially, and then thought we might need to fetch / generate the key to call the old validate function. So they ended up tied together.
But with the separate users API to validate with, now it functions mostly as a fallback / backwards compatibility measure.
I suppose I can roll back those particular changes and keep both storage methods.
|
Your OAuth client data has now been set up on the Nexus Mods side.
As previously mentioned, you'll need to use PKCE, which protects the login session from being hijacked. |
|
@Pickysaurus As I've stated I believe Qt versions since 6.8 automatically use a PKCE flow, but just to be sure I set it manually here: modorganizer/src/nxmaccessmanager.cpp Line 939 in a404cee I'm already using this OAuth login locally. @Al12rs did give us some of this info on Discord. It wasn't clear to me if the secret key was actually being used anywhere. I don't know if you have any info on that. |
|
I'm going to separate the credentials tonight and I think the PR will be ready. I suppose my only personal critique would be that it might be nice to serve a slightly more visually appealing HTML page on success. It's pretty plain at the moment. |
The secret is only used for private applications (i.e. a web server that makes requests on behalf of a user). Mod managers are public apps as they make API requests directly. Essentially for public apps you use PKCE to verify, whereas private apps don't need that and just send their client secret. |
- Functions as fallback / backwards compat - Fix a couple minor oversights
a404cee to
33d5378
Compare
|
Nevermind. I changed around how various functions triggered, and it turns out the tokenChanged signal fires before all of the response data is parsed. It's working if I wait until the 'granted' signal fires. |
- Remove callbacks, implement signal / slots - Revert isValid since the API key is handled separately - Add additional callbacks / error handling - Try to ensure the reply handler is ready
- Make sure validator is properly reset on finish - Move revalidation check to nexusinterface - Stop the API processing until validation clears - Avoids request collisions - Should restart on revalidation - Remove autorefresh for now as it isn't working - Extend refresh time to 5 minutes
This supplants #2302
This replaces the old API key authorization loop in favor of the newer OAuth authentication system.
For backwards compatibility, it is still able to load and use the old API key if set but will prefer any stored OAuth credentials.
It is still possible to manually enter an API key if desired.
So far as I can tell, all the v1 endpoints are functional with the OAuth token (with the exception of the 'validation' endpoint that requires an API key).
This is also a stepping stone to being able to use the GraphQL and v3 REST APIs which should allow us to implement collections fetching as well...