Skip to content

Conversation

paulastock
Copy link
Contributor

@paulastock paulastock commented Aug 1, 2025

Fixes #1486

Changes proposed in this pull request:

  • notify the user via mail about expiring access tokens:
    • using the email address provided in the user profile
    • in two cases: access token expiring in 7 days, access token has now expired (and was deleted)
  • restructuring in publisher module for Postgres Notification Listener and corresponding channels (using a channelHandler class to provide a channel to listen to (including channel name and preprocessing function))

How to test:

  • In docker-compose.yml, set deploy: replicas: 1 for services rabbitmq, publisher and mail
  • If not already set, in .env set the variable HOST_URL to =http://localhost (mail-related env variables are also required)
  • In /background-services/src/main/java/nl/esciencecenter/rsd/Main.java change the task execution time from LocalTime.MIDNIGHT to a closer point in time with LocalTime.of(HOUR_INT, MINUTE_INT) (must be in UTC)
  • Run the project, log into RSD and in the user settings, set the email address
  • In the user settings under API Access Tokens:
    • create one token that expires today (in the UI, this only works by typing in the date numbers and not using the date picker)
    • create one token that expires in 7 days
    • create one token that expires later than that
  • After the task in background-services has run, you should have received two emails: one about your token expiring in 7 days and one about the token that has expired today

PR Checklist:

  • Increase version numbers in docker-compose.yml
  • Link to a GitHub issue
  • Update documentation
  • Tests

@paulastock paulastock self-assigned this Aug 1, 2025
@paulastock paulastock force-pushed the 1486-notify-about-expiring-api-access-tokens branch from d75a59e to 553dd19 Compare August 1, 2025 07:44
@paulastock paulastock marked this pull request as ready for review August 1, 2025 07:58
Copy link
Collaborator

@ewan-escience ewan-escience left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In /background-services/src/main/java/nl/esciencecenter/rsd/Main.java change the task execution time from LocalTime.MIDNIGHT to a closer point in time with LocalTime.of(HOUR_INT, MINUTE_INT)

For me, I had to set the local time to the UTC time, so testers should take that into account.

create one token that expires today

This is not possible in the UI, some instructions should be added here. The easiest is to have the network tab of the dev tools open, create an access token through the UI, then edit and resend the POST request with a date and name of your choice.

I got an email for the token that expires today, but the email said it would expire in 7 days. The token did get deleted though. Furthermore, I didn't get any email for my tokens that expire in 7 or 8 days.

Furthermore, can you make sure every python file ends with a newline and indents using tabs?

@paulastock paulastock force-pushed the 1486-notify-about-expiring-api-access-tokens branch from 553dd19 to 230b4f9 Compare August 5, 2025 07:43
@cmeessen
Copy link
Contributor

cmeessen commented Aug 5, 2025

Furthermore, can you make sure every python file ends with a newline and indents using tabs?

The existing Python files were initially formatted with Black, with the only change as the line length set to 79. I would refrain from replacing all spaces with tabs and stick to the existing format. Furthermore, I suggest to add a CI job to verify that everything sticks to Black.

@jmaassen
Copy link
Member

jmaassen commented Aug 5, 2025

When I create a token with today's date, it seems to expire immediately?

Screenshot_2025-08-05_11-09-37

I created the tokens at 11:10 and set the background task to LocalTime.NOON. I'll let it run to see if I get the email notification

EDIT: I get an email notification of the test1 token at 14:00. Looking at the logs, the timezone in the container is 2h behind local time.

@ewan-escience
Copy link
Collaborator

The existing Python files were initially formatted with Black, with the only change as the line length set to 79. I would refrain from replacing all spaces with tabs and stick to the existing format. Furthermore, I suggest to add a CI job to verify that everything sticks to Black.

Sincere question, why prefer spaces? There is an accessibility argument for tabs. Is there any reason why spaces are inherently better than tabs? And I get that Black doesn't support tabs, but we should pick the tools that adapt to our needs.

@paulastock can you make sure all files end with a newline? See e.g. this.

@cmeessen
Copy link
Contributor

cmeessen commented Aug 5, 2025

Sincere question, why prefer spaces?

PEP 8 recommends that spaces should be used by default, and tabs should be used only to remain consistent with existing files.

I recognise the argument that it improves accessibility of the code. However, spaces vs tabs was neither addressed in #1475 nor #1525. Thus, I think it should not be discussed in this PR either. In my opinion, a dedicated issue to find a solution would be best.

@ewan-escience
Copy link
Collaborator

Thus, I think it should not be discussed in this PR either. In my opinion, a dedicated issue to find a solution would be best.

Fair enough.

@paulastock when you want me to review this again, can you re-request it? I'm assuming you're still working on it until you do this.

Copy link
Contributor

@cmeessen cmeessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generated an access token valid until today. The UI displays that the access token expired today at 02:00:00. The eMail notification about this token was submitted correctly.

But if I create an access token that is valid until today + 7 days, some error in the database happens and the publisher exits with error code 0:

background-services-1  | 12:03:00.000 [pool-2-thread-1] INFO  n.e.rsd.ScheduledService -- Delete Expired Access Tokens Service: Performing scheduled task
database-1             | 2025-08-05 12:03:00.124 UTC [33] ERROR:  invalid input syntax for type uuid: "None"
database-1             | 2025-08-05 12:03:00.124 UTC [33] CONTEXT:  unnamed portal parameter $1 = '...'
database-1             | 2025-08-05 12:03:00.124 UTC [33] STATEMENT:  WITH pgrst_source AS ( SELECT "public"."user_profile"."email_address" FROM "public"."user_profile" WHERE  "public"."user_profile"."account" = $1    )  SELECT null::bigint AS total_result_set, pg_catalog.count(_postgrest_t) AS page_total, coalesce(json_agg(_postgrest_t), '[]') AS body, nullif(current_setting('response.headers', true), '') AS response_headers, nullif(current_setting('response.status', true), '') AS response_status, '' AS response_inserted FROM ( SELECT * FROM pgrst_source ) _postgrest_t
background-services-1  | 12:03:00.127 [pool-2-thread-1] INFO  n.e.rsd.ScheduledService -- Delay till execution for Delete Expired Access Tokens Service: 86399
publisher-1            | 0
publisher-1 exited with code 0

I started the services without altering the compose file but rather used this command to control the containers:

docker compose up --scale rabbitmq=1 --scale mail=1 --scale publisher=1 --scale scrapers=0

@jmaassen
Copy link
Member

jmaassen commented Aug 5, 2025

Sincere question, why prefer spaces?

PEP 8 recommends that spaces should be used by default, and tabs should be used only to remain consistent with existing files.

The same is true for Java by the way. Sun/Oracle prescribes 4 spaces, Google 2 spaces. These are the two dominant Java code styles.

@ewan-escience
Copy link
Collaborator

I get the same error as @cmeessen under the same circumstances (i.e. a token that expires in 7 days). Maybe try printing the payload you get from the listening channels in the Python code, as it looks like the account ID is not passed on properly.

@paulastock paulastock force-pushed the 1486-notify-about-expiring-api-access-tokens branch 2 times, most recently from 4a69f34 to d4f0c4b Compare August 7, 2025 09:56
Copy link
Collaborator

@ewan-escience ewan-escience left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, everything seems to work now. There are still two HTML files that don't end in a newline, can you fix that?

@paulastock paulastock force-pushed the 1486-notify-about-expiring-api-access-tokens branch from d4f0c4b to 3f02ab6 Compare August 7, 2025 12:40
Copy link
Contributor

@cmeessen cmeessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created three token:

  • test (expires today)
  • test2 (expires today+6)
  • test3 (expires today+7)

I did receive two emails:

RSD: Your API access token expired

Your API access token "test" has expired and was deleted. If you are still using this token, you need to generate a new one.

RSD: Your API access token will expire in 7 days

Your API access token "test" will expire in 7 days. If you are still using this token, you need to generate a new one.

In the second case it did not pick up the correct name "test3".

@cmeessen
Copy link
Contributor

cmeessen commented Aug 11, 2025

I did some more additional tests, just in case that the mail service did not pick up the numbers of the names. I created a token "a" and "b", and the email that was sent in both cases was mentioning token "a".

/edit:

Here is a compose command to trigger the job without having to re-compile the background services every time:

docker compose exec database psql -U rsd -d rsd-db -c "SELECT cleanup_expired_token()"

@cmeessen
Copy link
Contributor

cmeessen commented Aug 11, 2025

Correction:

What I wrote below is incorrect, I could not reproduce it another time. I will update this comment if I can fully identify the issue.


I found the reason of the uuid: "None" error. It happens if the publisher has nothing to do when running cleanup_expired_token(). To reproduce start an empty instance with all required services running:

docker compose up --scale rabbitmq=1 --scale mail=1 --scale publisher=1 --scale scrapers=0

Wait until everything started. Then run

docker compose exec database psql -U rsd -d rsd-db -c "SELECT cleanup_expired_token()"

Output:

database-1             | 2025-08-11 12:31:52.284 UTC [33] ERROR:  invalid input syntax for type uuid: "None"
database-1             | 2025-08-11 12:31:52.284 UTC [33] CONTEXT:  unnamed portal parameter $1 = '...'
database-1             | 2025-08-11 12:31:52.284 UTC [33] STATEMENT:  WITH pgrst_source AS ( SELECT "public"."user_profile"."email_address" FROM "public"."user_profile" WHERE  "public"."user_profile"."account" = $1    )  SELECT null::bigint AS total_result_set, pg_catalog.count(_postgrest_t) AS page_total, coalesce(json_agg(_postgrest_t), '[]') AS body, nullif(current_setting('response.headers', true), '') AS response_headers, nullif(current_setting('response.status', true), '') AS response_status, '' AS response_inserted FROM ( SELECT * FROM pgrst_source ) _postgrest_t
publisher-1            | 0
publisher-1 exited with code 0

Do we want to address this in this PR or create an issue for that?

@cmeessen cmeessen self-requested a review August 11, 2025 13:21
@cmeessen
Copy link
Contributor

I tried to reproduce the errors again, but was not successful and am a bit puzzled as to why they were occurring in the first place.

@ewan-escience
Copy link
Collaborator

@cmeessen just to be sure, are you using the latest version of this branch? Some force pushes were made, so you might have to delete the branch and pull it again.

@cmeessen
Copy link
Contributor

@cmeessen just to be sure, are you using the latest version of this branch? Some force pushes were made, so you might have to delete the branch and pull it again.

Yes, I did a force reset and git pull confirms that the branch is Already up to date.

Copy link
Contributor

@dmijatovic dmijatovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get email notification about the token that expires in 7 days.

I do not get notification about expired token that should be automatically deleted. I tried DELETE SQL statement in DBeaver which also returns 0 deleted.

These are mine tokens

image

@dmijatovic dmijatovic self-requested a review August 12, 2025 10:06
Copy link
Contributor

@dmijatovic dmijatovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some more thinking and discussing it with Ewan we came to conclusion that it is beter to use CURRENT_TIMESTAMP instead of expires_at::date in DELETE statement.

So the DELETE statement should be updated to

DELETE FROM user_access_token WHERE expires_at <= CURRENT_TIMESTAMP;

@paulastock paulastock force-pushed the 1486-notify-about-expiring-api-access-tokens branch from 3f02ab6 to 5ca7f20 Compare August 13, 2025 07:34
Copy link

@paulastock paulastock requested a review from dmijatovic August 13, 2025 07:59
Copy link
Contributor

@cmeessen cmeessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rebuilt using --no-cache and cannot reproduce any of the bugs above.

Copy link
Contributor

@dmijatovic dmijatovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work! Thank you.

@paulastock Please merge this PR?

@paulastock paulastock merged commit 9294e94 into main Aug 14, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notify the user about expiring API access tokens
5 participants