Repository navigation
Support PKCE and refresh token rotation for connected app logins - #4013
Conversation
The CumulusCI connected app is adopting Salesforce connected app security controls: PKCE, refresh token rotation, and a 30-day idle limit on refresh tokens. Without client changes, `cci org connect` fails once PKCE is required, and rotated refresh tokens are lost. - `cci org connect` sends a PKCE code challenge (S256) and verifier. PKCE is opt-in on OAuth2ClientConfig, so other OAuth clients are unchanged. - Refreshing an org's token now holds a per-org file lock, starts from the tokens currently stored in the keychain, and saves a rotated refresh token right away. Before, `org info`, `org browser`, `org shell`, SalesforceCommand, and the Metadata API retry never saved it. - Once an org has seen a rotation, cci reuses the stored access token while it still works, because each refresh also ends the session of the access token issued before it. Saving such an org takes the same lock and keeps tokens another process rotated in the meantime. - A rejected refresh token now says to run `cci org connect <org>`. - Adds the filelock dependency.
sumeet-parmar
left a comment
There was a problem hiding this comment.
please review the feedback comments and make necessary changes
| org_path = self._get_org_path(name, org.global_org) if org else None | ||
| if org_path and org_path.exists(): | ||
| self.orgs[name] = org._replace( | ||
| data=org_path.read_bytes(), filename=str(org_path) |
There was a problem hiding this comment.
[Fix before merge] EnvironmentProjectKeychain mode: reloading an out-of-date file makes the next refresh reuse a rotated-out token
When CUMULUSCI_KEYCHAIN_CLASS=EnvironmentProjectKeychain, _set_org updates self.orgs in memory but doesn't write the .org file. But reload_org still reads that file when it exists. _load_latest_tokens then sees a refresh token that's different from the one in memory and takes it, even though it's older.
Reproduced (org file on disk, then two refreshes in one process with _access_token_works → False):
refresh 1: sends TOKEN_1 → gets TOKEN_2 (kept in memory only)
refresh 2: reload_org reads the file → TOKEN_1 again → sends TOKEN_1
REFRESH TOKENS SENT: ['TOKEN_1', 'TOKEN_1']
Under rotation, the second call reuses a rotated-out token, so Salesforce revokes the whole family.
Suggestions: skip the file re-read when saving is disabled. Or, more generally, only take stored tokens when they're newer, e.g. save a token_saved_at timestamp or a counter with the tokens and compare.
There was a problem hiding this comment.
Fixed. With that keychain, reload_org no longer reads the file and org_lock doesn't lock, since the file is never written. Also, _load_latest_tokens now adopts stored tokens only if they differ from the token this config was loaded with or last saved, not from the token in memory. So an older file can't replace newer tokens in memory. Test: test_refresh_oauth_token__rotation_when_orgs_not_saved_to_files: refresh tokens sent are TOKEN_1, then TOKEN_2.
| def save(self): | ||
| assert self.keychain, "Keychain was not set on OrgConfig" | ||
| self.keychain.set_org(self, self.global_org) | ||
| if self.refresh_token_rotation: |
There was a problem hiding this comment.
[Fix before merge] Only processes that already have the rotation flag are protected here
The lock and merge in save() depend on the in-memory refresh_token_rotation. If a process loaded the org before rotation was ever detected, it has no flag and writes its out-of-date tokens with a plain set_org, without the lock.
Reproduced:
- Process A loads the org (
TOKEN_1, no flag) - Process B refreshes, rotates, and saves
TOKEN_2+refresh_token_rotation: True - A changes something and calls
save()without refreshing first
STORED AFTER A.save(): TOKEN_1 None
The dead token is back in the file and the flag is gone, so the next command reuses TOKEN_1 and the family gets revoked.
Suggestion: also look at the flag in the stored file. Or always take the lock and merge tokens for orgs that use refresh tokens (it's cheap). Be careful not to break cci org connect re-connects, which should replace the stored tokens.
Related, but code can't fix it: an older cci version sharing ~/.cumulusci (e.g. a pipx install alongside a project venv) will always write back out-of-date tokens. That's worth a release note.
There was a problem hiding this comment.
Fixed. save() now locks and merges for any org with a refresh token, using the same "changed since loaded" check. Org configs that didn't come from the keychain (cci org connect) still replace the stored tokens. Tests: test_save__keeps_first_rotation_by_another_process, test_save__connected_org_replaces_stored_tokens. The older-cci-sharing-~/.cumulusci case goes in the release notes.
| def _access_token_works(self) -> bool: | ||
| if not self.access_token: | ||
| return False | ||
| response = requests.get( |
There was a problem hiding this comment.
[Fix before merge] Network calls run while the lock is held, with no timeout
This requests.get (and the token POST in _refresh_token) runs while the per-org file lock is held, and neither call has a timeout. If either one hangs (VPN drop, proxy, Salesforce maintenance), the lock stays held indefinitely. Every other cci process on that org then waits ORG_LOCK_TIMEOUT (60s) and fails with a raw filelock.Timeout.
filelock.Timeout isn't a CumulusCIException, so users see something like:
Error: The file lock '/Users/.../.cumulusci/<project>/dev.org.lock' could not be acquired.
Suggestions:
- Pass an explicit
timeout=to both HTTP calls (ideally well underORG_LOCK_TIMEOUT) - Catch
filelock.Timeoutin_refresh_lock/org_lockand raise aCumulusCIExceptionthat says another cci process is refreshing org X and to retry
There was a problem hiding this comment.
Fixed. The userinfo GET times out after 20s (USERINFO_TIMEOUT). The refresh POST only gets a 10s connect timeout (TOKEN_REFRESH_TIMEOUT = (10, None)), because it isn't safe to retry under rotation: if Salesforce rotated the token and the response timed out, the next run would send the old token and revoke the whole family. In that rare case, waiting processes get the lock timeout instead, which is safe. filelock.Timeout becomes a CumulusCIException: "Timed out after 60 seconds waiting for another cci process to finish updating org X. Try again."
| org_path = self._get_org_path(name, global_org) | ||
| if org_path is None or not org_path.parent.exists(): | ||
| return nullcontext() | ||
| return FileLock(f"{org_path}.lock", timeout=ORG_LOCK_TIMEOUT) |
There was a problem hiding this comment.
[Fix before merge] Orgs that don't rotate now fail to refresh when the directory isn't writable
The lock creates <org>.org.lock next to the org file. If the directory isn't writable but the .org file is, which happens with some Docker/CI images and shared setups, every refresh now fails. That includes orgs that never rotate, which used to work:
PermissionError: [Errno 13] Permission denied: '.../.cumulusci/TestProject/test.org.lock'
Suggestion: catch OSError when creating or acquiring the lock, log at debug level, and fall back to nullcontext(). That's the same behavior as having no lock, which is what everyone has today.
There was a problem hiding this comment.
Fixed. An OSError while acquiring logs at debug level and continues without the lock. cci creates the lock file itself before acquiring, because filelock on Windows takes EACCES when creating it to mean the lock is held and retries until the timeout (and os.access(dir, W_OK) is always true for directories on Windows). The atomic write from #6 also falls back to writing in place. Tests: test_org_lock__directory_not_writable, test_org_lock__lock_file_cannot_be_created, test_save_org__directory_not_writable.
| except OAuth2Error as e: | ||
| if "invalid_grant" not in str(e): | ||
| raise | ||
| raise OAuth2Error( |
There was a problem hiding this comment.
[Should fix] The new message hides Salesforce's error_description
handle_exception in cli/cci.py prints only str(error); the chained from e goes only to the log file. invalid_grant isn't always an expired or revoked token. Salesforce also returns it for inactive user, ip restricted, user hasn't approved this consumer, authentication failure, and a sandbox/prod login URL mix-up. In those cases, telling the user to run cci org connect sends them the wrong way.
Suggestion: parse the response and include error_description, e.g.
The refresh token for org dev is no longer valid (Salesforce said: "expired access/refresh token"). ...
Small nit: matching on "invalid_grant" in str(e) is fragile. Exposing the status/JSON on OAuth2Error would make this sturdier.
There was a problem hiding this comment.
Fixed. OAuth2Error now carries response. The message is "Salesforce rejected the refresh token for org X: <error_description>. If the token expired from inactivity or was revoked, run cci org connect X...". It matches on the JSON error field, not on a substring.
| else: | ||
| filename = Path(f"{self.project_local_dir}/{name}.org") | ||
|
|
||
| fd = os.open(filename, OS_FILE_FLAGS, SERVICE_ORG_FILE_MODE) |
There was a problem hiding this comment.
[Should fix] Org files aren't written atomically, and they're now re-read on every refresh
_save_org truncates the file (O_TRUNC) and then writes it, so the write isn't atomic. This PR makes reload_org run on every refresh, including orgs that don't rotate, but several writers don't take the lock: cci org connect (new OrgConfig with no flag), save() on orgs without the flag, and set_default_org. If a read lands in the middle of a write, it gets a truncated file and fails mid-flow with OrgCannotBeLoaded/KeychainKeyNotFound.
It's unlikely, but the fix is cheap: write to a temp file in the same directory and os.replace() it into place (keeping SERVICE_ORG_FILE_MODE).
There was a problem hiding this comment.
Fixed. cci writes a unique temp file next to the org file (tempfile.mkstemp, mode 0600), fsyncs it, and calls os.replace, following a symlinked org file. On PermissionError (directory not writable, or the file is open on Windows) or EBUSY (org file mounted on its own), it writes in place as before. Other errors (e.g. disk full) are raised without touching the org file. Tests: test_save_org__no_temp_file_left, test_save_org__file_in_use, test_save_org__write_fails, test_save_org__symlinked_org_file, test_save_org__org_file_mounted_on_its_own.
| self.config["refresh_token_rotation"] = True | ||
| if self.keychain: | ||
| # save() would take the lock again, which is already held | ||
| self.keychain.set_org(self, self.global_org) |
There was a problem hiding this comment.
[Should fix] Taking the lock twice in one process blocks for 60s
The comment above explains that set_org is used because save() would take the lock again. Each org_lock() call creates a new FileLock, and on Unix flock locks belong to the open file descriptor. So if one process takes the lock twice, the second attempt blocks for ORG_LOCK_TIMEOUT and then raises Timeout. Any later change that calls save() (or something that calls it) inside this block will hit that.
Suggestions:
- Use
FileLock(..., is_singleton=True)(or cache one lock per path) so taking it twice in the same process is safe filelockis added topyproject.tomlwith no version floor. Please add a minimum (e.g. the version whereis_singletonwas added) so older installs don't behave differently
There was a problem hiding this comment.
Fixed. FileLock(..., is_singleton=True), so the refresh path now calls save() directly. The pin is filelock>=3.15.2: is_singleton came in 3.13.0, but singleton fixes kept landing until 3.15.2.
|
@sf-brahm-gaur thanks for this. The PKCE implementation looks correct (128-char verifier, S256, unpadded base64url, regenerated on every login, scoped to I found a few rotation edge cases. In two of them, a rotated-out refresh token still gets reused, and Salesforce then revokes the whole token family. I reproduced these with throwaway tests against this branch. Inline comments below: Should be fixed before merge
Should fix |
- Only adopt stored tokens that another process wrote since this org was loaded or saved. Before, an older file could replace newer tokens in memory, for example with EnvironmentProjectKeychain, which never writes the file. That keychain no longer re-reads or locks org files. - Saving any org with a refresh token takes the org lock and keeps tokens another process rotated, even before this process has seen a rotation. An org config that didn't come from the keychain, like a new `cci org connect`, still replaces the stored tokens. - The userinfo check times out after 20 seconds. The token refresh only has a 10 second connect timeout: if Salesforce rotated the token but the response were cut off, sending the old token again would revoke the whole token family. A lock timeout raises a CumulusCIException saying another cci process is updating the org. If the lock file can't be created, cci goes on without a lock, as before. cci creates the lock file itself, because filelock on Windows waits until it times out when it can't. - A re-raised OAuth2Error no longer carries an unrelated AttributeError or ValueError as its context. - The org lock can be taken again by the process holding it (is_singleton). Requires filelock>=3.15.2. - Org files are written to a unique temporary file (mkstemp), synced to disk and moved into place (following a symlinked org file), falling back to writing in place on PermissionError or EBUSY (an org file mounted on its own). Other errors, like a full disk, are raised without touching the org file. - Stored tokens aren't adopted if the org was connected again as another org or user (the identity URL differs). If the stored org file can't be loaded, saving overwrites it, as before. - SubprocessKeychain (Snowfakery parallel workers) gets no-op org_lock and reload_org, so refreshing and saving orgs there doesn't fail. - On invalid_grant, the error includes Salesforce's error_description. OAuth2Error now carries the HTTP response.
|
@sumeet-parmar thanks for the review. All 7 comments are addressed in 961ac98, replies inline. A few more fixes from further review are in the same commit:
|
cumulusci/oauth/exceptions.py is checked by pyright in strict mode, which failed on the untyped *args and response parameters.
|
@sf-brahm-gaur please check this feedback:
|
The identity URL's host can differ between logging in and refreshing (login.salesforce.com or the org's My Domain). Comparing the whole URL would treat that as another org or user and keep a rotated-out token, which revokes the whole token family.
|
@sumeet-parmar thanks, I looked into both. 1. Snowfakery workers. Agreed, it can still happen as you describe. It isn't new to this PR, though. On
2. Identity check. Fixed in 4a314b6. It now compares only the org ID and user ID at the end of the identity URL, so a host change alone (login.salesforce.com vs. My Domain) keeps the rotated token. Connecting again as a different user or org is still detected. The new test Full suite and the live tests against the rotating test app pass. |
Why
The CumulusCI connected app (the built-in app used by
cci org connect) is adopting Salesforce connected app security controls:Current cci releases don't send PKCE, so
cci org connectfails once the app requires it. Under rotation, current cci also loses tokens: several commands refresh without saving the new refresh token, so the next command uses a rotated-out token, and Salesforce revokes the whole token family.What changed
PKCE (
cumulusci/oauth/client.py,cumulusci/cli/org.py)generate_pkce_pair()(RFC 7636, S256, 128-character verifier).pkceflag toOAuth2ClientConfig. When it's set, the authorize URL carriescode_challengeandcode_challenge_method=S256, and the token request carriescode_verifier.cci org connectturns it on. Sending PKCE to an app that doesn't require it is harmless, so this works before and after the app changes. Marketing Cloud and other OAuth clients are unchanged.Refresh token rotation (
cumulusci/core/config/org_config.py,cumulusci/core/keychain/)OrgConfig.refresh_oauth_token()holds a per-org lock (<org>.org.lock, viafilelock; the holding process can take it again). Under the lock it re-reads the org and adopts the stored tokens if another cci process changed them since this process loaded or last saved the org. It doesn't adopt them if the org was connected again as another org or user.refresh_token_rotation. This covers every caller, including the ones that didn't save the rotated token:SalesforceCommandtasks, the Metadata APIINVALID_SESSION_IDretry, andorg shellwhen its script raises.refresh_token_rotation, cci checks the stored access token against/services/oauth2/userinfoand refreshes only when it gets 401/403. Other errors are raised rather than triggering a refresh.cci org connectstill replaces the stored one.org_lock()andreload_org(). The base keychain andSubprocessKeychainimplement them as no-ops;EncryptedFileProjectKeychainuses the org file, except withCUMULUSCI_KEYCHAIN_CLASS=EnvironmentProjectKeychain, which never writes org files.filelock>=3.15.2.Error message
invalid_grantrefresh failure now raises: "Salesforce rejected the refresh token for org X: <Salesforce's error_description>. If the token expired from inactivity or was revoked, runcci org connect Xto connect the org again." Other OAuth errors are unchanged.OAuth2Errornow carries the HTTP response.Not affected: JWT auth (
SFDX_CLIENT_ID+SFDX_HUB_KEY), sf CLI–backed orgs (scratch orgs andcci org import), and the GitHub device flow.Notes for upgraders
CUMULUSCI_ORG_*environment variables with a refresh token don't work under rotation across CI jobs: the secret holds a fixed refresh token that's dead after the first job's refresh. Unused secrets also expire after 30 days. Use JWT auth instead, asdocs/headless.mdalready describes.~/.cumulusciwith this one don't take the lock or save rotated tokens. Upgrade every install that uses the same orgs.OrgConfig.OAuth2Client:refresh_token()is now called with atimeoutkeyword argument.cci org connect <org>to recover. This already happens onmain, where every worker refreshes when it starts.Testing
SubprocessKeychain, and theinvalid_grantmessage. Each fix was checked by reverting it and confirming a test fails.test_cci×8,test_org_list×2,test_config_expensive×3,test_cross_project_tasks,UninstallPackagedIncremental) also fail onmain.org connect(PKCE login accepted), then repeatedorg info,org browser,org browser -r,org shell, aSalesforceCommandtask, and a flow