-
Notifications
You must be signed in to change notification settings - Fork 25
Adding pilot registrations and authentification (Router) #421
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Adding pilot registrations and authentification (Router) #421
Conversation
e74fe72
to
9d1c062
Compare
The failed CI i'm not sure if I have to regenerate the client manually. |
Yes, you need to regenerate the client manually, here is the documentation: https://github.com/DIRACGrid/diracx/blob/main/docs/CLIENT.md#updating-the-client If you have any trouble, please let me know |
a269416
to
8645c01
Compare
if "foreign key" in str(e.orig).lower(): | ||
raise PilotNotFoundError(pilot_id=pilot_id) from e | ||
if "duplicate entry" in str(e.orig).lower(): | ||
raise PilotAlreadyExistsError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These look a bit fragile (e.g. at the moment we are effectively only supporting MySQL, but what if we add support also for e.g. PG?).
Maybe there's nothing different that can be done, but worth checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just went to the code of SQLAlchemy, there's indeed an IntegrityError
, but nothing is generic. We have to get some db-specific error: psycopg2.errors.ForeignKeyViolation
for postgres, if error_code == 2291:
for oracle, ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you rely on an error code instead of relying on a string at least?
Also, it seems you are not using and testing the case where PilotAlreadyExistsError
is raised (or I possibly missed it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we check if an error is an instance of another module pymysql
we could potentially catch some errors as code that are specific on a db
. And even with that, I saw errors where people had to use both IntegrityError
from sql-alchemy and pymy
integrity error because of a bad handling..
It is not pretty, and you can read this response: https://stackoverflow.com/a/70714697
Also, it seems you are not using and testing the case where PilotAlreadyExistsError is raised (or I possibly missed it)
This part add_pilot_credentials
is not used yet but soon will be when Dirac or another entity will register pilots on DiracX and add credentials. I currently didn't catch it, because HTTPExceptions
are to be raised on a router, and in the logic it will be automatically raised.
I don't know if it is fine to raise an error from the logic and raise the same one to the router: in a way it helps understand from the logic the potential, in another, it adds code...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll open an issue for this, to later fix this
5e80165
to
b22d1dc
Compare
Modified from |
536c2a5
to
a38f6ea
Compare
Tested with this Pilot PR version and worked successfully. Could retrieve a DiracX token from a Pilot. |
252da7c
to
b3822cd
Compare
If someone has a solution for this CI, I'm all ears. I moved a function as suggested above to |
8730f95
to
44310ed
Compare
if "foreign key" in str(e.orig).lower(): | ||
raise PilotNotFoundError(pilot_id=pilot_id) from e | ||
if "duplicate entry" in str(e.orig).lower(): | ||
raise PilotAlreadyExistsError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you rely on an error code instead of relying on a string at least?
Also, it seems you are not using and testing the case where PilotAlreadyExistsError
is raised (or I possibly missed it)
44310ed
to
70acf70
Compare
[DB Specific bug:]
|
c1da39c
to
be87858
Compare
b45a02d
to
2099d7b
Compare
87a4ac9
to
2b868a9
Compare
2b868a9
to
99357b9
Compare
10124b9
to
5c5dba7
Compare
587b280
to
e9f883a
Compare
e9f883a
to
21fa119
Compare
21fa119
to
9298ee6
Compare
Changes
Endpoints
Adding a pilot service with some endpoints:
POST /
creates a pilot with (if not prevented) a secretDELETE /
deletes pilots by stampDELETE /interval
deletes pilots that lived more than n daysPOST /token
exchanges a pilot secret for a tokenPOST /refresh-token
refresh a pilot tokenPOST /fields/secrets
creates secretsPATCH /fields/secrets
associates a pilot with a secretPATCH /fields/jobs
associates a pilot with jobsPATCH /fields
helps modifying pilot fields (benchmark, gridsite, ...)GET /search
searchs for pilots with parametersNote
The
DELETE /interval
is there because we need it directly and because it is faster, but we can simplify it withGET /search
thenDELETE /
.Security Model
As the security model dictates, pilot secrets are strings, and hashed in the db itself.
Important
For the JWT perspective, we need to chose whether a pilot will need refresh tokens or not, and how long a token will live to implement it.
These changes are mandatory for this PR.
After offline discussions: A pilot will have a different token (refresh and access), and with a different duration.