fix: added policies to RefreshToken DB

[Stellatsuu]

Closes: #524

Issue found:

Policies were added by mint_token after insert_token (in DB) before exchange_token was called, so they were never added to the DB, only to the JWT. RefreshTokens DB was also missing a Policy column.

Changes:

• RefreshTokens now has a policies column (updates to the DB tablecolumns might be needed)
• Policies are now added before inserting the token in the DB in the exchange_token function.
• mint_token function doesn't need to enrich the token anymore since the payloads already have the policies inside them, from exchange_token function.
• Removed unnecessary arguments from enrich_token function (?). I'm not sure if they are really unnecessary -> see about the lack of usage of enrich_token, : fix: added policies to RefreshToken DB #950 (comment)

Results:

made with a temporary change in BaseAccessPolicy.enrich_token to return {}, {"TemporaryRefreshPolicy"}

• In the DB, RefreshToken table:

| 019ed007e31a7ec0980b77d7bf880d28 | CREATED | vo:diracAdmin | diracAdmin:CiRBRkFFNjIzOS0wRkIzLTRFQUEtQUJGRC0zQ0MxRDQ1MzJBNDESBWxvY2Fs | {"wms": {"PolicySpecific": "TemporaryRefreshPolicy"}, "sandbox": {"PolicySpecific": "TemporaryRefreshPolicy"}} |

• In the JWT, refresh token payload:

{
  "jti": "019ed007-e31a-7ec0-980b-77d7bf880d28",
  "exp": 1781628238,
  "dirac_policies": {
    "sandbox": {
      "PolicySpecific": "TemporaryRefreshPolicy"
    },
    "wms": {
      "PolicySpecific": "TemporaryRefreshPolicy"
    }
  },
  "legacy_exchange": false
} 

[Stellatsuu]

@aldbr I have a question:

Is it normal that none of the policies (WMSAccessPolicy, SandboxAccessPolicy, LollygagAccessPolicy and MyPilotsAccessPolicy) implements this function?:

diracx/diracx-routers/src/diracx/routers/access_policies.py

Lines 93 to 104 in ebce8ad

	def enrich_tokens(
	access_payload: AccessTokenPayload, refresh_payload: RefreshTokenPayload | None
	) -> tuple[dict, dict]:
	"""Add content to access or refresh payload when issuing a token.
	Content can be whatever is desired inside the access or refresh payload.
	:param access_payload: access token payload
	:param refresh_payload: refresh token payload
	:returns: extra content for both payload
	"""
	return {}, {}

The only one having it is AlwaysAllowAccessPolicy, used for the tests. Even by adding the policies to the DB, they would only be empty dicts without this function implemented no?

Since all the work for adding policies to token is done here:

diracx/diracx-routers/src/diracx/routers/auth/token.py

Lines 56 to 76 in ebce8ad

	dirac_access_policies = {}
	dirac_refresh_policies = {}
	for policy_name, policy in all_access_policies.items():
	access_extra, refresh_extra = policy.enrich_tokens(
	access_payload, refresh_payload
	)
	if access_extra:
	dirac_access_policies[policy_name] = access_extra
	if refresh_extra:
	dirac_refresh_policies[policy_name] = refresh_extra
	# Create the access token
	access_payload.dirac_policies = dirac_access_policies
	access_token = create_token(access_payload, settings)
	# Create the refresh token
	if refresh_payload:
	refresh_payload.dirac_policies = dirac_refresh_policies
	refresh_token = create_token(refresh_payload, settings)
	elif existing_refresh_token:
	refresh_token = existing_refresh_token

[read-the-docs-community]

Documentation build overview

📚 diracx | 🛠️ Build #33197422 | 📁 Comparing 98aea44 against latest (ebce8ad)

  🔍 Preview build  

4 files changed

± admin/explanations/architecture_diagram/index.html
± admin/explanations/chart-structure/index.html
± admin/how-to/upgrading/index.html
± admin/reference/values/index.html

[chrisburr]

Is it normal that none of the policies (WMSAccessPolicy, SandboxAccessPolicy, LollygagAccessPolicy and MyPilotsAccessPolicy) implements this function?:

Yes, this is there as something that we know we will need but that we don't yet have a use for.

[aldbr]

Can you also design a migration plan for the installation already in production please (and make sure the code is able to handle that)? 🙂

Thanks!

[aldbr]

Any reason for modifying the enrich_tokens method?

[Stellatsuu]

These changes were made due to my comment here: #950 (comment)

I wasn't sure access_payload and refresh_payload had an impact or not since there was no real implementation of this method.

Also, if we need the payload to enrich the token, how can we do that? Because currently, the payload is created after we insert the token in the DB, and so, after we enrich it too:

# Enrich the token with policy specific content 
    dirac_access_policies = {}
    dirac_refresh_policies = {}
    for policy_name, policy in policies.items():
        access_extra, refresh_extra = policy.enrich_tokens() # --> with my refactor, token is enriched first here
        if access_extra:
            dirac_access_policies[policy_name] = access_extra
        if refresh_extra:
            dirac_refresh_policies[policy_name] = refresh_extra

    refresh_payload: RefreshTokenPayload | None = None

    if include_refresh_token:
        # Insert the refresh token with user details into the RefreshTokens table
        # User details are needed to regenerate access tokens later
        refresh_jti = await insert_refresh_token(
            auth_db=auth_db, subject=sub, scope=scope, policies=dirac_refresh_policies
        ) # --> we need to retrieve the JTI here

        # Generate refresh token payload
        if refresh_token_expire_minutes is None:
            refresh_token_expire_minutes = settings.refresh_token_expire_minutes
        refresh_exp = uuid7_to_datetime(refresh_jti) + timedelta(
            minutes=refresh_token_expire_minutes
        )
        refresh_payload = RefreshTokenPayload(
            jti=str(refresh_jti),
            exp=refresh_exp,
            # legacy_exchange is used to indicate that the original refresh token
            # was obtained from the legacy_exchange endpoint
            legacy_exchange=legacy_exchange,
            dirac_policies=dirac_refresh_policies,
        ) # --> the payload is only created here

Maybe one way to do that would be to create an "update_token_policies" function and call it at the end, so it would be:

• create refresh token in DB
• create refresh and access tokens payloads
• enrich them
• update refresh token and access payloads dirac_policies
• update refresh token policies in DB with function

What do you think? Do you have any other idea?

[aldbr]

When you renew the refresh token, you don't reuse the policies that you initially stored in the DB, is that expected? (then what's the point in storing the policies in the DB?)

What if the policies changed in the meantime? Then you get a different refresh token.
Please test this use case if possible

[Stellatsuu]

When you renew the refresh token, you don't reuse the policies that you initially stored in the DB, is that expected? (then what's the point in storing the policies in the DB?)

I think so, no? The issue was asking to add the policies to the DB, since they were only JWT-side, it's true that I don't see the point in storing them in DB if we don't reuse them but I also don't see a point of reusing them if the refresh token will always be re-created by calling exchange_token.

What if the policies changed in the meantime? Then you get a different refresh token.

Since we don't reuse the policies from the DB and the token is re-created anyway, you'll have a new token with the new policies?

[aldbr]

I think it should be null by default, at least for the migration (because old refresh tokens don't have Policies. Also it would probably by null permanently, am I wrong?

[Stellatsuu]

I agree about the null by default and I think it's automatically assigned for older values when you add the column in the DB with ALTER TABLE RefreshTokens ADD COLUMN Policies JSON; (sql-alchemy only auto-check for new tables, not new columns, I don't know if there's a setting for that), at least, it did it for my older tokens. I will re-check that if you want and add a default anyway.

I don't understand that:

Also it would probably by null permanently, am I wrong?

As I mentionned in the PR, the policies are stored correctly in the DB, it's not null:

made with a temporary change in BaseAccessPolicy.enrich_token to return {}, {"TemporaryRefreshPolicy"}

• In the DB, RefreshToken table:

| 019ed007e31a7ec0980b77d7bf880d28 | CREATED | vo:diracAdmin | diracAdmin:CiRBRkFFNjIzOS0wRkIzLTRFQUEtQUJGRC0zQ0MxRDQ1MzJBNDESBWxvY2Fs | {"wms": {"PolicySpecific": "TemporaryRefreshPolicy"}, "sandbox": {"PolicySpecific": "TemporaryRefreshPolicy"}} |

If you're talking about this case: #950 (comment), I will take a look at it.