-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Several problems in the 2.0 version #41
Comments
General:
Database:
Session interface:
Thank you very much for your feedback @ziege |
To 1) Okay. To 2) Yes, that's possible, but not the best practice. To 3) I saw you changed it, but there is another one: session_token_scope_id To 4) The names were created using the table name and the used column names, and these aren't up to date. That's not important but helps to avoid naming conflicts. To 5) The fields are: "access_token" in "oauth_session_access_tokens", "auth_code" in "oauth_session_authcodes", "redirect_uri" in "oauth_session_redirects", "refresh_token" and "client_id" in "oauth_session_refresh_tokens". All default to an empty string '', which is equal to NULL in some DBMS, but as they are also NOT NULL, this default value would not be possible. I think all fields should always be filled, so simply removing the default would fix it. To 6) In Postgres this would not be a problem, because Postgres supports arrays, but MySQL and other DBMS don't. Saving the data as a string is a bit easier, right, but it has many disadvantages:
Also with 20 scopes it's only one insert, because you can insert multiple entries with one query (http://dev.mysql.com/doc/refman/5.5/en/insert.html) - as far as I know, only SqlServer < 2008 doesn't support this. And you can also get all data with one query. Suggestion (not tested):
CREATE TABLE `oauth_session_authcode_scopes` (
`session_id` int(10) UNSIGNED NOT NULL,
`scope_id` smallint(5) UNSIGNED NOT NULL,
PRIMARY KEY (`session_id`, `scope_id`),
CONSTRAINT `f_oaseausc_seid` FOREIGN KEY (`session_id`) REFERENCES `oauth_sessions` (`id`) ON DELETE CASCADE ON UPDATE NO ACTION,
CONSTRAINT `f_oaseausc_scid` FOREIGN KEY (`scope_id`) REFERENCES `oauth_scopes` (`id`) ON DELETE CASCADE ON UPDATE NO ACTION
) ENGINE=InnoDB DEFAULT CHARSET=utf8; To 7) You can have multiple auth codes with different scopes - we discussed about this earlier I think (example where you have one client and one auth server, but several resource servers where you want to connect to using different scopes). That's why I split the session and the auth codes in the table structure. The RFC indirectly mentions this, because there it's possible "to obtain additional access tokens with identical or narrower scope" using refresh tokens. Hope this helps. |
|
Hi,
great to see that the development is going one, because I didn't get any feedback on my suggestions regarding the database structure. I just updated my implementation an recognized the following problems:
General:
Database:
You renamed some fields in the oauth_client and oauth_scopes tables, which is problematic, because now some field names are reserved keyword in some DMBS (e.g. "key" in SqlServer). That's why I prefixed them...
You renamed some id fields in the table, some not - e.g. 'client_id' in 'oauth_clients' to 'id', but 'endpoint_id' in 'oauth_client_endpoints' is still the same. It's okay to name it this way, but you should use the same naming convention for all tables.
Renaming some of the proposed tables/fields is was a good idea, but the naming of the indexes and foreign keys doesn't match these new names now.
Some fields have an empty default value, which doesn't make sense in most cases and would be treatet as NULL in some DBMS - this would lead to an error there, because NULL is not allowed for these fields.
The new field "scope_ids" is missing in your SQL script. This information should be saved in a sepparate table, not in one field.
Session Interface:
Why does the new method "removeAuthCode" delete the whole session and not just the auth code. One session can have more than one Auth Code, and all are deleted with this method.
Why did you remove the client_id from "validateRefreshToken"? This is a security problem. Following the RFC, "The authorization server MUST maintain the binding between a refresh token and the client to whom it was issued." - this doesn't make sense, if this information is not checked in the validate method.
As soon as the problems are solved, I will try to provide the database structure for other DBMS.
The text was updated successfully, but these errors were encountered: