Make google id case insensitive

[damithc]

From dam...@gmail.com on February 01, 2013 12:25:45

Instructors cannot login if there is a case mismatch between the one in TEAMMATES and the one stored by Google. We should make it case insensitive.

Original issue: http://code.google.com/p/teammatespes/issues/detail?id=616

[damithc]

From mail.Haoqiang@gmail.com on February 03, 2013 23:53:40

Can I do this? Since it's my problem :p

[damithc]

From dam...@gmail.com on February 04, 2013 02:52:08

Sure. Give it a try :-)

Owner: mail.Haoqiang@gmail.com
Labels: Reviewer-Damith

[damithc]

From dam...@gmail.com on February 05, 2013 02:51:48

Issue 465 has been merged into this issue.

Cc: whisperi...@gmail.com

[damithc]

From dam...@gmail.com on February 05, 2013 02:52:48

Haoqiang, have a look at the comments in Issue 465 . This issue is more complicated that it looks at first.

[damithc]

From mail.Haoqiang@gmail.com on February 05, 2013 21:58:51

Project Member #1 whisperingrwind We can make a uniform format (all smallcase) for googleID in the createAccount() method At the method Logic.getLoggedInUser(), we retrieve the logged in user's google Id then small-case it before any comparisons. Or simply just do a googleId.toLowerCase() at createAccount() and getAccount()

Is the above suggestion solved?

Anyway, for my case I still can use the student feature, which means the case-sensitive problem may not be the whole system's problem. I'll do some testing before start. By the way, my id is indeed lower case in gmail and all other places, only when coming to google code, it become upper case. :(

[damithc]

From dam...@gmail.com on February 07, 2013 18:56:50

There is one complication to consider. We already have some data in the database that uses googleId as the key. They may not be in small case.

[damithc]

From mail.Haoqiang@gmail.com on February 11, 2013 06:21:54

Current solution status:

After many tests, I found that the googleId with capital letters inside, sometimes return in lower case, sometimes return in normal case. However, the Instructor id only have one lower and upper case.This cause the issue when calling @private Account getAccountEntity(String googleId) for example: when I login, the query will search for mail.haoqiang, but the one in database is mail.Haoqiang, wise-versa. I try many times on my deployed online version, both case happened.

The best solution is to modify all queries: query = "select from " + Account.class.getName()+ " where this.googleId.toLowercase == \"" + "\""; But further testing, I found this: Interbase does not support the LOWER, SUBSTRING, or INSTR SQL functions, which means that toLowerCase(), indexOf(), and substring() methods in JDOQL cannot be used. http://docs.oracle.com/cd/E13189_01/kodo/docs324/supported_databases.html Other solution is to change all the data in database to lowercases or write a script to generate a lower case version of those special ids.

@Dr.Damith To further test my assumption, can you add both mail.Haoqiang and mail.haoqiang to the Instructor list?

Cc: -whisperi...@gmail.com

[damithc]

From dam...@gmail.com on February 11, 2013 06:31:47

Done :-) Added mail.Haoqiang but without any sample courses.

[damithc]

From mail.Haoqiang@gmail.com on February 11, 2013 06:59:07

Thanks, I can enter instructor page now.

Problem is mostly solved, now GoogleId is not case sensitive.

Next problem,

1. The Instructor still can add Uppercase ids, which is quite risky.
2. I found the name displayed in the jsp header file are intentionally cast to lowercase. This is not useful, I think we'd better remove it.

[damithc]

From dam...@gmail.com on February 11, 2013 07:21:37

Yes, downcasting the username can be misleading. See if you can change it, possible as a separate issue.

[damithc]

From mail.Haoqiang@gmail.com on February 12, 2013 23:07:23

https://code.google.com/r/mailhaoqiang-teammates/source/detail?r=47c8734c1f94d5e82334c6209dbd271fb72ae36a&name=Issue616 https://code.google.com/r/mailhaoqiang-teammates/source/detail?r=7ab460c75ee9abe1fd464f0f26d2fc89a6ecaa3c&name=Issue616 These are the two binary changes made incrementally.

I'll change that testing problem later, or should I add that to another issue and change a better method?

Status: ReadyForReview

[damithc]

From mail.Haoqiang@gmail.com on February 12, 2013 23:08:25

Sorry, two changes made incrementally. No binary.

[damithc]

From dam...@gmail.com on February 13, 2013 02:15:51

I need some time to review this. I don't like that we are using .toLowerCase() all over the place. We should find exactly where it needs to be done rather than do it everywhere we can. Also consider that when creating new instructors, sample courses are created by using the google id as part of the course id. That means some course IDs are also relevant to this issue. The id field (not currently used for anything) of the Instructor objects also contain the google id.

[damithc]

From mail.Haoqiang@gmail.com on February 13, 2013 02:50:15

Yes, that's what I have done. I have read through all the codes about data. Sad thing is that there are 4 kinds of changes that need to be changed. So for each case, only account and instructor affect, which I think is quite minimum.

1. To handle future creation problems: this is done in the core data entities. Account, Instructor and coordinator.
2. Some tests uses data transfer object to create entity and testing, so these class also needs change. AccountData and InstructorData
3. For existing upper case ids, we need to retrieve them, so query needs to change. AccountsDb
4. Many test cases use original data to compare, has to be changed. AdminHomePageUITest, LogicTest and BackDoor test

I can show you some test cases tomorrow.

p.s. sample courses are created by using the google id as part of the course id. That means some course IDs are also relevant to this issue. For future cases the course id will be in lower case, for old cases, even though it's in normal case, but still usable.

[damithc]

From mail.Haoqiang@gmail.com on February 13, 2013 08:47:28

small bug found in the merge method. All test pass. More test done, really cannot minimise the changes https://code.google.com/r/mailhaoqiang-teammates/source/detail?r=867ff3ba8e4da8bd7f863d2519404c1d33442536&name=Issue616

[damithc]

From mail.Haoqiang@gmail.com on February 13, 2013 17:22:17

https://code.google.com/r/mailhaoqiang-teammates/source/detail?r=b988881c830711696d440b9894a9a0f2d097c0a9&name=Issue616b Better solution, only one file affected. But Google id in db will be in any cases.

[damithc]

From mail.Haoqiang@gmail.com on February 18, 2013 20:42:37

https://code.google.com/r/mailhaoqiang-justanotherclone/source/detail?r=dc9a46c5ab1292b2eecad6303280b6f25514ff46&name=Issue616 Only change in the query part, all test pass.

For user id part, the logic will pass the google user nick name to generic helper.

UserType userType = new UserType(user.getNickname());

Problem is the .getNickname() is sometime upper, sometime lower. Should we change to

UserType userType = new UserType(user.getGoogleId);

[damithc]

From dam...@gmail.com on February 19, 2013 02:09:58

Looks like a neat solution :-) Minor refactorings requested. Also, modify AccountsDbTest to cover this new behavior?

[damithc]

From mail.Haoqiang@gmail.com on February 19, 2013 02:59:25

So I'll add some test cases:

1. add two user with same id but different case. (previous can, now cannot)
2. add one account in lower case, retrieve it by upper case. (previous cannot, now can)
3. reverse 2.

+. Also for the Mixed instructor info problem(courses will be merged if the instructor name is the same, regardless of cases). Do I need to create a test case for that?

[damithc]

From dam...@gmail.com on February 19, 2013 03:39:56

I guess we don't need test cases for the Mixed instructor info problem, as long as manual test is done. That is because this is a transient problem affecting demo courses of existing instructors only (am I right?). All existing instructors have their correct googleId in the database.

Status: ChangesRequested

[damithc]

From mail.Haoqiang@gmail.com on February 19, 2013 04:13:30

Oops, unfortunately, things is bit complicated here.

Currently instructors are added manually and the googleId of the instructor added has no constrain to it. Which means the initial create of instructor can be in any case. What's worse I just found that if the instructor change the original googleId, the old one will not be deleted.

In the future, unless the create instructor method takes in correct googleId, the mixed cases problem will always exist.

[damithc]

From dam...@gmail.com on February 19, 2013 05:21:31

Well, that's precisely the problem we are trying to fix :-p When folks apply for an account, they might not give us the google Id in the correct case. Later, they will complain that they cannot login. So, that problem isn't fixed?

[damithc]

From dam...@gmail.com on February 19, 2013 05:27:56

Basically, what we want is, the case of the google ID stored in the database may be different from the correct case, but the person should still be able to use the system.

[damithc]

From mail.Haoqiang@gmail.com on February 19, 2013 05:33:48

When folks apply for an account, they might not give us the google Id in the correct case. Later, they will complain that they cannot login. <-This is fixed, they can now login,

But the data in the database is not changed. I only made the query case insensitive so that we can always get data.

Ok, what I said above is the same as Comment #24.

[damithc]

From dam...@gmail.com on February 19, 2013 06:26:04

I think that is fine Haoqiang. As long as they can use the system, we don't need the exact Google id. In future, we can add code to correct the stored googleId automatically when the user logs in.

[damithc]

From dam...@gmail.com on February 20, 2013 06:11:08

Labels: Difficulty-Medium

[damithc]

From dam...@gmail.com on February 20, 2013 06:18:54

Labels: -Difficulty-Medium Difficulty-High

[damithc]

From mail.Haoqiang@gmail.com on February 21, 2013 01:24:53

test cases: https://code.google.com/r/mailhaoqiang-justanotherclone/source/detail?r=3c054ccb2b7013f12dd0029fb853c6b77016fc10&name=Issue616 refactoring: https://code.google.com/r/mailhaoqiang-justanotherclone/source/detail?r=9127e7bfc16e0e76e738d3a409ecd6208b2b37e1&name=Issue616 Results:

Id in database Id in real time is retrievable lower lower yes lower upper yes upper upper yes upper lower no ->query cannot cast cases

Status: ReadyForReview

[damithc]

From mail.Haoqiang@gmail.com on February 21, 2013 01:51:55

Change the merge function to union, much safer https://code.google.com/r/mailhaoqiang-justanotherclone/source/detail?r=3124c099907b9127f3860710ed02b49699bcc9f9&name=Issue616

[damithc]

From mail.Haoqiang@gmail.com on February 21, 2013 02:33:47

A trick to make lower case search upper case in the future possible. But will need to modify two entities. Benefit, google id will be totally transparent in the future. Well not very useful. :( Just try to fill up all cases in the table above. :) https://code.google.com/r/mailhaoqiang-justanotherclone/source/diff?spec=svna68379aad1d72f06a713b28f3b34260abb7b9b8f&name=Issue616b&r=a68379aad1d72f06a713b28f3b34260abb7b9b8f&format=side&path=/src/main/java/teammates/storage/entity/Account.java https://code.google.com/r/mailhaoqiang-justanotherclone/source/diff?format=side&name=Issue616b&path=/src/main/java/teammates/storage/entity/Instructor.java&r=a68379aad1d72f06a713b28f3b34260abb7b9b8f&spec=svna68379aad1d72f06a713b28f3b34260abb7b9b8f https://code.google.com/r/mailhaoqiang-justanotherclone/source/detail?r=604436f2b68e6445ad342b44a12e703cb8a1f778&name=Issue616b

[damithc]

From dam...@gmail.com on February 22, 2013 17:43:49

Haoqiang, the 4th case can happen too. i.e., the actual google id is lower case but the google ID typed in by the use is upper case. It seems such cases cannot be handled using the current code?

Furthermore, there are three items to consider in the permutations table: actual id (id stored by Google), our id (id stored in our database), input id (id typed in by the user e.g., when an instructors adds a new instructor to his course).

[damithc]

From mail.Haoqiang@gmail.com on February 22, 2013 18:33:18

google ID typed in by the use is upper <- This case is really rare.

Any way, suppose I have a "TeSting" in database, my actual id is "testing" Google doesn't allow to make changes to the data in data store, refer to comment #7 That is, I can only manipulate "testing". Then there's no way I can know the really database one.

this case can only be handle by adding a new filed to the entity, refer to comment #31

[damithc]

From dam...@gmail.com on February 23, 2013 23:32:31

Need some time to think this over. :-)

[damithc]

From mail.Haoqiang@gmail.com on February 24, 2013 00:29:48

You mind we discuss this issue in real time? I think this issue has hang for quite a long time.

[damithc]

From dam...@gmail.com on February 24, 2013 02:40:05

Yes, let's do that. When can we meet sometime next week?

[damithc]

From mail.Haoqiang@gmail.com on February 24, 2013 05:08:52

I'm ok with tomorrow or Tuesday.

[damithc]

From dam...@gmail.com on February 24, 2013 23:37:20

Putting this on hold because we might not need this if we implement 'join' functionality for instructors as well. Thanks Haoqiang for all the tireless work done to resolve this issue :-)

Status: Done

[damithc]

[Change Log] status: null ~> status.closed